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
> + 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.
LS
[1]
http://www.freeipa.org/page/Coding_Style
[2]
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-March/018814.html