From d1ca1f7c2ae052e59d0cbe8512e852b9ef059451 Mon Sep 17 00:00:00 2001 From: =?UTF-8?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=UTF-8 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 0d33c70401..a19d2d7e24 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 zcmeI$U2EG`7{Kwgunm%nT#Q}~hUVe99Wy$T?L?^%rK};8f>YLA1gaLI)M>q>wkOG~ zF6`6vW9(u#`waUU`wBbfNIH^VW4B$|*1zeCY@HXK-}C5LejeZJB^T*%WE~FDc=BZX z$H}M{+#QWB`)8+Ve>m9go^B=b@O;vYZ~kiaFP{&$4`21p0^6GOeuyorQNFI3!*9${ zb`ZvmMz!V4>GG{<*i0_^qacmPy=^aw99M*46v;o)xjYtbTkLzb_)fkTj=Qz>$;hg0 zpEQ$6lJ;Kix12q1U$vDdotcx*Dgz%(Qrnl~_D0qfExn1v`KoB#J-$ern#`)-s7yRH zb`raT)u*lc;BxZl#c+3b_w4MT6NY=&I-PpoYVE~xCOg~`a%DR^ozFX+PA_>hPQ$Qq zJhK-2-Hp9@-t+Iyd@U|Xvn53t1+I;xp zG-*0&-cU&2%FA&)=Y0IEcdRaUEA3m|)0cLlGG=T0@si{T)2D~8!t|o%SL0IUDPmeg zY+1X_L$vnP%hT!8GG}DkNfe=SNy>~SIwBgZ*td9f$^ z_>SYtcDmt-X&6Pt;koJ|gd=lwewJ!K$5ta3fpJcnj-D$2z%gfp=X&~-j-EZK_9_Be z8L0S3&TqPvBV;#Ib*9+XxBVnh5JjJ2a>p-@%&>E9WMY{{D#un#VJg64#~IUruw+O#KcisK>Z= zU3}O)kg=54{MT>4$M(vcUP*33o25Ews-7^eOjDIB*SE5l`X-s%u9EobbTZb91_;dA zayK=*kqO#*x3fwVzu0Ok8`Rw__sy^?6Q#KEv#iUv`^sSX4jb>TwOMCZQ`wa(hYON< zjT_kvFn<#+7*MRYXd%j-V&g9wYd(Z)tm7lyklZ4ECbu`=kct#7hRUB%*qF0zVwUG^ z9*fr7a7+N+&+|idcNONbvslhMZ%F&7 zaBrUn#qZKp|7R1<7bl){qIg@Yg?@Ql z`}55oZ^Ezc)VB36-50}O$^o+KNb>#Hr)lr`FrL`SWlt6jZOS&4 zA#1g)L*M@Sa;8_kAADI?*E{UTmEqKH>wIi^PSzPznPF&Iy?Ss^uiZH6ewLq>KbWWH zTkF=TJ|Hck1S%MdV=gNNty^v@kOhN1w;k9Qu`Qoy$49+^+?91UN4jEg(tL35uqn5! ztOlT}T(WGHRT`>=hS$|jatPHdzN3cB9!zD)K^DaFehW??WEBZjcbO_ms4|8urinPe zKsB0f600+gYOg97E2Fuqb9z zr9|E!fB*srAb+VV_Pb{Q0XBR!kxsT)N?2z6n%RmTr-xf?D{ zE+Yy@Cb=`7XfzFCN!&`I;Cd+asL cYroM5<3__WAFMc#4b%VYt6cUb99Oyh4N1lhp8x;= literal 0 HcmV?d00001