[U-Boot] [PATCH v2] Prevented possible null dereference.

[Tom Rini]

On Mon, Aug 26, 2019 at 05:54:59AM -0700, Niv Shetrit wrote:

> Signed-off-by: Niv Shetrit <niv.shetrit at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191205/51752ed2/attachment.sig>