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

Jakub Hrozek jhrozek at redhat.com
Wed Jun 25 11:04:32 UTC 2014


On Wed, Jun 25, 2014 at 12:42:01PM +0200, 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:

I agree with the tab comment, too. Code readability should not depend on
the editor settings.

> 
> > -        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.

+1

>             
> > +            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.

+1

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

+1 

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



More information about the sssd-devel mailing list