[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