[SSSD] [PATCH] UTIL: Fix access out of bound in parse_args

Pavel Reichl preichl at redhat.com
Wed Jun 25 10:42:01 UTC 2014


On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> While parsing string with multiple whitespaces, it may happen variable i is
> zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of
> this bug is duplicite string output array.
>   Input string:    "foo  b"
>   Expected output: { "foo", "a", NULL }
>   Output:          { "foo", "foo", "a", NULL }
> 
> This patch uses inverted logic. Instead of testing whether to read next char or
> skip multiple whitespaces, we will test whether we have new argument which
> should be stored in output array.
> 
> How to test?
> libtool --mode=execute valgrind ./util-tests
> Running suite(s): util
> ==17839== Invalid read of size 1
> ==17839==    at 0x4E60329: parse_args (util.c:208)
> ==17839==    by 0x403A04: test_parse_args (util-tests.c:166)
> ==17839==    by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327)
> ==17839==    by 0x4C2F3C5: srunner_run (check_run.c:187)
> ==17839==    by 0x4057ED: main (util-tests.c:1090)
> ==17839==  Address 0x62f709f is 1 bytes before a block of size 7 alloc'd
> ==17839==    at 0x4A0645D: malloc (vg_replace_malloc.c:291)
> ==17839==    by 0x4E6020E: parse_args (util.c:149)
> ==17839==    by 0x403A04: test_parse_args (util-tests.c:166)
> ==17839==    by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327)
> ==17839==    by 0x4C2F3C5: srunner_run (check_run.c:187)
> ==17839==    by 0x4057ED: main (util-tests.c:1090)
> ==17839==
> 
> This problem was also reported by clang static analysers.
> We thought it was false positive. Unfortunately, it wasn't.

ACK to the patch as is although I would appreciate if you considered my
and Nick's comments about '\t' and following nitpicks:

> -        if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
> -            /* check next char and skip multiple spaces */
> -            continue;
> -        }
>  
> -        r = realloc(ret, (num + 2) * sizeof(char *));
> -        if (!r) goto fail;
> -        ret = r;
> -        ret[num+1] = NULL;
> -        ret[num] = strdup(tmp);
> -        if (!ret[num]) goto fail;
> -        num++;
> -        i = 0;
> +        /* save token to result array */
> +        if (i>1 && tmp[i-1] == '\0') {
I think it's way more usual in SSSD code base to put space before and after '>' operator.
            
> +            r = realloc(ret, (num + 2) * sizeof(char *));
> +            if (!r) goto fail;
Would you consider replacing !r with r == NULL. 
I agree it is not in our code style guide, but I think most of new code use this form.

> +            ret = r;
> +            ret[num+1] = NULL;
> +            ret[num] = strdup(tmp);
> +            if (!ret[num]) goto fail;
Same as above.

> +            num++;
> +            i = 0;
> +        }
>      }

Thanks.




More information about the sssd-devel mailing list