[U-Boot] [PATCH v2] Prevented possible null dereference.
Signed-off-by: Niv Shetrit niv.shetrit@altair-semi.com --- common/cli_hush.c | 73 ++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c index 8f86e4aa4a..c14302c3ad 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -3539,41 +3539,44 @@ static char *insert_var_value_sub(char *inp, int tag_subst) } inp = ++p; /* find the ending marker */ - p = strchr(inp, SPECIAL_VAR_SYMBOL); - *p = '\0'; - /* look up the value to substitute */ - if ((p1 = lookup_param(inp))) { - if (tag_subst) - len = res_str_len + strlen(p1) + 2; - else - len = res_str_len + strlen(p1); - res_str = xrealloc(res_str, (1 + len)); - if (tag_subst) { - /* - * copy the variable value to the result - * string - */ - strcpy((res_str + res_str_len + 1), p1); - - /* - * mark the replaced text to be accepted as - * is - */ - res_str[res_str_len] = SUBSTED_VAR_SYMBOL; - res_str[res_str_len + 1 + strlen(p1)] = - SUBSTED_VAR_SYMBOL; - } else - /* - * copy the variable value to the result - * string - */ - strcpy((res_str + res_str_len), p1); - - res_str_len = len; - } - *p = SPECIAL_VAR_SYMBOL; - inp = ++p; - done = 1; + p = strchr(inp, SPECIAL_VAR_SYMBOL) + if (p != NULL) { + *p = '\0'; + /* look up the value to substitute */ + p1 = lookup_param(inp) + if (p1 != NULL) { + if (tag_subst) + len = res_str_len + strlen(p1) + 2; + else + len = res_str_len + strlen(p1); + res_str = xrealloc(res_str, (1 + len)); + if (tag_subst) { + /* + * copy the variable value to the + * result string + */ + strcpy((res_str + res_str_len + 1), p1); + + /* + * mark the replaced text to be + * accepted as is + */ + res_str[res_str_len] = SUBSTED_VAR_SYMBOL; + res_str[res_str_len + 1 + strlen(p1)] = + SUBSTED_VAR_SYMBOL; + } else + /* + * copy the variable value to the result + * string + */ + strcpy((res_str + res_str_len), p1); + + res_str_len = len; + } + *p = SPECIAL_VAR_SYMBOL; + inp = ++p; + done = 1; + } } if (done) { res_str = xrealloc(res_str, (1 + res_str_len + strlen(inp)));
On Mon, Aug 26, 2019 at 05:54:59AM -0700, Niv Shetrit wrote:
Signed-off-by: Niv Shetrit niv.shetrit@altair-semi.com
Sorry for taking so long to get back to this. A few problems. And re-ordering the diff to make explanation clearer:
common/cli_hush.c | 73 ++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c index 8f86e4aa4a..c14302c3ad 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -3539,41 +3539,44 @@ static char *insert_var_value_sub(char *inp, int tag_subst) } inp = ++p; /* find the ending marker */
p = strchr(inp, SPECIAL_VAR_SYMBOL);
p = strchr(inp, SPECIAL_VAR_SYMBOL)
You forgot the trailing ';' so this can't compile and wasn't tested.
*p = '\0';/* look up the value to substitute */if ((p1 = lookup_param(inp))) {
p1 = lookup_param(inp)if (p1 != NULL) {
Again you forgot a trailing ';' and you've expanded equivalent statements. If we had a single set of parenthesis, in other words: if (p1 = lookup_param(inp)) { ...
it would still check if we got NULL/non-NULL, but gcc would complain: warning: suggest parentheses around assignment used as truth value [-Wparenthese]
and with the parenthesis we have, it does what we want here.
But we have the extra set of parenthesis to say we care about the result of that assignment and are doing this on purpose.
Which brings us to the other functional change (which is more visible with git show -w or similar, to ignore whitespace changes):
p = strchr(inp, SPECIAL_VAR_SYMBOL)if (p != NULL) {*p = '\0';/* look up the value to substitute */p1 = lookup_param(inp)
[I've re-included the code above back here for more context] You're making sure that if strchr gives us NULL back, we don't call lookup_param. But lookup_param handles being passed NULL correctly.
So in sum, there's no problem here. Thanks for looking around.
participants (2)
-
Niv Shetrit -
Tom Rini