From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Tue, 7 Jul 2020 09:24:36 +0000 (+0200)
Subject: xdg-autostart: avoid quadratic behaviour in strv parsing
X-Git-Tag: v246-rc1~16^2~1
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d1ca1f7c2ae052e59d0cbe8512e852b9ef059451;p=thirdparty%2Fsystemd.git

xdg-autostart: avoid quadratic behaviour in strv parsing

The fuzzer test case has a giant line with ";;;;;;;;;;;..." which is turned into
a strv of empty strings. Unfortunately, when pushing each string, strv_push() needs
to walk the whole array, which leads to quadratic behaviour. So let's use
greedy_allocation here and also keep location in the string to avoid iterating.

build/fuzz-xdg-desktop test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812  51.10s user 0.01s system 99% cpu 51.295 total
↓
build/fuzz-xdg-desktop test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812  0.07s user 0.01s system 96% cpu 0.083 total

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22812.

Other minor changes:
- say "was already defined" instead of "defined multiple times" to make it
  clear that we're ignoring this second definition, and not all definitions
  of the key
- unescaping needs to be done also for the last entry
---

diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c
index 0d33c704012..a19d2d7e249 100644
--- a/src/xdg-autostart-generator/xdg-autostart-service.c
+++ b/src/xdg-autostart-generator/xdg-autostart-service.c
@@ -182,6 +182,37 @@ static int xdg_config_parse_string(
         return 0;
 }
 
+static int strv_strndup_unescape_and_push(
+                const char *unit,
+                const char *filename,
+                unsigned line,
+                char ***sv,
+                size_t *n_allocated,
+                size_t *n,
+                const char *start,
+                const char *end) {
+
+        _cleanup_free_ char *copy = NULL;
+        int r;
+
+        copy = strndup(start, end - start);
+        if (!copy)
+                return log_oom();
+
+        r = xdg_unescape_string(unit, filename, line, copy);
+        if (r < 0)
+                return r;
+
+        if (!greedy_realloc((void**) sv, n_allocated, *n + 2, sizeof(char*))) /* One extra for NULL */
+                return log_oom();
+
+        (*sv)[*n] = TAKE_PTR(copy);
+        (*sv)[*n + 1] = NULL;
+        (*n)++;
+
+        return 0;
+}
+
 static int xdg_config_parse_strv(
                 const char *unit,
                 const char *filename,
@@ -194,9 +225,7 @@ static int xdg_config_parse_strv(
                 void *data,
                 void *userdata) {
 
-        char ***sv = data;
-        const char *start;
-        const char *end;
+        char ***ret_sv = data;
         int r;
 
         assert(filename);
@@ -205,23 +234,25 @@ static int xdg_config_parse_strv(
         assert(data);
 
         /* XDG does not allow duplicate definitions. */
-        if (*sv) {
-                log_syntax(unit, LOG_ERR, filename, line, 0, "Key %s was defined multiple times, ignoring.", lvalue);
+        if (*ret_sv) {
+                log_syntax(unit, LOG_ERR, filename, line, 0, "Key %s was already defined, ignoring.", lvalue);
                 return 0;
         }
 
-        *sv = strv_new(NULL);
-        if (!*sv)
+        size_t n = 0, n_allocated = 0;
+        _cleanup_strv_free_ char **sv = NULL;
+
+        if (!GREEDY_REALLOC0(sv, n_allocated, 1))
                 return log_oom();
 
         /* We cannot use strv_split because it does not handle escaping correctly. */
-        start = rvalue;
+        const char *start = rvalue, *end;
 
         for (end = start; *end; end++) {
                 if (*end == '\\') {
                         /* Move forward, and ensure it is a valid escape. */
                         end++;
-                        if (strchr("sntr\\;", *end) == NULL) {
+                        if (!strchr("sntr\\;", *end)) {
                                 log_syntax(unit, LOG_ERR, filename, line, 0, "Undefined escape sequence \\%c.", *end);
                                 return 0;
                         }
@@ -229,17 +260,11 @@ static int xdg_config_parse_strv(
                 }
 
                 if (*end == ';') {
-                        _cleanup_free_ char *copy = NULL;
-
-                        copy = strndup(start, end - start);
-                        if (!copy)
-                                return log_oom();
-                        r = xdg_unescape_string(unit, filename, line, copy);
+                        r = strv_strndup_unescape_and_push(unit, filename, line,
+                                                           &sv, &n_allocated, &n,
+                                                           start, end);
                         if (r < 0)
                                 return r;
-                        r = strv_consume(sv, TAKE_PTR(copy));
-                        if (r < 0)
-                                return log_oom();
 
                         start = end + 1;
                 }
@@ -247,11 +272,14 @@ static int xdg_config_parse_strv(
 
         /* Any trailing entry should be ignored if it is empty. */
         if (end > start) {
-                r = strv_extend(sv, start);
+                r = strv_strndup_unescape_and_push(unit, filename, line,
+                                                   &sv, &n_allocated, &n,
+                                                   start, end);
                 if (r < 0)
-                        return log_oom();
+                        return r;
         }
 
+        *ret_sv = TAKE_PTR(sv);
         return 0;
 }
 
diff --git a/test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812 b/test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812
new file mode 100644
index 00000000000..4b4cadffb70
Binary files /dev/null and b/test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812 differ