This is an automated email from Gerrit.
"Jan Matyas <jan.mat...@codasip.com>" just uploaded a new patch set to Gerrit,
which you can find at https://review.openocd.org/c/openocd/+/8315
-- gerrit
commit 8e7a7638e6801dfe0391e32bcd8655d9af964291
Author: Jan Matyas <jan.mat...@codasip.com>
Date: Mon Jun 3 10:23:02 2024 +0200
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 nubmers 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:
"Number huh10 (base-10) contains an invalid digit"
reg pc 0x123456789
- 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.mat...@codasip.com>
diff --git a/src/helper/binarybuffer.c b/src/helper/binarybuffer.c
index 5f38b43ae1..b5bace2c9d 100644
--- a/src/helper/binarybuffer.c
+++ b/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,34 +205,55 @@ char *buf_to_hex_str(const void *_buf, unsigned buf_len)
return str;
}
-/** identify radix, and skip radix-prefix (0, 0x or 0X) */
-static void str_radix_guess(const char **_str, unsigned *_str_len,
- unsigned *_radix)
+/** Identify radix, and skip the radix-prefix (0, 0x or 0X) */
+static void str_radix_guess(const char **_str, unsigned int *_str_len,
+ unsigned int *_radix)
{
+ assert(_str);
+ assert(_str_len);
+ assert(_radix);
+
unsigned radix = *_radix;
- if (radix != 0)
- return;
+ assert(radix == 0 || radix == 8 || radix == 10 || radix == 16);
+
const char *str = *_str;
unsigned str_len = *_str_len;
- if (str[0] == '0' && (str[1] == 'x' || str[1] == 'X')) {
+
+ const bool has_hex_prefix = (str[0] == '0') && (str[1] == 'x' || str[1]
== 'X');
+ const bool has_oct_prefix = (str[0] == '0') && (str_len != 1);
+
+ if (has_hex_prefix && (radix == 16 || radix == 0)) {
+ /* OK, detected HEX number, strip the prefix */
radix = 16;
str += 2;
str_len -= 2;
- } else if ((str[0] == '0') && (str_len != 1)) {
+ } else if (has_oct_prefix && (radix == 8 || radix == 0)) {
+ /* OK, detected an octal number, strip the prefix */
radix = 8;
str += 1;
str_len -= 1;
- } else
+ } else if (radix == 0) {
+ /* Assume decadic number */
radix = 10;
+ }
+
*_str = str;
*_str_len = str_len;
*_radix = radix;
}
-int str_to_buf(const char *str, unsigned str_len,
- void *_buf, unsigned buf_len, unsigned radix)
+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);
+
+ unsigned int str_len = strlen(str);
+
str_radix_guess(&str, &str_len, &radix);
+ assert(radix != 0);
+
+ if (_detected_radix)
+ *_detected_radix = radix;
float factor;
if (radix == 16)
@@ -243,41 +263,70 @@ 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 */
- char *charbuf = strndup(str, str_len);
+ const unsigned int b256_len = ceil_f_to_u32(str_len * factor);
+ uint8_t *b256_buf = NULL;
- /* number of digits in base-256 notation */
- unsigned b256_len = ceil_f_to_u32(str_len * factor);
- uint8_t *b256_buf = calloc(b256_len, 1);
+ /* Allocate a buffer for digits in base-256 notation */
+ b256_buf = calloc(b256_len, 1);
+ if (!b256_buf) {
+ LOG_ERROR("Unable to allocate memory");
+ return ERROR_FAIL;
+ }
- /* go through zero terminated buffer
- * input digits (ASCII) */
- unsigned i;
- for (i = 0; charbuf[i]; i++) {
- uint32_t tmp = charbuf[i];
- if ((tmp >= '0') && (tmp <= '9'))
+ /* Go through the zero-terminated buffer
+ * of input digits (ASCII) */
+ for (unsigned int i = 0; str[i]; i++) {
+ uint32_t tmp = str[i];
+ 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
- continue; /* skip characters other than
[0-9,a-f,A-F] */
+ } else {
+ /* Characters other than [0-9,a-f,A-F] are invalid */
+ free(b256_buf);
+ return ERROR_INVALID_DIGIT;
+ }
- if (tmp >= radix)
- continue; /* skip digits invalid for the current
radix */
+ if (tmp >= radix) {
+ /* Encountered a digit that is invalid for the current
radix */
+ free(b256_buf);
+ return ERROR_INVALID_DIGIT;
+ }
- /* base-256 digits */
- for (unsigned j = 0; j < b256_len; j++) {
+ /* Add the current digit (tmp) to the intermediate result
+ * 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);
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 +335,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 i;
+ return ERROR_OK;
}
void bit_copy_queue_init(struct bit_copy_queue *q)
diff --git a/src/helper/binarybuffer.h b/src/helper/binarybuffer.h
index 3446296817..159b4cbd26 100644
--- a/src/helper/binarybuffer.h
+++ b/src/helper/binarybuffer.h
@@ -14,6 +14,9 @@
#include <helper/list.h>
#include <helper/types.h>
+#define ERROR_INVALID_DIGIT (-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 */
diff --git a/src/helper/command.c b/src/helper/command.c
index a775c730b8..49c6885740 100644
--- a/src/helper/command.c
+++ b/src/helper/command.c
@@ -1360,6 +1360,28 @@ int command_parse_bool_arg(const char *in, bool *out)
return ERROR_COMMAND_SYNTAX_ERROR;
}
+int command_parse_str_to_buf(const char *str, void *buf, unsigned int buf_len,
+ unsigned int radix)
+{
+ assert(str);
+ assert(buf);
+
+ unsigned int detected_radix = -1;
+ int ret = str_to_buf(str, buf, buf_len, radix, &detected_radix);
+ if (ret == ERROR_OK)
+ return ret;
+
+ /* Provide a user-readable error message */
+ if (ret == ERROR_INVALID_DIGIT)
+ LOG_ERROR("Number %s (base-%u) contains an invalid digit", str,
detected_radix);
+ else if (ret == ERROR_NUMBER_EXCEEDS_BUFFER)
+ LOG_ERROR("Number %s exceeds %u bits", str, buf_len);
+ else
+ LOG_ERROR("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) {
diff --git a/src/helper/command.h b/src/helper/command.h
index fc26dda81a..e3c830cd51 100644
--- a/src/helper/command.h
+++ b/src/helper/command.h
@@ -517,6 +517,18 @@ 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, produce a user-readable
+ * error message.
+ * @param str Input number as a zero-terminated string
+ * @param buf Bit buffer, allocated by the caller
+ * @param buf_len Size of the buf in bits
+ * @param radix base of the number - 16, 10, 8 (or 0 for autodetection)
+ */
+int 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")
diff --git a/src/jtag/tcl.c b/src/jtag/tcl.c
index 7995529018..e9da9ac513 100644
--- a/src/jtag/tcl.c
+++ b/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 = 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++;
}
diff --git a/src/target/target.c b/src/target/target.c
index efc168903b..6fcf323265 100644
--- a/src/target/target.c
+++ b/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 = 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 {
@@ -4834,8 +4841,13 @@ static int target_jim_set_reg(Jim_Interp *interp, int
argc,
return JIM_ERR;
}
- str_to_buf(reg_value, strlen(reg_value), buf, reg->size, 0);
- int retval = reg->type->set(reg, buf);
+ int retval = command_parse_str_to_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) {
--
