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

Lukas Slebodnik lslebodn at redhat.com
Mon Jun 23 07:13:43 UTC 2014


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.

LS
-------------- next part --------------
>From 24ac49c5770526c4c6360b5f899d17125273d89d 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 | 28 +++++++++++++++++++++++++---
 src/util/util.c        | 26 +++++++++++++-------------
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
index 198edf597b1bc8f1133e5c934e7f6fa2f6d4e021..5ef0102b59d4db9047b2019a7e2530481289208c 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", "	", "\\'", NULL };
+    const char *parsed11[] = { "a", NULL };
     struct pa_testcase tc[] = {
         { "foo", parsed1 },
         { "foo a", parsed2 },
@@ -150,13 +153,20 @@ START_TEST(test_parse_args)
         { "foo   a   e   ", parsed6 },
         { "foo	a	 	f 	", parsed7 },
         { "foo a\\\tg", parsed8 },
+        { "   foo  ", parsed9 },
+        { "\\   foo \\	 \\'  ", parsed10 },
+        { "a", parsed11 },
+        { " ", NULL },
+        { "", NULL },
+        { "  	  ", 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 len1st;
+            int len2nd;
+
+            for (len1st=0; parsed[len1st]; ++len1st);
+            for (len2nd=0; tc[i].parsed[len2nd]; ++len2nd);
+
+            fail_unless(len1st == len2nd,
+                        "Test %d: length of 1st array [%d] != length of 2nd "
+                        "array[%d]\n", i, len1st, len2nd);
+
+            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..151f9d2cf683af7daada86cd04c69f2bfdfbcf73 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140623/db5a0273/attachment.html>


More information about the sssd-devel mailing list