From fd5b9b84739cc50ed47681a1245d727ccaadcc71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 14:33:42 +0100 Subject: [PATCH 01/13] bus/bus-match: inline iterator variable declarations --- src/libsystemd/sd-bus/bus-internal.h | 4 +- src/libsystemd/sd-bus/bus-match.c | 60 +++++++++++----------------- 2 files changed, 25 insertions(+), 39 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-internal.h b/src/libsystemd/sd-bus/bus-internal.h index a331ee3f3e..4a45507da5 100644 --- a/src/libsystemd/sd-bus/bus-internal.h +++ b/src/libsystemd/sd-bus/bus-internal.h @@ -44,8 +44,8 @@ struct match_callback { unsigned last_iteration; /* Don't dispatch this slot with messages that arrived in any iteration before or at the this - * one. We use this to ensure that matches don't apply "retroactively" and thus can confuse the - * caller: matches will only match incoming messages from the moment on the match was installed. */ + * one. We use this to ensure that matches don't apply "retroactively" and confuse the caller: + * only messages received after the match was installed will be considered. */ uint64_t after; char *match_string; diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index fd74867afa..3725d279d5 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -408,12 +408,9 @@ int bus_match_run( if (r != 0) return r; } - } else { - struct bus_match_node *c; - + } else /* No hash table, so let's iterate manually... */ - - for (c = node->child; c; c = c->next) { + for (struct bus_match_node *c = node->child; c; c = c->next) { if (!value_node_test(c, node->type, test_u8, test_str, test_strv, m)) continue; @@ -424,7 +421,6 @@ int bus_match_run( if (bus && bus->match_callbacks_modified) return 0; } - } if (bus && bus->match_callbacks_modified) return 0; @@ -440,7 +436,7 @@ static int bus_match_add_compare_value( const char *value_str, struct bus_match_node **ret) { - struct bus_match_node *c = NULL, *n = NULL; + struct bus_match_node *c, *n = NULL; int r; assert(where); @@ -452,25 +448,22 @@ static int bus_match_add_compare_value( ; if (c) { - /* Comparison node already exists? Then let's see if - * the value node exists too. */ + /* Comparison node already exists? Then let's see if the value node exists too. */ if (t == BUS_MATCH_MESSAGE_TYPE) n = hashmap_get(c->compare.children, UINT_TO_PTR(value_u8)); else if (BUS_MATCH_CAN_HASH(t)) n = hashmap_get(c->compare.children, value_str); - else { + else for (n = c->child; n && !value_node_same(n, t, value_u8, value_str); n = n->next) ; - } if (n) { *ret = n; return 0; } } else { - /* Comparison node, doesn't exist yet? Then let's - * create it. */ + /* Comparison node, doesn't exist yet? Then let's create it. */ c = new0(struct bus_match_node, 1); if (!c) { @@ -706,9 +699,7 @@ static int match_component_compare(const struct bus_match_component *a, const st } void bus_match_parse_free(struct bus_match_component *components, unsigned n_components) { - unsigned i; - - for (i = 0; i < n_components; i++) + for (unsigned i = 0; i < n_components; i++) free(components[i].value_str); free(components); @@ -722,7 +713,7 @@ int bus_match_parse( const char *p = match; struct bus_match_component *components = NULL; size_t components_allocated = 0; - unsigned n_components = 0, i; + unsigned n_components = 0; _cleanup_free_ char *value = NULL; int r; @@ -838,7 +829,7 @@ int bus_match_parse( typesafe_qsort(components, n_components, match_component_compare); /* Check for duplicates */ - for (i = 0; i+1 < n_components; i++) + for (unsigned i = 0; i+1 < n_components; i++) if (components[i].type == components[i+1].type) { r = -EINVAL; goto fail; @@ -855,10 +846,8 @@ fail: } char *bus_match_to_string(struct bus_match_component *components, unsigned n_components) { - _cleanup_fclose_ FILE *f = NULL; char *buffer = NULL; size_t size = 0; - unsigned i; int r; if (n_components <= 0) @@ -866,11 +855,11 @@ char *bus_match_to_string(struct bus_match_component *components, unsigned n_com assert(components); - f = open_memstream_unlocked(&buffer, &size); + _cleanup_fclose_ FILE *f = open_memstream_unlocked(&buffer, &size); if (!f) return NULL; - for (i = 0; i < n_components; i++) { + for (unsigned i = 0; i < n_components; i++) { char buf[32]; if (i != 0) @@ -901,23 +890,22 @@ int bus_match_add( unsigned n_components, struct match_callback *callback) { - unsigned i; - struct bus_match_node *n; int r; assert(root); assert(callback); - n = root; - for (i = 0; i < n_components; i++) { - r = bus_match_add_compare_value( - n, components[i].type, - components[i].value_u8, components[i].value_str, &n); + for (unsigned i = 0; i < n_components; i++) { + r = bus_match_add_compare_value(root, + components[i].type, + components[i].value_u8, + components[i].value_str, + &root); if (r < 0) return r; } - return bus_match_add_leaf(n, callback); + return bus_match_add_leaf(root, callback); } int bus_match_remove( @@ -1029,7 +1017,6 @@ const char* bus_match_node_type_to_string(enum bus_match_node_type t, char buf[] } void bus_match_dump(struct bus_match_node *node, unsigned level) { - struct bus_match_node *c; _cleanup_free_ char *pfx = NULL; char buf[32]; @@ -1047,23 +1034,23 @@ void bus_match_dump(struct bus_match_node *node, unsigned level) { } else if (node->type == BUS_MATCH_ROOT) puts(" root"); else if (node->type == BUS_MATCH_LEAF) - printf(" %p/%p\n", node->leaf.callback->callback, container_of(node->leaf.callback, sd_bus_slot, match_callback)->userdata); + printf(" %p/%p\n", node->leaf.callback->callback, + container_of(node->leaf.callback, sd_bus_slot, match_callback)->userdata); else putchar('\n'); if (BUS_MATCH_CAN_HASH(node->type)) { - + struct bus_match_node *c; HASHMAP_FOREACH(c, node->compare.children) bus_match_dump(c, level + 1); } - for (c = node->child; c; c = c->next) + for (struct bus_match_node *c = node->child; c; c = c->next) bus_match_dump(c, level + 1); } enum bus_match_scope bus_match_get_scope(const struct bus_match_component *components, unsigned n_components) { bool found_driver = false; - unsigned i; if (n_components <= 0) return BUS_MATCH_GENERIC; @@ -1076,7 +1063,7 @@ enum bus_match_scope bus_match_get_scope(const struct bus_match_component *compo * local messages, then we check if it only matches on the * driver. */ - for (i = 0; i < n_components; i++) { + for (unsigned i = 0; i < n_components; i++) { const struct bus_match_component *c = components + i; if (c->type == BUS_MATCH_SENDER) { @@ -1095,5 +1082,4 @@ enum bus_match_scope bus_match_get_scope(const struct bus_match_component *compo } return found_driver ? BUS_MATCH_DRIVER : BUS_MATCH_GENERIC; - } From 4ff42f8327cdd67b311bcc6e4c38dc15d52feea7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 14:35:46 +0100 Subject: [PATCH 02/13] bus/bus-match: use "ret_" prefix for output parameters --- src/libsystemd/sd-bus/bus-match.c | 12 ++++++------ src/libsystemd/sd-bus/bus-match.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index 3725d279d5..db8da6ee12 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -707,8 +707,8 @@ void bus_match_parse_free(struct bus_match_component *components, unsigned n_com int bus_match_parse( const char *match, - struct bus_match_component **_components, - unsigned *_n_components) { + struct bus_match_component **ret_components, + unsigned *ret_n_components) { const char *p = match; struct bus_match_component *components = NULL; @@ -718,8 +718,8 @@ int bus_match_parse( int r; assert(match); - assert(_components); - assert(_n_components); + assert(ret_components); + assert(ret_n_components); while (*p != 0) { const char *eq, *q; @@ -835,8 +835,8 @@ int bus_match_parse( goto fail; } - *_components = components; - *_n_components = n_components; + *ret_components = components; + *ret_n_components = n_components; return 0; diff --git a/src/libsystemd/sd-bus/bus-match.h b/src/libsystemd/sd-bus/bus-match.h index 577100081d..d079f6aec7 100644 --- a/src/libsystemd/sd-bus/bus-match.h +++ b/src/libsystemd/sd-bus/bus-match.h @@ -73,7 +73,7 @@ void bus_match_dump(struct bus_match_node *node, unsigned level); const char* bus_match_node_type_to_string(enum bus_match_node_type t, char buf[], size_t l); enum bus_match_node_type bus_match_node_type_from_string(const char *k, size_t n); -int bus_match_parse(const char *match, struct bus_match_component **_components, unsigned *_n_components); +int bus_match_parse(const char *match, struct bus_match_component **ret_components, unsigned *ret_n_components); void bus_match_parse_free(struct bus_match_component *components, unsigned n_components); char *bus_match_to_string(struct bus_match_component *components, unsigned n_components); From 49ba1522fc585d9b9a8867de41a215dabd3b2c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 14:40:55 +0100 Subject: [PATCH 03/13] fuzz-bus-message: move sources to src/libsystemd/ There's also fuzz-bus-label, but despite the name, it tests code that is in src/shared/, so it shouldn't move. --- src/fuzz/meson.build | 2 -- src/libsystemd/meson.build | 6 ++++++ src/{fuzz => libsystemd/sd-bus}/fuzz-bus-message.c | 0 3 files changed, 6 insertions(+), 2 deletions(-) rename src/{fuzz => libsystemd/sd-bus}/fuzz-bus-message.c (100%) diff --git a/src/fuzz/meson.build b/src/fuzz/meson.build index 96528a191d..1ed1dd8251 100644 --- a/src/fuzz/meson.build +++ b/src/fuzz/meson.build @@ -1,8 +1,6 @@ # SPDX-License-Identifier: LGPL-2.1-or-later fuzzers += [ - [['src/fuzz/fuzz-bus-message.c']], - [['src/fuzz/fuzz-catalog.c']], [['src/fuzz/fuzz-json.c']], diff --git a/src/libsystemd/meson.build b/src/libsystemd/meson.build index b8a9bbe222..2fe1978d02 100644 --- a/src/libsystemd/meson.build +++ b/src/libsystemd/meson.build @@ -315,3 +315,9 @@ if cxx_cmd != '' [['src/libsystemd/sd-bus/test-bus-vtable-cc.cc']], ] endif + +############################################################ + +fuzzers += [ + [['src/libsystemd/sd-bus/fuzz-bus-message.c']], +] diff --git a/src/fuzz/fuzz-bus-message.c b/src/libsystemd/sd-bus/fuzz-bus-message.c similarity index 100% rename from src/fuzz/fuzz-bus-message.c rename to src/libsystemd/sd-bus/fuzz-bus-message.c From 5963e6f43c4f33d5255ef0fb887cdf382bd51c9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 21:19:02 +0100 Subject: [PATCH 04/13] sd-bus: fix memstream buffer extraction I'm getting the following error under valgrind: ==305970== Invalid free() / delete / delete[] / realloc() ==305970== at 0x483E9F1: free (vg_replace_malloc.c:538) ==305970== by 0x4012CD: mfree (alloc-util.h:48) ==305970== by 0x4012EF: freep (alloc-util.h:83) ==305970== by 0x4017F4: LLVMFuzzerTestOneInput (fuzz-bus-match.c:58) ==305970== by 0x401A58: main (fuzz-main.c:39) ==305970== Address 0x59972f0 is 0 bytes inside a block of size 8,192 free'd ==305970== at 0x483FCE4: realloc (vg_replace_malloc.c:834) ==305970== by 0x4C986F7: _IO_mem_finish (in /usr/lib64/libc-2.33.so) ==305970== by 0x4C8F5E0: fclose@@GLIBC_2.2.5 (in /usr/lib64/libc-2.33.so) ==305970== by 0x49D2CDB: fclose_nointr (fd-util.c:108) ==305970== by 0x49D2D3D: safe_fclose (fd-util.c:124) ==305970== by 0x4A4BCCC: fclosep (fd-util.h:41) ==305970== by 0x4A4E00F: bus_match_to_string (bus-match.c:859) ==305970== by 0x4016C2: LLVMFuzzerTestOneInput (fuzz-bus-match.c:58) ==305970== by 0x401A58: main (fuzz-main.c:39) ==305970== Block was alloc'd at ==305970== at 0x483FAE5: calloc (vg_replace_malloc.c:760) ==305970== by 0x4C98787: open_memstream (in /usr/lib64/libc-2.33.so) ==305970== by 0x49D56D6: open_memstream_unlocked (fileio.c:97) ==305970== by 0x4A4DEC5: bus_match_to_string (bus-match.c:859) ==305970== by 0x4016C2: LLVMFuzzerTestOneInput (fuzz-bus-match.c:58) ==305970== by 0x401A58: main (fuzz-main.c:39) ==305970== So the fclose() which is called from _cleanup_fclose_ clearly reallocates the buffer (maybe to save memory?). open_memstream(3) says: The locations referred to by these pointers are updated each time the stream is flushed (fflush(3)) and when the stream is closed (fclose(3)). This seems to mean that we should close the stream first before grabbing the buffer pointer. --- src/libsystemd/sd-bus/bus-match.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index db8da6ee12..2a3e7039a7 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -846,7 +846,7 @@ fail: } char *bus_match_to_string(struct bus_match_component *components, unsigned n_components) { - char *buffer = NULL; + _cleanup_free_ char *buffer = NULL; size_t size = 0; int r; @@ -855,7 +855,7 @@ char *bus_match_to_string(struct bus_match_component *components, unsigned n_com assert(components); - _cleanup_fclose_ FILE *f = open_memstream_unlocked(&buffer, &size); + FILE *f = open_memstream_unlocked(&buffer, &size); if (!f) return NULL; @@ -878,10 +878,10 @@ char *bus_match_to_string(struct bus_match_component *components, unsigned n_com } r = fflush_and_check(f); + safe_fclose(f); if (r < 0) return NULL; - - return buffer; + return TAKE_PTR(buffer); } int bus_match_add( From fc561c8eac84290b4541af01568c4482fc6f26a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 21:20:00 +0100 Subject: [PATCH 05/13] sd-bus: let bus_match_dump() take an output file --- src/libsystemd/sd-bus/bus-match.c | 20 ++++++++++---------- src/libsystemd/sd-bus/bus-match.h | 4 +++- src/libsystemd/sd-bus/test-bus-chat.c | 2 +- src/libsystemd/sd-bus/test-bus-match.c | 4 ++-- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index 2a3e7039a7..f624d1ab3c 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -1016,7 +1016,7 @@ const char* bus_match_node_type_to_string(enum bus_match_node_type t, char buf[] } } -void bus_match_dump(struct bus_match_node *node, unsigned level) { +void bus_match_dump(FILE *out, struct bus_match_node *node, unsigned level) { _cleanup_free_ char *pfx = NULL; char buf[32]; @@ -1024,29 +1024,29 @@ void bus_match_dump(struct bus_match_node *node, unsigned level) { return; pfx = strrep(" ", level); - printf("%s[%s]", strempty(pfx), bus_match_node_type_to_string(node->type, buf, sizeof(buf))); + fprintf(out, "%s[%s]", strempty(pfx), bus_match_node_type_to_string(node->type, buf, sizeof(buf))); if (node->type == BUS_MATCH_VALUE) { if (node->parent->type == BUS_MATCH_MESSAGE_TYPE) - printf(" <%u>\n", node->value.u8); + fprintf(out, " <%u>\n", node->value.u8); else - printf(" <%s>\n", node->value.str); + fprintf(out, " <%s>\n", node->value.str); } else if (node->type == BUS_MATCH_ROOT) - puts(" root"); + fputs(" root\n", out); else if (node->type == BUS_MATCH_LEAF) - printf(" %p/%p\n", node->leaf.callback->callback, - container_of(node->leaf.callback, sd_bus_slot, match_callback)->userdata); + fprintf(out, " %p/%p\n", node->leaf.callback->callback, + container_of(node->leaf.callback, sd_bus_slot, match_callback)->userdata); else - putchar('\n'); + putc('\n', out); if (BUS_MATCH_CAN_HASH(node->type)) { struct bus_match_node *c; HASHMAP_FOREACH(c, node->compare.children) - bus_match_dump(c, level + 1); + bus_match_dump(out, c, level + 1); } for (struct bus_match_node *c = node->child; c; c = c->next) - bus_match_dump(c, level + 1); + bus_match_dump(out, c, level + 1); } enum bus_match_scope bus_match_get_scope(const struct bus_match_component *components, unsigned n_components) { diff --git a/src/libsystemd/sd-bus/bus-match.h b/src/libsystemd/sd-bus/bus-match.h index d079f6aec7..6042f90fba 100644 --- a/src/libsystemd/sd-bus/bus-match.h +++ b/src/libsystemd/sd-bus/bus-match.h @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include + #include "sd-bus.h" #include "hashmap.h" @@ -68,7 +70,7 @@ int bus_match_remove(struct bus_match_node *root, struct match_callback *callbac void bus_match_free(struct bus_match_node *node); -void bus_match_dump(struct bus_match_node *node, unsigned level); +void bus_match_dump(FILE *out, struct bus_match_node *node, unsigned level); const char* bus_match_node_type_to_string(enum bus_match_node_type t, char buf[], size_t l); enum bus_match_node_type bus_match_node_type_from_string(const char *k, size_t n); diff --git a/src/libsystemd/sd-bus/test-bus-chat.c b/src/libsystemd/sd-bus/test-bus-chat.c index ba367677cf..df6dd62151 100644 --- a/src/libsystemd/sd-bus/test-bus-chat.c +++ b/src/libsystemd/sd-bus/test-bus-chat.c @@ -101,7 +101,7 @@ static int server_init(sd_bus **_bus) { goto fail; } - bus_match_dump(&bus->match_callbacks, 0); + bus_match_dump(stdout, &bus->match_callbacks, 0); *_bus = bus; return 0; diff --git a/src/libsystemd/sd-bus/test-bus-match.c b/src/libsystemd/sd-bus/test-bus-match.c index 031e42ff46..701476ed03 100644 --- a/src/libsystemd/sd-bus/test-bus-match.c +++ b/src/libsystemd/sd-bus/test-bus-match.c @@ -105,7 +105,7 @@ int main(int argc, char *argv[]) { assert_se(match_add(slots, &root, "arg4has='po'", 17) >= 0); assert_se(match_add(slots, &root, "arg4='pi'", 18) >= 0); - bus_match_dump(&root, 0); + bus_match_dump(stdout, &root, 0); assert_se(sd_bus_message_new_signal(bus, &m, "/foo/bar", "bar.x", "waldo") >= 0); assert_se(sd_bus_message_append(m, "ssssas", "one", "two", "/prefix/three", "prefix.four", 3, "pi", "pa", "po") >= 0); @@ -118,7 +118,7 @@ int main(int argc, char *argv[]) { assert_se(bus_match_remove(&root, &slots[8].match_callback) >= 0); assert_se(bus_match_remove(&root, &slots[13].match_callback) >= 0); - bus_match_dump(&root, 0); + bus_match_dump(stdout, &root, 0); zero(mask); assert_se(bus_match_run(NULL, &root, m) == 0); From c25eb44aef73095545a733e4d446c6325148caab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 21:22:47 +0100 Subject: [PATCH 06/13] sd-bus: avoid alloc and missing oom check in bus_match_dump() --- src/libsystemd/sd-bus/bus-match.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index f624d1ab3c..eb719c0105 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -1017,14 +1017,12 @@ const char* bus_match_node_type_to_string(enum bus_match_node_type t, char buf[] } void bus_match_dump(FILE *out, struct bus_match_node *node, unsigned level) { - _cleanup_free_ char *pfx = NULL; char buf[32]; if (!node) return; - pfx = strrep(" ", level); - fprintf(out, "%s[%s]", strempty(pfx), bus_match_node_type_to_string(node->type, buf, sizeof(buf))); + fprintf(out, "%*s[%s]", 2 * level, "", bus_match_node_type_to_string(node->type, buf, sizeof(buf))); if (node->type == BUS_MATCH_VALUE) { if (node->parent->type == BUS_MATCH_MESSAGE_TYPE) From dd2e9b7658ba2da021ec1a7a053375f7ba698361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 21:25:14 +0100 Subject: [PATCH 07/13] test-bus-match: small modernization --- src/libsystemd/sd-bus/test-bus-match.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libsystemd/sd-bus/test-bus-match.c b/src/libsystemd/sd-bus/test-bus-match.c index 701476ed03..7a20a6c3c4 100644 --- a/src/libsystemd/sd-bus/test-bus-match.c +++ b/src/libsystemd/sd-bus/test-bus-match.c @@ -37,13 +37,12 @@ static bool mask_contains(unsigned a[], unsigned n) { } static int match_add(sd_bus_slot *slots, struct bus_match_node *root, const char *match, int value) { - struct bus_match_component *components = NULL; - unsigned n_components = 0; + struct bus_match_component *components; + unsigned n_components; sd_bus_slot *s; int r; s = slots + value; - zero(*s); r = bus_match_parse(match, &components, &n_components); if (r < 0) @@ -74,8 +73,7 @@ int main(int argc, char *argv[]) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - enum bus_match_node_type i; - sd_bus_slot slots[19]; + sd_bus_slot slots[19] = {}; int r; test_setup_logging(LOG_INFO); @@ -124,7 +122,7 @@ int main(int argc, char *argv[]) { assert_se(bus_match_run(NULL, &root, m) == 0); assert_se(mask_contains((unsigned[]) { 9, 5, 10, 12, 14, 7, 15, 16, 17 }, 9)); - for (i = 0; i < _BUS_MATCH_NODE_TYPE_MAX; i++) { + for (enum bus_match_node_type i = 0; i < _BUS_MATCH_NODE_TYPE_MAX; i++) { char buf[32]; const char *x; From 84f11eda20e99f8496731a19dda41bffe862b99c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Mar 2021 21:29:48 +0100 Subject: [PATCH 08/13] fuzz-bus-match: new fuzzer This fuzzer is based on test-bus-match. Even the initial corpus is derived entirely from it. https://bugzilla.redhat.com/show_bug.cgi?id=1935084 shows an crash in bus_match_parse(). I checked the coverage stats on oss-fuzz, and sadly existing fuzzing did not cover this code at all. --- src/libsystemd/meson.build | 2 + src/libsystemd/sd-bus/fuzz-bus-match.c | 86 ++++++++++++++++++++++++++ test/fuzz/fuzz-bus-match/test.input | 18 ++++++ 3 files changed, 106 insertions(+) create mode 100644 src/libsystemd/sd-bus/fuzz-bus-match.c create mode 100644 test/fuzz/fuzz-bus-match/test.input diff --git a/src/libsystemd/meson.build b/src/libsystemd/meson.build index 2fe1978d02..ad50815a7b 100644 --- a/src/libsystemd/meson.build +++ b/src/libsystemd/meson.build @@ -320,4 +320,6 @@ endif fuzzers += [ [['src/libsystemd/sd-bus/fuzz-bus-message.c']], + + [['src/libsystemd/sd-bus/fuzz-bus-match.c']], ] diff --git a/src/libsystemd/sd-bus/fuzz-bus-match.c b/src/libsystemd/sd-bus/fuzz-bus-match.c new file mode 100644 index 0000000000..0585338e28 --- /dev/null +++ b/src/libsystemd/sd-bus/fuzz-bus-match.c @@ -0,0 +1,86 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "bus-internal.h" +#include "bus-match.h" +#include "env-util.h" +#include "fd-util.h" +#include "fileio.h" +#include "fuzz.h" + +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { + _cleanup_free_ char *out = NULL; /* out should be freed after g */ + size_t out_size; + _cleanup_fclose_ FILE *g = NULL; + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + int r; + + /* We don't want to fill the logs with messages about parse errors. + * Disable most logging if not running standalone */ + if (!getenv("SYSTEMD_LOG_LEVEL")) + log_set_max_level(LOG_CRIT); + + r = sd_bus_new(&bus); + assert_se(r >= 0); + + struct bus_match_node root = { + .type = BUS_MATCH_ROOT, + }; + + /* Note that we use the pointer to match_callback substructure, but the code + * uses container_of() to access outside of the passed-in type. */ + sd_bus_slot slot = { + .type = BUS_MATCH_CALLBACK, + .match_callback = {}, + }; + + if (getenv_bool("SYSTEMD_FUZZ_OUTPUT") <= 0) + assert_se(g = open_memstream_unlocked(&out, &out_size)); + + for (size_t offset = 0; offset < size; ) { + _cleanup_free_ char *line = NULL; + char *end; + + end = memchr((char*) data + offset, '\n', size - offset); + + line = memdup_suffix0((char*) data + offset, + end ? end - (char*) data - offset : size - offset); + if (!line) + return log_oom_debug(); + + offset = end ? (size_t) (end - (char*) data + 1) : size; + + struct bus_match_component *components; + unsigned n_components; + r = bus_match_parse(line, &components, &n_components); + if (IN_SET(r, -EINVAL, -ENOMEM)) { + log_debug_errno(r, "Failed to parse line: %m"); + continue; + } + assert_se(r >= 0); /* We only expect EINVAL and ENOMEM errors, or success. */ + + log_debug("Parsed %u components.", n_components); + + _cleanup_free_ char *again = bus_match_to_string(components, n_components); + if (!again) { + bus_match_parse_free(components, n_components); + log_oom(); + break; + } + + if (g) + fprintf(g, "%s\n", again); + + r = bus_match_add(&root, components, n_components, &slot.match_callback); + bus_match_parse_free(components, n_components); + if (r < 0) { + log_error_errno(r, "Failed to add match: %m"); + break; + } + } + + bus_match_dump(g ?: stdout, &root, 0); /* We do this even on failure, to check consistency after error. */ + bus_match_free(&root); + + return 0; +} diff --git a/test/fuzz/fuzz-bus-match/test.input b/test/fuzz/fuzz-bus-match/test.input new file mode 100644 index 0000000000..92c02fc75a --- /dev/null +++ b/test/fuzz/fuzz-bus-match/test.input @@ -0,0 +1,18 @@ +arg2='wal\'do',sender='foo',type='signal',interface='bar.x', +arg2='wal\'do2',sender='foo',type='signal',interface='bar.x', +arg3='test',sender='foo',type='signal',interface='bar.x', +arg3='test',sender='foo',type='method_call',interface='bar.x', + +interface='quux.x' +interface='bar.x' +member='waldo',path='/foo/bar' +path='/foo/bar' +path_namespace='/foo' +path_namespace='/foo/quux' +arg1='two' +member='waldo',arg2path='/prefix/' +member=waldo,path='/foo/bar',arg3namespace='prefix' +arg4has='pi' +arg4has='pa' +arg4has='po' +arg4='pi' From f92d8e4446bb40603574c9010b02e26b150af8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Mar 2021 17:10:53 +0100 Subject: [PATCH 09/13] fuzz-bus-match: add example from bugzilla#1935084 The fuzzer seems to have no trouble with this sample. It seems that the problem reported in the bug is not caused by the match parsing code. But let's add the sample just in case. https://bugzilla.redhat.com/show_bug.cgi?id=1935084 --- test/fuzz/fuzz-bus-match/bugzilla1935084.input | 1 + 1 file changed, 1 insertion(+) create mode 100644 test/fuzz/fuzz-bus-match/bugzilla1935084.input diff --git a/test/fuzz/fuzz-bus-match/bugzilla1935084.input b/test/fuzz/fuzz-bus-match/bugzilla1935084.input new file mode 100644 index 0000000000..1a0b3cfaab --- /dev/null +++ b/test/fuzz/fuzz-bus-match/bugzilla1935084.input @@ -0,0 +1 @@ +type='signal',sender='org.freedesktop.DBus',path='/org/freedesktop/DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged',arg0=':1.134' From 8df3f44c90a3e479b717e8cc4bb26c95ebbc5cb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Mar 2021 15:16:18 +0100 Subject: [PATCH 10/13] sd-bus: fix memleak in failure path in bus_match_parse() --- src/libsystemd/sd-bus/bus-match.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index eb719c0105..71a6cd1318 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -733,12 +733,16 @@ int bus_match_parse( p += strspn(p, " "); eq = strchr(p, '='); - if (!eq) - return -EINVAL; + if (!eq) { + r = -EINVAL; + goto fail; + } t = bus_match_node_type_from_string(p, eq - p); - if (t < 0) - return -EINVAL; + if (t < 0) { + r = -EINVAL; + goto fail; + } quoted = eq[1] == '\''; From c1c9510c9bb6922bdb315d39b1961e75ebdbbd69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Mar 2021 16:01:50 +0100 Subject: [PATCH 11/13] fuzz-main: allow the number of runs to be overridden This is useful when debugging. --- src/fuzz/fuzz-main.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/fuzz/fuzz-main.c b/src/fuzz/fuzz-main.c index c39db5d59d..cf70424c16 100644 --- a/src/fuzz/fuzz-main.c +++ b/src/fuzz/fuzz-main.c @@ -1,9 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "alloc-util.h" -#include "log.h" #include "fileio.h" #include "fuzz.h" +#include "log.h" +#include "parse-util.h" +#include "string-util.h" #include "tests.h" /* This is a test driver for the systemd fuzzers that provides main function @@ -15,13 +17,22 @@ /* This one was borrowed from * https://github.com/google/oss-fuzz/blob/646fca1b506b056db3a60d32c4a1a7398f171c94/infra/base-images/base-runner/bad_build_check#L19 */ -#define MIN_NUMBER_OF_RUNS 4 +#define NUMBER_OF_RUNS 4 int main(int argc, char **argv) { int r; test_setup_logging(LOG_DEBUG); + unsigned number_of_runs = NUMBER_OF_RUNS; + + const char *v = getenv("SYSTEMD_FUZZ_RUNS"); + if (!isempty(v)) { + r = safe_atou(v, &number_of_runs); + if (r < 0) + return log_error_errno(r, "Failed to parse SYSTEMD_FUZZ_RUNS=%s: %m", v); + } + for (int i = 1; i < argc; i++) { _cleanup_free_ char *buf = NULL; size_t size; @@ -35,7 +46,7 @@ int main(int argc, char **argv) { } printf("%s... ", name); fflush(stdout); - for (int j = 0; j < MIN_NUMBER_OF_RUNS; j++) + for (unsigned j = 0; j < number_of_runs; j++) if (LLVMFuzzerTestOneInput((uint8_t*)buf, size) == EXIT_TEST_SKIP) return EXIT_TEST_SKIP; printf("ok\n"); From 48eb2af68a0258fcee49633934ddb67806cb5e98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Mar 2021 16:06:15 +0100 Subject: [PATCH 12/13] docs: document fuzzer variables --- docs/ENVIRONMENT.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index ab10a4fc7e..ad2d3ad84b 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -301,3 +301,15 @@ installed systemd tests: * `$SYSTEMD_SYSVRCND_PATH` — Controls where `systemd-sysv-generator` looks for SysV init script runlevel link farms. + +fuzzers: + +* `$SYSTEMD_FUZZ_OUTPUT` — A boolean that specifies whether to write output to + stdout. Setting to true is useful in manual invocations, since all output is + suppressed by default. + +* `$SYSTEMD_FUZZ_RUNS` — The number of times execution should be repeated in + manual invocations. + +Note that is may be also useful to set `$SYSTEMD_LOG_LEVEL`, since all logging +is suppressed by default. From 7a39ec2e3edcf2cd8759bf0cb5ba1f5d7c636e6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 5 Mar 2021 16:36:37 +0100 Subject: [PATCH 13/13] sd-bus: remove unnecessary variable Also use structued initialization in one more place, use '\0' for NUL bytes, and move variable to the right block (the code was OK, but it is strange to have 'char *value' defined in a different scope then 'size_t value_allocated'). --- src/libsystemd/sd-bus/bus-match.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index 71a6cd1318..96b50aeaed 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -710,35 +710,34 @@ int bus_match_parse( struct bus_match_component **ret_components, unsigned *ret_n_components) { - const char *p = match; struct bus_match_component *components = NULL; size_t components_allocated = 0; unsigned n_components = 0; - _cleanup_free_ char *value = NULL; int r; assert(match); assert(ret_components); assert(ret_n_components); - while (*p != 0) { + while (*match != '\0') { const char *eq, *q; enum bus_match_node_type t; unsigned j = 0; + _cleanup_free_ char *value = NULL; size_t value_allocated = 0; bool escaped = false, quoted; uint8_t u; /* Avahi's match rules appear to include whitespace, skip over it */ - p += strspn(p, " "); + match += strspn(match, " "); - eq = strchr(p, '='); + eq = strchr(match, '='); if (!eq) { r = -EINVAL; goto fail; } - t = bus_match_node_type_from_string(p, eq - p); + t = bus_match_node_type_from_string(match, eq - match); if (t < 0) { r = -EINVAL; goto fail; @@ -748,14 +747,14 @@ int bus_match_parse( for (q = eq + 1 + quoted;; q++) { - if (*q == 0) { + if (*q == '\0') { if (quoted) { r = -EINVAL; goto fail; } else { if (value) - value[j] = 0; + value[j] = '\0'; break; } } @@ -769,14 +768,13 @@ int bus_match_parse( if (quoted) { if (*q == '\'') { if (value) - value[j] = 0; + value[j] = '\0'; break; } } else { if (*q == ',') { if (value) - value[j] = 0; - + value[j] = '\0'; break; } } @@ -813,10 +811,11 @@ int bus_match_parse( goto fail; } - components[n_components].type = t; - components[n_components].value_str = TAKE_PTR(value); - components[n_components].value_u8 = u; - n_components++; + components[n_components++] = (struct bus_match_component) { + .type = t, + .value_str = TAKE_PTR(value), + .value_u8 = u, + }; if (q[quoted] == 0) break; @@ -826,7 +825,7 @@ int bus_match_parse( goto fail; } - p = q + 1 + quoted; + match = q + 1 + quoted; } /* Order the whole thing, so that we always generate the same tree */