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

Lukas Slebodnik lslebodn at redhat.com
Wed Jun 25 12:09:24 UTC 2014


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
-------------- next part --------------
>From afc52674a8fd5002e6ce2c9e8177142c01128cbc Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 20 Jun 2014 17:04:59 +0200
Subject: [PATCH] UTIL: Fix access out of bound in parse_args

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.
---
 src/tests/util-tests.c | 30 ++++++++++++++++++++++++++----
 src/util/util.c        | 26 +++++++++++++-------------
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index 198edf597b1bc8f1133e5c934e7f6fa2f6d4e021..b79153e3e89b90182bfce7c504a7d6a764a0e59b 100644
--- a/src/tests/util-tests.c
+++ b/src/tests/util-tests.c
@@ -141,6 +141,9 @@ START_TEST(test_parse_args)
     const char *parsed6[] = { "foo", "a", "e", NULL };
     const char *parsed7[] = { "foo", "a", "f", NULL };
     const char *parsed8[] = { "foo", "a\tg", NULL };
+    const char *parsed9[] = { "foo", NULL };
+    const char *parsed10[] = { " ", "foo", "\t", "\\'", NULL };
+    const char *parsed11[] = { "a", NULL };
     struct pa_testcase tc[] = {
         { "foo", parsed1 },
         { "foo a", parsed2 },
@@ -148,15 +151,22 @@ START_TEST(test_parse_args)
         { "foo a\\ c", parsed4 },
         { "foo a d ", parsed5 },
         { "foo   a   e   ", parsed6 },
-        { "foo	a	 	f 	", parsed7 },
+        { "foo\ta\t \tf \t", parsed7 },
         { "foo a\\\tg", parsed8 },
+        { "   foo  ", parsed9 },
+        { "\\   foo \\\t \\'  ", parsed10 },
+        { "a", parsed11 },
+        { " ", NULL },
+        { "", NULL },
+        { "  \t  ", NULL },
         { NULL, NULL }
     };
 
     for (i=0; tc[i].argstr != NULL; i++) {
         parsed = parse_args(tc[i].argstr);
         fail_if(parsed == NULL && tc[i].parsed != NULL,
-                "Could not parse correct argument string '%s'\n");
+                "Could not parse correct %d argument string '%s'\n",
+                i, tc[i].argstr);
 
         ret = diff_string_lists(test_ctx, parsed, discard_const(tc[i].parsed),
                                 &only_ret, &only_exp, &both);
@@ -164,8 +174,20 @@ START_TEST(test_parse_args)
         fail_unless(only_ret[0] == NULL, "The parser returned more data than expected\n");
         fail_unless(only_exp[0] == NULL, "The parser returned less data than expected\n");
 
-        for (ii = 0; parsed[ii]; ii++) free(parsed[ii]);
-        free(parsed);
+        if (parsed) {
+            int parsed_len;
+            int expected_len;
+
+            for (parsed_len=0; parsed[parsed_len]; ++parsed_len);
+            for (expected_len=0; tc[i].parsed[expected_len]; ++expected_len);
+
+            fail_unless(parsed_len == expected_len,
+                        "Test %d: length of 1st array [%d] != length of 2nd "
+                        "array[%d]\n", i, parsed_len, expected_len);
+
+            for (ii = 0; parsed[ii]; ii++) free(parsed[ii]);
+            free(parsed);
+        }
     }
 
     talloc_free(test_ctx);
diff --git a/src/util/util.c b/src/util/util.c
index ad93ca1a0715e3a5f5c6c7371eb142718c38afa3..7f80771ecd9868feaf43e34cbd61e44dd8ae5f3a 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -153,7 +153,8 @@ char **parse_args(const char *str)
     num = 0;
     i = 0;
     e = false;
-    w = false;
+    /* skip leading whitespaces */
+    w = true;
     p = str;
     while (*p) {
         if (*p == '\\') {
@@ -205,19 +206,18 @@ char **parse_args(const char *str)
             tmp[i] = '\0';
             i++;
         }
-        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') {
+            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;
+        }
     }
 
     free(tmp);
-- 
1.9.3



More information about the sssd-devel mailing list