binarybuffer: Fix str_to_buf() parsing function

The function str_to_buf() was too benevolent and did
not perform sufficient error checking on the input
string being parsed. Especially:

- Invalid numbers were silently ignored.
- Out-of-range numbers were silently truncated.

The following commands that use str_to_buf()
were affected:

- reg (when writing a register value)
- set_reg
- jtag drscan

This pull request fixes that by:

- Rewriting str_to_buf() to add the missing checks.
- Adding function command_parse_str_to_buf() which can
  be used in command handlers. It parses the input
  numbers and provides user-readable error messages
  in case of parsing errors.

Examples:

jtag drscan 10 huh10

- Old behavior: The string "huh10" is silently
  converted to 10 and the command is then executed.
  No warning error or warning is shown to the user.
- New behavior: Error message is shown:
  "'huh10' is not a valid number"

reg pc 0x123456789

Assuming the "pc" is 32 bits wide:

- Old behavior: The register value is silently
  truncated to 0x23456789 and the command is performed.
- New behavior: Error message is shown to the user:
  "Number 0x123456789 exceeds 32 bits"

Change-Id: I079e19cd153aec853a3c2eb66953024b8542d0f4
Signed-off-by: Jan Matyas <jan.matyas@codasip.com>
Reviewed-on: https://review.openocd.org/c/openocd/+/8315
Tested-by: jenkins
Reviewed-by: Marek Vrbka <marek.vrbka@codasip.com>
Reviewed-by: Antonio Borneo <borneo.antonio@gmail.com>

src/helper/binarybuffer.c

@@ -102,7 +102,6 @@ bool buf_cmp_mask(const void *_buf1, const void *_buf2,
 	return buf_cmp_trailing(buf1[last], buf2[last], mask[last], trailing);
 }
 
-
 void *buf_set_ones(void *_buf, unsigned size)
 {
 	uint8_t *buf = _buf;
@@ -206,36 +205,75 @@ char *buf_to_hex_str(const void *_buf, unsigned buf_len)
 	return str;
 }
 
-/** identify radix, and skip radix-prefix (0, 0x or 0X) */
+static bool str_has_hex_prefix(const char *s)
-static void str_radix_guess(const char **_str, unsigned *_str_len,
-	unsigned *_radix)
 {
-	unsigned radix = *_radix;
+	/* Starts with "0x" or "0X" */
-	if (radix != 0)
+	return (s[0] == '0') && (s[1] == 'x' || s[1] == 'X');
-		return;
-	const char *str = *_str;
-	unsigned str_len = *_str_len;
-	if (str[0] == '0' && (str[1] == 'x' || str[1] == 'X')) {
-		radix = 16;
-		str += 2;
-		str_len -= 2;
-	} else if ((str[0] == '0') && (str_len != 1)) {
-		radix = 8;
-		str += 1;
-		str_len -= 1;
-	} else
-		radix = 10;
-	*_str = str;
-	*_str_len = str_len;
-	*_radix = radix;
 }
 
-int str_to_buf(const char *str, unsigned str_len,
+static bool str_has_octal_prefix(const char *s)
-	void *_buf, unsigned buf_len, unsigned radix)
 {
-	str_radix_guess(&str, &str_len, &radix);
+	/* - starts with '0',
+	 * - has at least two characters, and
+	 * - the second character is not 'x' or 'X' */
+	return (s[0] == '0') && (s[1] != '\0') && (s[1] != 'x') && (s[1] != 'X');
+}
 
-	float factor;
+/**
+ * Try to identify the radix of the number by looking at its prefix.
+ * No further validation of the number is preformed.
+ */
+static unsigned int str_radix_guess(const char *str)
+{
+	assert(str);
+
+	if (str_has_hex_prefix(str))
+		return 16;
+
+	if (str_has_octal_prefix(str))
+		return 8;
+
+	/* Otherwise assume a decadic number. */
+	return 10;
+}
+
+/** Strip leading "0x" or "0X" from hex numbers or "0" from octal numbers. */
+static void str_strip_number_prefix_if_present(const char **_str, unsigned int radix)
+{
+	assert(radix == 16 || radix == 10 || radix == 8);
+	assert(_str);
+
+	const char *str = *_str;
+	assert(str);
+
+	if (radix == 16 && str_has_hex_prefix(str))
+		str += 2;
+	else if (radix == 8 && str_has_octal_prefix(str))
+		str += 1;
+
+	/* No prefix to strip for radix == 10. */
+
+	*_str = str;
+}
+
+int str_to_buf(const char *str, void *_buf, unsigned int buf_len,
+	unsigned int radix, unsigned int *_detected_radix)
+{
+	assert(radix == 0 || radix == 8 || radix == 10 || radix == 16);
+
+	if (radix == 0)
+		radix = str_radix_guess(str);
+
+	if (_detected_radix)
+		*_detected_radix = radix;
+
+	str_strip_number_prefix_if_present(&str, radix);
+
+	const size_t str_len = strlen(str);
+	if (str_len == 0)
+		return ERROR_INVALID_NUMBER;
+
+	float factor = 0.0;
 	if (radix == 16)
 		factor = 0.5;	/* log(16) / log(256) = 0.5 */
 	else if (radix == 10)
@@ -243,41 +281,69 @@ int str_to_buf(const char *str, unsigned str_len,
 	else if (radix == 8)
 		factor = 0.375;	/* log(8) / log(256) = 0.375 */
 	else
-		return 0;
+		assert(false);
 
-	/* copy to zero-terminated buffer */
+	const unsigned int b256_len = ceil_f_to_u32(str_len * factor);
-	char *charbuf = strndup(str, str_len);
 
-	/* number of digits in base-256 notation */
+	/* Allocate a buffer for digits in base-256 notation */
-	unsigned b256_len = ceil_f_to_u32(str_len * factor);
 	uint8_t *b256_buf = calloc(b256_len, 1);
+	if (!b256_buf) {
+		LOG_ERROR("Unable to allocate memory");
+		return ERROR_FAIL;
+	}
 
-	/* go through zero terminated buffer
+	/* Go through the zero-terminated buffer
-	 * input digits (ASCII) */
+	 * of input digits (ASCII) */
-	unsigned i;
+	for (unsigned int i = 0; str[i]; i++) {
-	for (i = 0; charbuf[i]; i++) {
+		uint32_t tmp = str[i];
-		uint32_t tmp = charbuf[i];
+		if ((tmp >= '0') && (tmp <= '9')) {
-		if ((tmp >= '0') && (tmp <= '9'))
 			tmp = (tmp - '0');
-		else if ((tmp >= 'a') && (tmp <= 'f'))
+		} else if ((tmp >= 'a') && (tmp <= 'f')) {
 			tmp = (tmp - 'a' + 10);
-		else if ((tmp >= 'A') && (tmp <= 'F'))
+		} else if ((tmp >= 'A') && (tmp <= 'F')) {
 			tmp = (tmp - 'A' + 10);
-		else
+		} else {
-			continue;	/* skip characters other than [0-9,a-f,A-F] */
+			/* Characters other than [0-9,a-f,A-F] are invalid */
+			free(b256_buf);
+			return ERROR_INVALID_NUMBER;
+		}
 
-		if (tmp >= radix)
+		if (tmp >= radix) {
-			continue;	/* skip digits invalid for the current radix */
+			/* Encountered a digit that is invalid for the current radix */
+			free(b256_buf);
+			return ERROR_INVALID_NUMBER;
+		}
 
-		/* base-256 digits */
+		/* Add the current digit (tmp) to the intermediate result
-		for (unsigned j = 0; j < b256_len; j++) {
+		 * in b256_buf (base-256 digits) */
+		for (unsigned int j = 0; j < b256_len; j++) {
 			tmp += (uint32_t)b256_buf[j] * radix;
-			b256_buf[j] = (uint8_t)(tmp & 0xFF);
+			b256_buf[j] = (uint8_t)(tmp & 0xFFu);
 			tmp >>= 8;
 		}
 
+		/* The b256_t buffer is large enough to contain the whole result. */
+		assert(tmp == 0);
 	}
 
+	/* The result must not contain more bits than buf_len. */
+	/* Check the whole bytes: */
+	for (unsigned int j = DIV_ROUND_UP(buf_len, 8); j < b256_len; j++) {
+		if (b256_buf[j] != 0x0) {
+			free(b256_buf);
+			return ERROR_NUMBER_EXCEEDS_BUFFER;
+		}
+	}
+	/* Check the partial byte: */
+	if (buf_len % 8) {
+		const uint8_t mask = 0xFFu << (buf_len % 8);
+		if ((b256_buf[(buf_len / 8)] & mask) != 0x0) {
+			free(b256_buf);
+			return ERROR_NUMBER_EXCEEDS_BUFFER;
+		}
+	}
+
+	/* Copy the digits to the output buffer */
 	uint8_t *buf = _buf;
 	for (unsigned j = 0; j < DIV_ROUND_UP(buf_len, 8); j++) {
 		if (j < b256_len)
@@ -286,14 +352,8 @@ int str_to_buf(const char *str, unsigned str_len,
 			buf[j] = 0;
 	}
 
-	/* mask out bits that don't belong to the buffer */
-	if (buf_len % 8)
-		buf[(buf_len / 8)] &= 0xff >> (8 - (buf_len % 8));
-
 	free(b256_buf);
-	free(charbuf);
+	return ERROR_OK;
-
-	return i;
 }
 
 void bit_copy_queue_init(struct bit_copy_queue *q)

src/helper/binarybuffer.h

@@ -14,6 +14,9 @@
 #include <helper/list.h>
 #include <helper/types.h>
 
+#define ERROR_INVALID_NUMBER         (-1700)
+#define ERROR_NUMBER_EXCEEDS_BUFFER  (-1701)
+
 /** @file
  * Support functions to access arbitrary bits in a byte array
  */
@@ -189,8 +192,18 @@ void *buf_set_ones(void *buf, unsigned size);
 void *buf_set_buf(const void *src, unsigned src_start,
 		  void *dst, unsigned dst_start, unsigned len);
 
-int str_to_buf(const char *str, unsigned len,
+/**
-		void *bin_buf, unsigned buf_size, unsigned radix);
+ * Parse an unsigned number (provided as a zero-terminated string)
+ * into a bit buffer whose size is buf_len bits.
+ * @param str Input number, zero-terminated string
+ * @param _buf Output buffer, allocated by the caller
+ * @param buf_len Output buffer size in bits
+ * @param radix Base of the input number - 16, 10, 8 or 0.
+ *              0 means auto-detect the radix.
+ */
+int str_to_buf(const char *str, void *_buf, unsigned int buf_len,
+	unsigned int radix, unsigned int *_detected_radix);
+
 char *buf_to_hex_str(const void *buf, unsigned size);
 
 /* read a uint32_t from a buffer in target memory endianness */

src/helper/command.c

@@ -1360,6 +1360,46 @@ int command_parse_bool_arg(const char *in, bool *out)
 	return ERROR_COMMAND_SYNTAX_ERROR;
 }
 
+static const char *radix_to_str(unsigned int radix)
+{
+	switch (radix) {
+		case 16: return "hexadecimal";
+		case 10: return "decadic";
+		case 8: return "octal";
+	}
+	assert(false);
+	return "";
+}
+
+COMMAND_HELPER(command_parse_str_to_buf, const char *str, void *buf, unsigned int buf_len,
+	unsigned int radix)
+{
+	assert(str);
+	assert(buf);
+
+	int ret = str_to_buf(str, buf, buf_len, radix, NULL);
+	if (ret == ERROR_OK)
+		return ret;
+
+	/* Provide a clear error message to the user */
+	if (ret == ERROR_INVALID_NUMBER) {
+		if (radix == 0) {
+			/* Any radix is accepted, so don't include it in the error message. */
+			command_print(CMD, "'%s' is not a valid number", str);
+		} else {
+			/* Specific radix is required - tell the user what it is. */
+			command_print(CMD, "'%s' is not a valid number (requiring %s number)",
+				str, radix_to_str(radix));
+		}
+	} else if (ret == ERROR_NUMBER_EXCEEDS_BUFFER) {
+		command_print(CMD, "Number %s exceeds %u bits", str, buf_len);
+	} else {
+		command_print(CMD, "Could not parse number '%s'", str);
+	}
+
+	return ERROR_COMMAND_ARGUMENT_INVALID;
+}
+
 COMMAND_HELPER(handle_command_parse_bool, bool *out, const char *label)
 {
 	switch (CMD_ARGC) {

src/helper/command.h

@@ -517,6 +517,17 @@ DECLARE_PARSE_WRAPPER(_target_addr, target_addr_t);
 int command_parse_bool_arg(const char *in, bool *out);
 COMMAND_HELPER(handle_command_parse_bool, bool *out, const char *label);
 
+/**
+ * Parse a number (base 10, base 16 or base 8) and store the result
+ * into a bit buffer.
+ *
+ * In case of parsing error, a user-readable error message is produced.
+ *
+ * If radix = 0 is given, the function guesses the radix by looking at the number prefix.
+ */
+COMMAND_HELPER(command_parse_str_to_buf, const char *str, void *buf, unsigned int buf_len,
+	unsigned int radix);
+
 /** parses an on/off command argument */
 #define COMMAND_PARSE_ON_OFF(in, out) \
 	COMMAND_PARSE_BOOL(in, out, "on", "off")

src/jtag/tcl.c

@@ -87,8 +87,11 @@ static COMMAND_HELPER(handle_jtag_command_drscan_fields, struct scan_field *fiel
 			LOG_ERROR("Out of memory");
 			return ERROR_FAIL;
 		}
+
 		fields[field_count].out_value = t;
-		str_to_buf(CMD_ARGV[i + 1], strlen(CMD_ARGV[i + 1]), t, bits, 0);
+		int ret = CALL_COMMAND_HANDLER(command_parse_str_to_buf, CMD_ARGV[i + 1], t, bits, 0);
+		if (ret != ERROR_OK)
+			return ret;
 		fields[field_count].in_value = t;
 		field_count++;
 	}

src/target/target.c

@@ -3128,11 +3128,18 @@ COMMAND_HANDLER(handle_reg_command)
 	/* set register value */
 	if (CMD_ARGC == 2) {
 		uint8_t *buf = malloc(DIV_ROUND_UP(reg->size, 8));
-		if (!buf)
+		if (!buf) {
+			LOG_ERROR("Failed to allocate memory");
 			return ERROR_FAIL;
-		str_to_buf(CMD_ARGV[1], strlen(CMD_ARGV[1]), buf, reg->size, 0);
+		}
 
-		int retval = reg->type->set(reg, buf);
+		int retval = CALL_COMMAND_HANDLER(command_parse_str_to_buf, CMD_ARGV[1], buf, reg->size, 0);
+		if (retval != ERROR_OK) {
+			free(buf);
+			return retval;
+		}
+
+		retval = reg->type->set(reg, buf);
 		if (retval != ERROR_OK) {
 			LOG_ERROR("Could not write to register '%s'", reg->name);
 		} else {
@@ -4788,63 +4795,64 @@ static int target_jim_get_reg(Jim_Interp *interp, int argc,
 	return JIM_OK;
 }
 
-static int target_jim_set_reg(Jim_Interp *interp, int argc,
+COMMAND_HANDLER(handle_set_reg_command)
-		Jim_Obj * const *argv)
 {
-	if (argc != 2) {
+	if (CMD_ARGC != 1)
-		Jim_WrongNumArgs(interp, 1, argv, "dict");
+		return ERROR_COMMAND_SYNTAX_ERROR;
-		return JIM_ERR;
-	}
 
 	int tmp;
 #if JIM_VERSION >= 80
-	Jim_Obj **dict = Jim_DictPairs(interp, argv[1], &tmp);
+	Jim_Obj **dict = Jim_DictPairs(CMD_CTX->interp, CMD_JIMTCL_ARGV[0], &tmp);
 
 	if (!dict)
-		return JIM_ERR;
+		return ERROR_FAIL;
 #else
 	Jim_Obj **dict;
-	int ret = Jim_DictPairs(interp, argv[1], &dict, &tmp);
+	int ret = Jim_DictPairs(CMD_CTX->interp, CMD_JIMTCL_ARGV[0], &dict, &tmp);
 
 	if (ret != JIM_OK)
-		return ret;
+		return ERROR_FAIL;
 #endif
 
 	const unsigned int length = tmp;
-	struct command_context *cmd_ctx = current_command_context(interp);
+
-	assert(cmd_ctx);
+	const struct target *target = get_current_target(CMD_CTX);
-	const struct target *target = get_current_target(cmd_ctx);
+	assert(target);
 
 	for (unsigned int i = 0; i < length; i += 2) {
 		const char *reg_name = Jim_String(dict[i]);
 		const char *reg_value = Jim_String(dict[i + 1]);
-		struct reg *reg = register_get_by_name(target->reg_cache, reg_name,
+		struct reg *reg = register_get_by_name(target->reg_cache, reg_name, false);
-			false);
 
 		if (!reg || !reg->exist) {
-			Jim_SetResultFormatted(interp, "unknown register '%s'", reg_name);
+			command_print(CMD, "unknown register '%s'", reg_name);
-			return JIM_ERR;
+			return ERROR_FAIL;
 		}
 
 		uint8_t *buf = malloc(DIV_ROUND_UP(reg->size, 8));
-
 		if (!buf) {
 			LOG_ERROR("Failed to allocate memory");
-			return JIM_ERR;
+			return ERROR_FAIL;
 		}
 
-		str_to_buf(reg_value, strlen(reg_value), buf, reg->size, 0);
+		int retval = CALL_COMMAND_HANDLER(command_parse_str_to_buf,
-		int retval = reg->type->set(reg, buf);
+			reg_value, buf, reg->size, 0);
+		if (retval != ERROR_OK) {
+			free(buf);
+			return retval;
+		}
+
+		retval = reg->type->set(reg, buf);
 		free(buf);
 
 		if (retval != ERROR_OK) {
-			Jim_SetResultFormatted(interp, "failed to set '%s' to register '%s'",
+			command_print(CMD, "failed to set '%s' to register '%s'",
 				reg_value, reg_name);
-			return JIM_ERR;
+			return retval;
 		}
 	}
 
-	return JIM_OK;
+	return ERROR_OK;
 }
 
 /**
@@ -5584,7 +5592,7 @@ static const struct command_registration target_instance_command_handlers[] = {
 	{
 		.name = "set_reg",
 		.mode = COMMAND_EXEC,
-		.jim_handler = target_jim_set_reg,
+		.handler = handle_set_reg_command,
 		.help = "Set target register values",
 		.usage = "dict",
 	},
@@ -6719,7 +6727,7 @@ static const struct command_registration target_exec_command_handlers[] = {
 	{
 		.name = "set_reg",
 		.mode = COMMAND_EXEC,
-		.jim_handler = target_jim_set_reg,
+		.handler = handle_set_reg_command,
 		.help = "Set target register values",
 		.usage = "dict",
 	},