From 3f2e15abc52c4749fa46346a5a314b839bfe9e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jul 2020 10:02:02 +0200 Subject: [PATCH 1/4] sleep: one spelling unification We use "writable" everywhere else. --- src/sleep/sleep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index f494ca6ffb..7029352ca5 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -71,7 +71,7 @@ static int write_hibernate_location_info(const HibernateLocation *hibernate_loca return 0; } - return log_debug_errno(errno, "/sys/power/resume_offset not writeable: %m"); + return log_debug_errno(errno, "/sys/power/resume_offset not writable: %m"); } xsprintf(offset_str, "%" PRIu64, hibernate_location->offset); From 9ecf5d9340aeddc2fae51134ac1ff15100da974d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jul 2020 10:26:28 +0200 Subject: [PATCH 2/4] fuzz: add test case that should already be resolved --- test/fuzz/fuzz-network-parser/oss-fuzz-23950 | Bin 0 -> 21603 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 test/fuzz/fuzz-network-parser/oss-fuzz-23950 diff --git a/test/fuzz/fuzz-network-parser/oss-fuzz-23950 b/test/fuzz/fuzz-network-parser/oss-fuzz-23950 new file mode 100644 index 0000000000000000000000000000000000000000..5bfb17ba38cfedcf03cd991fd1805a4a6a7e7df6 GIT binary patch literal 21603 zcmeHP%}(1u5boJ8(Cf5K4^(Vq^OH&l3le_H0Z>q&Q3V{VWKC8fHd-4{;L=Cwi}b1b z0v+2iv6DD;f|JH!kRr!;V~=-sXTJG%W-m{<`)JGdbw!Hi4Zl|QY{%7=3x`*#`xm-$ zWC*ET9#}2IYVhW{G%Tkh97otzjoGz6j*?wSVN2n5PmDWVc|PLC zL-$CyE|#Wly#x3-jWvnlz&*Av4sZumLx+}YBQ-LQ#L zk+#un7+!Ct>AyVmUXqT(uAf71kJiE9e?vW2BYM6uT=Brq<-OyUtwzh1jxN?gJ3NG) z0zkd+{RabPs?L?BZbF&Cv9Lg6rYHfn)WESoWgt4(bQM-UFsOe9##zf>z(5yu@3#O){O`wVAq@jk-sP_k2 zRL1B)`}6DPZ5-!fk&I2OIN8+vcI{i&={FVuG02*}TE|He?k222;+$-egLi z9fe%XjxWLVpU3Qo8HcD@qp|i6i%(bp}DU2x3Cg$~uX&M0J0&|%wr z5+J;EE;8a0qJm7}*E5*d^T8cG*Vl(V=@5f&JJ3BvP|z?43bmF5gLWC>8bsKO7YPuS zonp)nW7#*2rr#-3g$vzr;X(#xB?~mIhG;soIC-X+u@9)1A#(E~(lpRbo#S6Z;OvQ6 zID;ZaA1d@h=uM29Edj@P015HHHHf}K(}Tfb;PqqvBk#z+7x*v8OdcN>M?p~j(U@;o zqa@A9@fD$I2%n5$Hs6}Xo`BU38P7heO=NTWHGx#UM*uzal0-K5+4NjERU(_+GLg+m z3jniNYa+5ao5kFjLLMTUiEN&75jg|5M`ZJGe#^YlP6*3pf@O(po;){&$mWq*Tdzq_ zAhLPZyfcx_lV+J^01&cRHX^clOimz?%|tdYSn66H(7||{LR?#JxHiH~gqsHPG1H}@ z&4Mw-1Pd=ae;IL8P~)9~Rj!eWeWEu+pesx*G%syGt1zh#VGFEk+eI`9t!D z37bHS0jT5;akB;Thc6pL#CLkgM=d*%WnnqoEU3s{y$}^w#`a=a;^Imo%l_lGIQeiz zM3(tGDkYbKF4`7J6dX`+K*2$b3{AV%n-7ajWLd&KcSM%q{*ueQ`%q}sl;<{gk3~ng Mx{7;#Hd3Db1ptc{IsgCw literal 0 HcmV?d00001 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 3/4] 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 From dea7f5cc8751ab36c75acb7e3d181edef5e73876 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 Jul 2020 11:31:17 +0200 Subject: [PATCH 4/4] xdg-autostart: ignore all empty entries in multi-string entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The desktop file specification allows entries like ";;;;;;", full of empty strings. But looking at the actual list of supported keys [1], empty entries are meaningless (unless we would allow e.g. the desktop name to be the empty string. But that doesn't seem very useful either). So let's just simplify our life and skip any empty substrings entirely. This would also resolve the fuzzer case: $ valgrind build/fuzz-xdg-desktop test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812 test/fuzz/fuzz-xdg-desktop/oss-fuzz-22812... ok ==2899241== HEAP SUMMARY: ==2899241== in use at exit: 0 bytes in 0 blocks ==2899241== total heap usage: 484,385 allocs, 484,385 frees, 12,411,330 bytes allocated ↓ ==2899650== HEAP SUMMARY: ==2899650== in use at exit: 0 bytes in 0 blocks ==2899650== total heap usage: 1,325 allocs, 1,325 frees, 1,463,602 bytes allocated --- src/test/test-xdg-autostart.c | 6 ++---- .../xdg-autostart-service.c | 17 +++++++++-------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/test/test-xdg-autostart.c b/src/test/test-xdg-autostart.c index cc75bc6024..70287b3c55 100644 --- a/src/test/test-xdg-autostart.c +++ b/src/test/test-xdg-autostart.c @@ -67,7 +67,7 @@ static void test_xdg_desktop_parse(unsigned i, const char *s) { case 0: assert_se(streq(service->exec_string, "/bin/sleep 100")); assert_se(strv_equal(service->only_show_in, STRV_MAKE("A", "B"))); - assert_se(strv_equal(service->not_show_in, STRV_MAKE("C", "", "D\\;", "E"))); + assert_se(strv_equal(service->not_show_in, STRV_MAKE("C", "D\\;", "E"))); assert_se(!service->hidden); break; case 1: @@ -81,14 +81,12 @@ static void test_xdg_desktop_parse(unsigned i, const char *s) { } int main(int argc, char *argv[]) { - size_t i; - test_setup_logging(LOG_DEBUG); test_translate_name(); test_xdg_format_exec_start(); - for (i = 0; i < ELEMENTSOF(xdg_desktop_file); i++) + for (size_t i = 0; i < ELEMENTSOF(xdg_desktop_file); i++) test_xdg_desktop_parse(i, xdg_desktop_file[i]); return 0; diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index a19d2d7e24..4a30f9e433 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -192,6 +192,9 @@ static int strv_strndup_unescape_and_push( const char *start, const char *end) { + if (end == start) + return 0; + _cleanup_free_ char *copy = NULL; int r; @@ -270,14 +273,12 @@ static int xdg_config_parse_strv( } } - /* Any trailing entry should be ignored if it is empty. */ - if (end > start) { - r = strv_strndup_unescape_and_push(unit, filename, line, - &sv, &n_allocated, &n, - start, end); - if (r < 0) - return r; - } + /* Handle the trailing entry after the last separator */ + r = strv_strndup_unescape_and_push(unit, filename, line, + &sv, &n_allocated, &n, + start, end); + if (r < 0) + return r; *ret_sv = TAKE_PTR(sv); return 0;