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

Jakub Hrozek jhrozek at redhat.com
Tue Jul 8 07:50:13 UTC 2014


On Wed, Jun 25, 2014 at 02:09:24PM +0200, Lukas Slebodnik wrote:
> On (25/06/14 12:42), Pavel Reichl wrote:
> >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:
> As I already wrote. I don't mind. I was just waiting for common agreement.
> 
> >> -        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.
> Fixed.
> 
> >
> >> +            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.
> >
> No.
> It is not against coding style [1]
> http://www.freeipa.org/page/Coding_Style

You're right we shouldn't nack the patch on any formal grounds. For the
record I agree with:
    https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/

> 
> 
> >> +            ret = r;
> >> +            ret[num+1] = NULL;
> >> +            ret[num] = strdup(tmp);
> >> +            if (!ret[num]) goto fail;
> >Same as above.
> >
> No.
> I asked for a change in Coding Style few months ago [2]. Nothing happended.
> Simo wrote few times that this kind of test is acceptable for pointers after
> memory allocation.
> 
> >> +            num++;
> >> +            i = 0;
> >> +        }
> >>      }
> >
> >Thanks.
> >
> Thank you for review.
> 
> New version is attached.

The style issues were fixed. Unit tests pass, warnings are gone and the
parse_args functionality seems to still work fine (I tested some
different options and running valgrind for instance)

ACK



More information about the sssd-devel mailing list