From f810b631cd69c73a2f27a4adccfa7ed940fbb711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 22:03:15 +0100 Subject: [PATCH 01/10] Revert "Replace use of snprintf with xsprintf" This reverts commit a7419dbc59da5c8cc9e90b3d96bc947cad91ae16. _All_ changes in that commit were wrong. Fixes #8211. --- src/basic/log.c | 4 ++-- src/core/unit.c | 2 +- src/udev/collect/collect.c | 2 +- src/udev/udev-builtin-net_id.c | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 88aea4e1da..b7e3cf3527 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -365,7 +365,7 @@ static int write_to_console( highlight = LOG_PRI(level) <= LOG_ERR && show_color; if (show_location) { - xsprintf(location, "(%s:%i) ", file, line); + snprintf(location, sizeof(location), "(%s:%i) ", file, line); iovec[n++] = IOVEC_MAKE_STRING(location); } @@ -800,7 +800,7 @@ static void log_assert( return; DISABLE_WARNING_FORMAT_NONLITERAL; - xsprintf(buffer, format, text, file, line, func); + snprintf(buffer, sizeof buffer, format, text, file, line, func); REENABLE_WARNING; log_abort_msg = buffer; diff --git a/src/core/unit.c b/src/core/unit.c index 61a7203744..cbb3015590 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1699,7 +1699,7 @@ static void unit_status_log_starting_stopping_reloading(Unit *u, JobType t) { format = unit_get_status_message_format(u, t); DISABLE_WARNING_FORMAT_NONLITERAL; - xsprintf(buf, format, unit_description(u)); + snprintf(buf, sizeof buf, format, unit_description(u)); REENABLE_WARNING; mid = t == JOB_START ? "MESSAGE_ID=" SD_MESSAGE_UNIT_STARTING_STR : diff --git a/src/udev/collect/collect.c b/src/udev/collect/collect.c index 3e278bd637..2821640e93 100644 --- a/src/udev/collect/collect.c +++ b/src/udev/collect/collect.c @@ -94,7 +94,7 @@ static int prepare(char *dir, char *filename) if (r < 0 && errno != EEXIST) return -errno; - xsprintf(buf, "%s/%s", dir, filename); + snprintf(buf, sizeof buf, "%s/%s", dir, filename); fd = open(buf, O_RDWR|O_CREAT|O_CLOEXEC, S_IRUSR|S_IWUSR); if (fd < 0) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 945585d82a..16a78657d7 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -275,7 +275,7 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { goto out; } - xsprintf(slots, "%s/slots", udev_device_get_syspath(pci)); + snprintf(slots, sizeof slots, "%s/slots", udev_device_get_syspath(pci)); dir = opendir(slots); if (!dir) { err = -errno; @@ -294,7 +294,7 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { if (i < 1) continue; - xsprintf(str, "%s/%s/address", slots, dent->d_name); + snprintf(str, sizeof str, "%s/%s/address", slots, dent->d_name); if (read_one_line_file(str, &address) >= 0) { /* match slot address with device by stripping the function */ if (strneq(address, udev_device_get_sysname(names->pcidev), strlen(address))) From 4ad2b562cece42a895f40c810beea75d6c72ef45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 22:09:16 +0100 Subject: [PATCH 02/10] basic/log: voidify snprintf statements The buffers are fixed size, so the message may not fit, but we don't particularly care. --- src/basic/log.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index b7e3cf3527..76771397c8 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -365,7 +365,7 @@ static int write_to_console( highlight = LOG_PRI(level) <= LOG_ERR && show_color; if (show_location) { - snprintf(location, sizeof(location), "(%s:%i) ", file, line); + (void) snprintf(location, sizeof location, "(%s:%i) ", file, line); iovec[n++] = IOVEC_MAKE_STRING(location); } @@ -698,7 +698,7 @@ int log_internalv_realm( if (error != 0) errno = error; - vsnprintf(buffer, sizeof(buffer), format, ap); + (void) vsnprintf(buffer, sizeof buffer, format, ap); return log_dispatch_internal(level, error, file, line, func, NULL, NULL, NULL, NULL, buffer); } @@ -757,7 +757,7 @@ int log_object_internalv( } else b = buffer = newa(char, LINE_MAX); - vsnprintf(b, LINE_MAX, format, ap); + (void) vsnprintf(b, LINE_MAX, format, ap); return log_dispatch_internal(level, error, file, line, func, object_field, object, extra_field, extra, buffer); @@ -800,7 +800,7 @@ static void log_assert( return; DISABLE_WARNING_FORMAT_NONLITERAL; - snprintf(buffer, sizeof buffer, format, text, file, line, func); + (void) snprintf(buffer, sizeof buffer, format, text, file, line, func); REENABLE_WARNING; log_abort_msg = buffer; @@ -974,7 +974,7 @@ int log_struct_internal( errno = error; va_copy(aq, ap); - vsnprintf(buf, sizeof(buf), format, aq); + (void) vsnprintf(buf, sizeof buf, format, aq); va_end(aq); if (startswith(buf, "MESSAGE=")) { @@ -1273,7 +1273,7 @@ int log_syntax_internal( errno = error; va_start(ap, format); - vsnprintf(buffer, sizeof(buffer), format, ap); + (void) vsnprintf(buffer, sizeof buffer, format, ap); va_end(ap); if (unit) From a3bf71378e5ffea7df8b45a44e7a781803ac4b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 23:07:58 +0100 Subject: [PATCH 03/10] basic/log: make log_object_internalv static It makes the code easier to read, because it's obvious that the function cannot be called from elsewhere. --- src/basic/log.c | 3 ++- src/basic/log.h | 13 ------------- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 76771397c8..bb60ce5298 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -721,7 +721,8 @@ int log_internal_realm( return r; } -int log_object_internalv( +_printf_(10,0) +static int log_object_internalv( int level, int error, const char *file, diff --git a/src/basic/log.h b/src/basic/log.h index 5b5a25bd6d..efcf0f1bfc 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -144,19 +144,6 @@ int log_object_internal( const char *extra, const char *format, ...) _printf_(10,11); -int log_object_internalv( - int level, - int error, - const char *file, - int line, - const char *func, - const char *object_field, - const char *object, - const char *extra_field, - const char *extra, - const char *format, - va_list ap) _printf_(10,0); - int log_struct_internal( int level, int error, From 2af42b9a787371b6ed2787a663e1149314322777 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 23:39:17 +0100 Subject: [PATCH 04/10] journal: drop left-over header line Fixup for 53978b98f9ecc16dca216e8dab17d0d5622c9056. --- src/journal/journald-native.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/journal/journald-native.h b/src/journal/journald-native.h index 7ca17ec609..f1b4abc41a 100644 --- a/src/journal/journald-native.h +++ b/src/journal/journald-native.h @@ -22,8 +22,6 @@ #include "journald-server.h" -bool valid_user_field(const char *p, size_t l, bool allow_protected); - void server_process_native_message(Server *s, const void *buffer, size_t buffer_size, const struct ucred *ucred, const struct timeval *tv, const char *label, size_t label_len); void server_process_native_file(Server *s, int fd, const struct ucred *ucred, const struct timeval *tv, const char *label, size_t label_len); From 4dd09c6a1fc70f6bb0155661ab0a980b7a507986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 23:45:27 +0100 Subject: [PATCH 05/10] basic/log: fix confusion with parameters to log_dispatch_internal log_dispatch_internal has only one caller where the extra_field/extra params are not null: log_unit_full. When log_unit_full() was called, when we got to log_dispatch_internal, our header would look like this: PRIORITY=7 SYSLOG_FACILITY=3 CODE_FILE=../src/core/manager.c CODE_LINE=2145 CODE_FUNC=manager_invoke_sigchld_event USER_UNIT=gnome-terminal-server.service 65dffa7a3b984a6d9a46f0b8fb57710bUSER_INVOCATION_ID= SYSLOG_IDENTIFIER=systemd It took me a while to understand why I'm not seeing mangled messages in the journal (after all, "" is a valid rvalue for log messages). The answer is that journald rejects any field name which starts with a digit, and the MESSAGE_ID that was used here starts with a digit. Hence, those lines would be silently filtered out. --- src/basic/log.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/log.c b/src/basic/log.c index bb60ce5298..85f8eeaa9b 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -577,8 +577,8 @@ int log_dispatch_internal( const char *func, const char *object_field, const char *object, - const char *extra, const char *extra_field, + const char *extra, char *buffer) { assert(buffer); From f8e6f4aaedf8c96bf909e027631f4910ac9c4539 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 22 Feb 2018 23:55:14 +0100 Subject: [PATCH 06/10] basic/log: make sure header is printed correctly, add test If log_do_header() was called with overly long parameters, it'd generate improper output. Essentially, it'd be truncated at random point, in particular missing a newline at the end, so it'd run with the next field, usually MESSAGE=. log_do_header is called with parameters from compiled code (file name, lien nubmer, etc), so in practice this was unlikely to ever be a problem, but it is possible. In particular, if systemd was compiled from sources in some deeply nested directory (which happens for example in mock and other build roots), the filename could be very long. As a safety measure, let's truncate all parameters to 256 bytes. So we have 5 fields which are 256 bytes (plus the field name prefix), and a few other fields with fixed width. This must always fit in the 2048 byte buffer. I don't think there's much gain in calculating the required length precisely, since it's a lot of fields and a few bytes allocated on the stack don't matter. --- src/basic/log.c | 64 +++++++++++++++++++++++---------------------- src/test/test-log.c | 39 ++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 85f8eeaa9b..19c4ec7649 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -498,38 +498,40 @@ static int log_do_header( const char *file, int line, const char *func, const char *object_field, const char *object, const char *extra_field, const char *extra) { + int r; - snprintf(header, size, - "PRIORITY=%i\n" - "SYSLOG_FACILITY=%i\n" - "%s%s%s" - "%s%.*i%s" - "%s%s%s" - "%s%.*i%s" - "%s%s%s" - "%s%s%s" - "SYSLOG_IDENTIFIER=%s\n", - LOG_PRI(level), - LOG_FAC(level), - isempty(file) ? "" : "CODE_FILE=", - isempty(file) ? "" : file, - isempty(file) ? "" : "\n", - line ? "CODE_LINE=" : "", - line ? 1 : 0, line, /* %.0d means no output too, special case for 0 */ - line ? "\n" : "", - isempty(func) ? "" : "CODE_FUNC=", - isempty(func) ? "" : func, - isempty(func) ? "" : "\n", - error ? "ERRNO=" : "", - error ? 1 : 0, error, - error ? "\n" : "", - isempty(object) ? "" : object_field, - isempty(object) ? "" : object, - isempty(object) ? "" : "\n", - isempty(extra) ? "" : extra_field, - isempty(extra) ? "" : extra, - isempty(extra) ? "" : "\n", - program_invocation_short_name); + r = snprintf(header, size, + "PRIORITY=%i\n" + "SYSLOG_FACILITY=%i\n" + "%s%.256s%s" /* CODE_FILE */ + "%s%.*i%s" /* CODE_LINE */ + "%s%.256s%s" /* CODE_FUNC */ + "%s%.*i%s" /* ERRNO */ + "%s%.256s%s" /* object */ + "%s%.256s%s" /* extra */ + "SYSLOG_IDENTIFIER=%.256s\n", + LOG_PRI(level), + LOG_FAC(level), + isempty(file) ? "" : "CODE_FILE=", + isempty(file) ? "" : file, + isempty(file) ? "" : "\n", + line ? "CODE_LINE=" : "", + line ? 1 : 0, line, /* %.0d means no output too, special case for 0 */ + line ? "\n" : "", + isempty(func) ? "" : "CODE_FUNC=", + isempty(func) ? "" : func, + isempty(func) ? "" : "\n", + error ? "ERRNO=" : "", + error ? 1 : 0, error, + error ? "\n" : "", + isempty(object) ? "" : object_field, + isempty(object) ? "" : object, + isempty(object) ? "" : "\n", + isempty(extra) ? "" : extra_field, + isempty(extra) ? "" : extra, + isempty(extra) ? "" : "\n", + program_invocation_short_name); + assert((size_t) r < size); return 0; } diff --git a/src/test/test-log.c b/src/test/test-log.c index fd19899480..4a3c8955e3 100644 --- a/src/test/test-log.c +++ b/src/test/test-log.c @@ -35,19 +35,18 @@ assert_cc((LOG_REALM_PLUS_LEVEL(LOG_REALM_SYSTEMD, LOG_LOCAL3 | LOG_DEBUG) & LOG assert_cc((LOG_REALM_PLUS_LEVEL(LOG_REALM_UDEV, LOG_USER | LOG_INFO) & LOG_PRIMASK) == LOG_INFO); -int main(int argc, char* argv[]) { - - log_set_target(LOG_TARGET_CONSOLE); - log_open(); +#define X10(x) x x x x x x x x x x +#define X100(x) X10(X10(x)) +#define X1000(x) X100(X10(x)) +static void test_log_console(void) { log_struct(LOG_INFO, "MESSAGE=Waldo PID="PID_FMT, getpid_cached(), "SERVICE=piepapo", NULL); +} - log_set_target(LOG_TARGET_JOURNAL); - log_open(); - +static void test_log_journal(void) { log_struct(LOG_INFO, "MESSAGE=Foobar PID="PID_FMT, getpid_cached(), "SERVICE=foobar", @@ -59,6 +58,32 @@ int main(int argc, char* argv[]) { (int) 1, 'A', (short) 2, (long int) 3, (long long int) 4, (void*) 1, "foo", (float) 2.5f, (double) 3.5, (long double) 4.5, "SUFFIX=GOT IT", NULL); +} + +static void test_long_lines(void) { + log_object_internal(LOG_NOTICE, + EUCLEAN, + X1000("abcd_") ".txt", + 1000000, + X1000("fff") "unc", + "OBJECT=", + X1000("obj_") "ect", + "EXTRA=", + X1000("ext_") "tra", + "asdfasdf %s asdfasdfa", "foobar"); +} + +int main(int argc, char* argv[]) { + int target; + + for (target = 0; target < _LOG_TARGET_MAX; target++) { + log_set_target(target); + log_open(); + + test_log_console(); + test_log_journal(); + test_long_lines(); + } return 0; } From 5b464a18e78bcbb2cd00ce662e2c08d8a2dfde59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Feb 2018 10:22:44 +0100 Subject: [PATCH 07/10] systemctl: do not hint logs when a unit cannot be reloaded The logs don't contain any additional information. --- src/systemctl/systemctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 6eec0171c6..e47c3dd2e9 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2942,7 +2942,8 @@ static int start_unit_one( log_error("Failed to %s %s: %s", verb, name, bus_error_message(error, r)); if (!sd_bus_error_has_name(error, BUS_ERROR_NO_SUCH_UNIT) && - !sd_bus_error_has_name(error, BUS_ERROR_UNIT_MASKED)) + !sd_bus_error_has_name(error, BUS_ERROR_UNIT_MASKED) && + !sd_bus_error_has_name(error, BUS_ERROR_JOB_TYPE_NOT_APPLICABLE)) log_error("See %s logs and 'systemctl%s status%s %s' for details.", arg_scope == UNIT_FILE_SYSTEM ? "system" : "user", arg_scope == UNIT_FILE_SYSTEM ? "" : " --user", From cc5bbdb274d6941f74f9227781e6715951cf3dfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Feb 2018 11:09:13 +0100 Subject: [PATCH 08/10] udev/net-id: use _cleanup_ This fixes a minor memleak of 'address' if the file could not be read. --- src/udev/udev-builtin-net_id.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 16a78657d7..0eaf789f16 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -115,6 +115,7 @@ #include "stdio-util.h" #include "string-util.h" #include "udev.h" +#include "udev-util.h" #define ONBOARD_INDEX_MAX (16*1024-1) @@ -236,11 +237,11 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { size_t l; char *s; const char *attr, *port_name; - struct udev_device *pci = NULL; + _cleanup_udev_device_unref_ struct udev_device *pci = NULL; char slots[PATH_MAX]; _cleanup_closedir_ DIR *dir = NULL; struct dirent *dent; - int hotplug_slot = 0, err = 0; + int hotplug_slot = 0; if (sscanf(udev_device_get_sysname(names->pcidev), "%x:%x:%x.%u", &domain, &bus, &slot, &func) != 4) return -ENOENT; @@ -270,21 +271,18 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { /* ACPI _SUN — slot user number */ pci = udev_device_new_from_subsystem_sysname(udev, "subsystem", "pci"); - if (!pci) { - err = -ENOENT; - goto out; - } + if (!pci) + return -ENOENT; snprintf(slots, sizeof slots, "%s/slots", udev_device_get_syspath(pci)); dir = opendir(slots); - if (!dir) { - err = -errno; - goto out; - } + if (!dir) + return -errno; FOREACH_DIRENT_ALL(dent, dir, break) { int i; - char *rest, *address, str[PATH_MAX]; + char *rest, str[PATH_MAX]; + _cleanup_free_ char *address = NULL; if (dent->d_name[0] == '.') continue; @@ -299,7 +297,6 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { /* match slot address with device by stripping the function */ if (strneq(address, udev_device_get_sysname(names->pcidev), strlen(address))) hotplug_slot = i; - free(address); } if (hotplug_slot > 0) @@ -321,9 +318,8 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { if (l == 0) names->pci_slot[0] = '\0'; } -out: - udev_device_unref(pci); - return err; + + return 0; } static int names_vio(struct udev_device *dev, struct netnames *names) { From 73fc96c8ac0aa95477e53838a22ce94918c9d06f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Feb 2018 11:12:19 +0100 Subject: [PATCH 09/10] udev/net-id: check all snprintf return values gcc-8 throws an error if it knows snprintf might truncate output and the return value is ignored: ../src/udev/udev-builtin-net_id.c: In function 'dev_pci_slot': ../src/udev/udev-builtin-net_id.c:297:47: error: '%s' directive output may be truncated writing up to 255 bytes into a region of size between 0 and 4095 [-Werror=format-truncation=] snprintf(str, sizeof str, "%s/%s/address", slots, dent->d_name); ^~ ../src/udev/udev-builtin-net_id.c:297:17: note: 'snprintf' output between 10 and 4360 bytes into a destination of size 4096 snprintf(str, sizeof str, "%s/%s/address", slots, dent->d_name); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors Let's check all return values. This actually makes the code better, because there's no point in trying to open a file when the name has been truncated, etc. --- src/basic/stdio-util.h | 6 ++- src/udev/udev-builtin-net_id.c | 70 +++++++++++++++++----------------- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/basic/stdio-util.h b/src/basic/stdio-util.h index dbfafba269..d3fed365d8 100644 --- a/src/basic/stdio-util.h +++ b/src/basic/stdio-util.h @@ -27,9 +27,11 @@ #include "macro.h" -#define xsprintf(buf, fmt, ...) \ - assert_message_se((size_t) snprintf(buf, ELEMENTSOF(buf), fmt, __VA_ARGS__) < ELEMENTSOF(buf), "xsprintf: " #buf "[] must be big enough") +#define snprintf_ok(buf, len, fmt, ...) \ + ((size_t) snprintf(buf, len, fmt, __VA_ARGS__) < (len)) +#define xsprintf(buf, fmt, ...) \ + assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, __VA_ARGS__), "xsprintf: " #buf "[] must be big enough") #define VA_FORMAT_ADVANCE(format, ap) \ do { \ diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 0eaf789f16..36994360c7 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -274,7 +274,9 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { if (!pci) return -ENOENT; - snprintf(slots, sizeof slots, "%s/slots", udev_device_get_syspath(pci)); + if (!snprintf_ok(slots, sizeof slots, "%s/slots", udev_device_get_syspath(pci))) + return -ENAMETOOLONG; + dir = opendir(slots); if (!dir) return -errno; @@ -292,12 +294,11 @@ static int dev_pci_slot(struct udev_device *dev, struct netnames *names) { if (i < 1) continue; - snprintf(str, sizeof str, "%s/%s/address", slots, dent->d_name); - if (read_one_line_file(str, &address) >= 0) { + if (snprintf_ok(str, sizeof str, "%s/%s/address", slots, dent->d_name) && + read_one_line_file(str, &address) >= 0) /* match slot address with device by stripping the function */ - if (strneq(address, udev_device_get_sysname(names->pcidev), strlen(address))) + if (streq(address, udev_device_get_sysname(names->pcidev))) hotplug_slot = i; - } if (hotplug_slot > 0) break; @@ -513,7 +514,6 @@ static int names_ccw(struct udev_device *dev, struct netnames *names) { const char *bus_id, *subsys; size_t bus_id_len; size_t bus_id_start; - int rc; assert(dev); assert(names); @@ -555,9 +555,9 @@ static int names_ccw(struct udev_device *dev, struct netnames *names) { bus_id += bus_id_start < bus_id_len ? bus_id_start : bus_id_len - 1; /* Store the CCW bus-ID for use as network device name */ - rc = snprintf(names->ccw_busid, sizeof(names->ccw_busid), "c%s", bus_id); - if (rc >= 0 && rc < (int)sizeof(names->ccw_busid)) + if (snprintf_ok(names->ccw_busid, sizeof(names->ccw_busid), "c%s", bus_id)) names->type = NET_CCW; + return 0; } @@ -670,7 +670,7 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (err >= 0 && names.type == NET_CCW) { char str[IFNAMSIZ]; - if (snprintf(str, sizeof(str), "%s%s", prefix, names.ccw_busid) < (int)sizeof(str)) + if (snprintf_ok(str, sizeof str, "%s%s", prefix, names.ccw_busid)) udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); goto out; } @@ -680,7 +680,7 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (err >= 0 && names.type == NET_VIO) { char str[IFNAMSIZ]; - if (snprintf(str, sizeof(str), "%s%s", prefix, names.vio_slot) < (int)sizeof(str)) + if (snprintf_ok(str, sizeof str, "%s%s", prefix, names.vio_slot)) udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); goto out; } @@ -690,7 +690,7 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (err >= 0 && names.type == NET_PLATFORM) { char str[IFNAMSIZ]; - if (snprintf(str, sizeof(str), "%s%s", prefix, names.platform_path) < (int)sizeof(str)) + if (snprintf_ok(str, sizeof str, "%s%s", prefix, names.platform_path)) udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); goto out; } @@ -704,21 +704,21 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (names.type == NET_PCI) { char str[IFNAMSIZ]; - if (names.pci_onboard[0]) - if (snprintf(str, sizeof(str), "%s%s", prefix, names.pci_onboard) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_ONBOARD", str); + if (names.pci_onboard[0] && + snprintf_ok(str, sizeof str, "%s%s", prefix, names.pci_onboard)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_ONBOARD", str); - if (names.pci_onboard_label) - if (snprintf(str, sizeof(str), "%s%s", prefix, names.pci_onboard_label) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_LABEL_ONBOARD", str); + if (names.pci_onboard_label && + snprintf_ok(str, sizeof str, "%s%s", prefix, names.pci_onboard_label)) + udev_builtin_add_property(dev, test, "ID_NET_LABEL_ONBOARD", str); - if (names.pci_path[0]) - if (snprintf(str, sizeof(str), "%s%s", prefix, names.pci_path) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); + if (names.pci_path[0] && + snprintf_ok(str, sizeof str, "%s%s", prefix, names.pci_path)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); - if (names.pci_slot[0]) - if (snprintf(str, sizeof(str), "%s%s", prefix, names.pci_slot) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); + if (names.pci_slot[0] && + snprintf_ok(str, sizeof str, "%s%s", prefix, names.pci_slot)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); goto out; } @@ -727,13 +727,13 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (err >= 0 && names.type == NET_USB) { char str[IFNAMSIZ]; - if (names.pci_path[0]) - if (snprintf(str, sizeof(str), "%s%s%s", prefix, names.pci_path, names.usb_ports) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); + if (names.pci_path[0] && + snprintf_ok(str, sizeof str, "%s%s%s", prefix, names.pci_path, names.usb_ports)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); - if (names.pci_slot[0]) - if (snprintf(str, sizeof(str), "%s%s%s", prefix, names.pci_slot, names.usb_ports) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); + if (names.pci_slot[0] && + snprintf_ok(str, sizeof str, "%s%s%s", prefix, names.pci_slot, names.usb_ports)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); goto out; } @@ -742,13 +742,13 @@ static int builtin_net_id(struct udev_device *dev, int argc, char *argv[], bool if (err >= 0 && names.type == NET_BCMA) { char str[IFNAMSIZ]; - if (names.pci_path[0]) - if (snprintf(str, sizeof(str), "%s%s%s", prefix, names.pci_path, names.bcma_core) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); + if (names.pci_path[0] && + snprintf_ok(str, sizeof str, "%s%s%s", prefix, names.pci_path, names.bcma_core)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_PATH", str); - if (names.pci_slot[0]) - if (snprintf(str, sizeof(str), "%s%s%s", prefix, names.pci_slot, names.bcma_core) < (int)sizeof(str)) - udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); + if (names.pci_slot[0] && + snprintf(str, sizeof str, "%s%s%s", prefix, names.pci_slot, names.bcma_core)) + udev_builtin_add_property(dev, test, "ID_NET_NAME_SLOT", str); goto out; } out: From 780747da75b737b5036f8c17fb99c54c52762eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 23 Feb 2018 13:29:03 +0100 Subject: [PATCH 10/10] basic/log: add an assert that does not recurse into logging functions Then it can be used in the asserts in logging functions without causing infinite recursion. The error is just printed to stderr, it should be good enough for the common case. --- src/basic/log.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 19c4ec7649..72b60da6c6 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -83,6 +83,16 @@ static bool prohibit_ipc = false; * use here. */ static char *log_abort_msg = NULL; +/* An assert to use in logging functions that does not call recursively + * into our logging functions (since that might lead to a loop). */ +#define assert_raw(expr) \ + do { \ + if (_unlikely_(!(expr))) { \ + fputs(#expr "\n", stderr); \ + abort(); \ + } \ + } while (false) + static void log_close_console(void) { if (console_fd < 0) @@ -531,7 +541,7 @@ static int log_do_header( isempty(extra) ? "" : extra, isempty(extra) ? "" : "\n", program_invocation_short_name); - assert((size_t) r < size); + assert_raw((size_t) r < size); return 0; } @@ -583,7 +593,7 @@ int log_dispatch_internal( const char *extra, char *buffer) { - assert(buffer); + assert_raw(buffer); if (error < 0) error = -error;