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.