From d1ca1f7c2ae052e59d0cbe8512e852b9ef059451 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jul 2020 11:24:36 +0200 Subject: [PATCH] xdg-autostart: avoid quadratic behaviour in strv parsing MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 --- .../xdg-autostart-service.c | 68 ++++++++++++------ test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812 | Bin 0 -> 128273 bytes 2 files changed, 48 insertions(+), 20 deletions(-) create mode 100644 test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812 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 0000000000000000000000000000000000000000..4b4cadffb70a3a0f38acbe64b6b813af4ae3980f GIT binary patch literal 128273 zc-rmQZ)@8|9Kdn6unn9Cc`*8DFfM={3l9cU=4U&bEn$zH?W#@@p2B>#~UyOx(UTf6UvkeqzKyZhapBBOh95XPs;aAdRxNi=y{ z{&6x2z5AolS^xMb=?@3x&QU2=#gmB{-Tu|+pS>8C+poi8&oU{S)j_eK9Ms;vD zdHiy?ySsaQyx;Ww){SPf+BfR0NR4FqC81`vv(x;%*=&aK<8k8qweH}y>$=a}9N(`u zwVj9KC#^WQF|udMaqa`T*fPg<(yH{rn#3!%bKMS4n=N|CRxUqMWTpT1lUS!XYqQ8= zeZTV>TjYxG9Af@Sp`lkSz`DK*4O(ouE;Fs|MvxbqUuqFHFXYwk>)QMm!yH^$`ZsP~ z^L>!z{MT>4UuMarKC&;SW&v6Ii-!|W%eLx#dQkBy^bR}y zN7M6(2Ltagdq1sA1?lfEDs#TOC|CY``^Q`Vt9zAlH+wDrWzj+Qf*XJT^-&VO7)BE- zJ`2?wsd`uMj0{ywuPT=Dt*K%vd0B(L_4C=eR@J`uWmV@p>_@o@{cb(IP<7pG?4e)>~sTCD1C>7e7>j4#er z&+@Kr_+W9MvehBW^w6Zxf-em}UFM?U+h)7vF;9MAq1ScAec*4t59S-ayoD=A*|28- z00000000000000000000000000000000000000000000000000000000000000000 z00000000000000000000000000000000000000000000000000000000000000000 z00000000000000000000000000000000000000000000000000000000000000000 z00000000000000000000006LHKd@ZikMysgW!Hm1I(5fhd}evpI9A0#%Kf?%IhHG2 zDP0wp2KMHGK`<+x=oUgqA*Dz^5^^bA7aC%z$zU;_9&O$-4GM%1IYjR zzAr?hA@c2{m7mhO7WVsF^vmUS$F6&6*mU&32`t^qe2=Dk6Wu%ZX4g?8OVm+`egHde z3){Xp^SYk7T7Z__@T}9wQqzkL!;R@S(kAE9vtm_0&ac(g?dgg<#~1SKnGX-z=H_hQ t