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