[U-Boot] [PATCH 1/1] tools: sunxi: avoid possible null pointer dereference

[U-Boot] [PATCH 1/1] tools: sunxi: avoid possible null pointer dereference

[Heinrich Schuchardt]

If endptr is NULL we should not dereference it.

The problem was indicated by cppcheck.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 tools/sunxi-spl-image-builder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/sunxi-spl-image-builder.c b/tools/sunxi-spl-image-builder.c
index d538a38813..0072a64728 100644
--- a/tools/sunxi-spl-image-builder.c
+++ b/tools/sunxi-spl-image-builder.c
@@ -433,7 +433,7 @@ int main(int argc, char **argv)
 			break;
 		case 'c':
 			info.ecc_strength = strtol(optarg, &endptr, 0);
-			if (endptr || *endptr == '/')
+			if (endptr && *endptr == '/')
 				info.ecc_step_size = strtol(endptr + 1, NULL, 0);
 			break;
 		case 'p':
-- 
2.11.0

[U-Boot] [PATCH 1/1] tools: sunxi: avoid possible null pointer dereference

[Jagan Teki]

On Thu, May 4, 2017 at 2:41 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> If endptr is NULL we should not dereference it.
>
> The problem was indicated by cppcheck.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  tools/sunxi-spl-image-builder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/sunxi-spl-image-builder.c b/tools/sunxi-spl-image-builder.c
> index d538a38813..0072a64728 100644
> --- a/tools/sunxi-spl-image-builder.c
> +++ b/tools/sunxi-spl-image-builder.c
> @@ -433,7 +433,7 @@ int main(int argc, char **argv)
>                         break;
>                 case 'c':
>                         info.ecc_strength = strtol(optarg, &endptr, 0);
> -                       if (endptr || *endptr == '/')
> +                       if (endptr && *endptr == '/')

Did the config function as 'if' with single argument, can you check
below sample - couldn't reproduce null dereference.

# cppcheck --library=test.cfg tools/sunxi-spl-image-builder.c
Checking tools/sunxi-spl-image-builder.c...
[tools/sunxi-spl-image-builder.c:286]: (error) Resource leak: src
[tools/sunxi-spl-image-builder.c:286]: (error) Resource leak: dst
[tools/sunxi-spl-image-builder.c:286]: (error) Resource leak: rnd
[tools/sunxi-spl-image-builder.c:263]: (error) Memory leak: buffer
# cat test.cfg
<?xml version="1.0"?>
<def>
  <function name="if">
    <arg nr="1">
      <not-null/>
    </arg>
  </function>
</def>

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH v2 1/1] tools: sunxi: avoid read after end of string

[Heinrich Schuchardt]

The evaluation of option -c is incorrect:

According to the C99 standard endptr in the first strtol is always
set as &endptr is not NULL.
So the first part of the or condition is always true.
If all digits in optarg are valid endptr will point to the closing \0
and the second strtol will read beyond the end of the string optarg
points to.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
  Simplify the logical expression.
v1:
  In the original patch I missed that envptr is always set in strtol
  and used an unnecessary check if endptr is non-NULL.
  [PATCH 1/1] tools: sunxi: avoid possible null pointer dereference
  https://patchwork.ozlabs.org/patch/758224/
---
 tools/sunxi-spl-image-builder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/sunxi-spl-image-builder.c b/tools/sunxi-spl-image-builder.c
index d538a38813..a367f11774 100644
--- a/tools/sunxi-spl-image-builder.c
+++ b/tools/sunxi-spl-image-builder.c
@@ -433,7 +433,7 @@ int main(int argc, char **argv)
 			break;
 		case 'c':
 			info.ecc_strength = strtol(optarg, &endptr, 0);
-			if (endptr || *endptr == '/')
+			if (*endptr == '/')
 				info.ecc_step_size = strtol(endptr + 1, NULL, 0);
 			break;
 		case 'p':
-- 
2.11.0

[U-Boot] [PATCH v2 1/1] tools: sunxi: avoid read after end of string

[Boris Brezillon]

On Thu,  4 May 2017 22:26:42 +0200
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> The evaluation of option -c is incorrect:
> 
> According to the C99 standard endptr in the first strtol is always
> set as &endptr is not NULL.
> So the first part of the or condition is always true.
> If all digits in optarg are valid endptr will point to the closing \0
> and the second strtol will read beyond the end of the string optarg
> points to.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> v2:
>   Simplify the logical expression.
> v1:
>   In the original patch I missed that envptr is always set in strtol
>   and used an unnecessary check if endptr is non-NULL.
>   [PATCH 1/1] tools: sunxi: avoid possible null pointer dereference
>   https://patchwork.ozlabs.org/patch/758224/
> ---
>  tools/sunxi-spl-image-builder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/sunxi-spl-image-builder.c b/tools/sunxi-spl-image-builder.c
> index d538a38813..a367f11774 100644
> --- a/tools/sunxi-spl-image-builder.c
> +++ b/tools/sunxi-spl-image-builder.c
> @@ -433,7 +433,7 @@ int main(int argc, char **argv)
>  			break;
>  		case 'c':
>  			info.ecc_strength = strtol(optarg, &endptr, 0);
> -			if (endptr || *endptr == '/')
> +			if (*endptr == '/')
>  				info.ecc_step_size = strtol(endptr + 1, NULL, 0);
>  			break;
>  		case 'p':

[U-Boot] [U-Boot, v2, 1/1] tools: sunxi: avoid read after end of string

[Tom Rini]

On Thu, May 04, 2017 at 10:26:42PM +0200, xypron.glpk at gmx.de wrote:

> The evaluation of option -c is incorrect:
> 
> According to the C99 standard endptr in the first strtol is always
> set as &endptr is not NULL.
> So the first part of the or condition is always true.
> If all digits in optarg are valid endptr will point to the closing \0
> and the second strtol will read beyond the end of the string optarg
> points to.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170506/bad7aaff/attachment.sig>