From 032b75419d7ad6cab188d1ff5c5d1d322eff1171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Apr 2017 14:09:47 -0400 Subject: [PATCH 1/5] basic/log: fix _printf_ annotation on log_object_internalv Fixup for 4b58153dd22172d817055d2a09a0cdf3f4bd9db3. I saw this because of a clang warning. With gcc the -Wformat-nonliteral warning doesn't seem to work as expected. In two places, a string constructed with strjoina is used as the pattern. This is safe, because we're taking a pattern which was already marked with _printf_ and prepending a known value to it. Those places are marked with #pragma to silence the warning. --- src/basic/log.h | 2 +- src/core/selinux-access.c | 7 ++++++- src/locale/localed.c | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/basic/log.h b/src/basic/log.h index 72714e02e5..e578258114 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -115,7 +115,7 @@ int log_object_internalv( const char *extra_field, const char *extra, const char *format, - va_list ap) _printf_(9,0); + va_list ap) _printf_(10,0); int log_struct_internal( int level, diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index 2b96a9551b..0f8a2d68e2 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -135,7 +135,12 @@ _printf_(2, 3) static int log_callback(int type, const char *fmt, ...) { fmt2 = strjoina("selinux: ", fmt); va_start(ap, fmt); - log_internalv(LOG_AUTH | callback_type_to_priority(type), 0, __FILE__, __LINE__, __FUNCTION__, fmt2, ap); +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" + log_internalv(LOG_AUTH | callback_type_to_priority(type), + 0, __FILE__, __LINE__, __FUNCTION__, + fmt2, ap); +#pragma GCC diagnostic pop va_end(ap); return 0; diff --git a/src/locale/localed.c b/src/locale/localed.c index 1cb049e74a..b4798d674c 100644 --- a/src/locale/localed.c +++ b/src/locale/localed.c @@ -436,7 +436,10 @@ static void log_xkb(struct xkb_context *ctx, enum xkb_log_level lvl, const char const char *fmt; fmt = strjoina("libxkbcommon: ", format); +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" log_internalv(LOG_DEBUG, 0, __FILE__, __LINE__, __func__, fmt, args); +#pragma GCC diagnostic pop } #define LOAD_SYMBOL(symbol, dl, name) \ From c8304ba9027aa2bbca3aa9dfe9fcc951c16932e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Apr 2017 14:45:30 -0400 Subject: [PATCH 2/5] =?UTF-8?q?=C2=B5httpd-util:=20use=20#pragma=20to=20si?= =?UTF-8?q?lence=20warning=20about=20nonliteral=20pattern?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is safe, because we're taking a pattern which was already marked with _printf_ and appending a literal string. --- src/journal-remote/microhttpd-util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/journal-remote/microhttpd-util.c b/src/journal-remote/microhttpd-util.c index cae10203c6..f5d2d7967a 100644 --- a/src/journal-remote/microhttpd-util.c +++ b/src/journal-remote/microhttpd-util.c @@ -103,7 +103,10 @@ int mhd_respondf(struct MHD_Connection *connection, errno = -error; fmt = strjoina(format, "\n"); va_start(ap, format); +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" r = vasprintf(&m, fmt, ap); +#pragma GCC diagnostic pop va_end(ap); if (r < 0) From ba360bb05ce9b0bb86821fcdfebf66ccd9f7efd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Apr 2017 14:15:28 -0400 Subject: [PATCH 3/5] tree-wide: mark log_struct with _printf_ and fix fallout log_struct takes multiple format strings, each one followed by arguments. The _printf_ annotation is not sufficiently flexible to express this, but we can still annotate the first format string, though not its arguments (because their number is unknown). With the annotation, the places which specified the message id or similar as the first pattern cause a warning from -Wformat-nonliteral. This can be trivially fixed by putting the MESSAGE= first. This change will help find issues where a non-literal is erroneously used as the pattern. --- src/basic/log.h | 2 +- src/core/execute.c | 4 ++-- src/core/job.c | 6 +++--- src/core/service.c | 2 +- src/core/unit.c | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/basic/log.h b/src/basic/log.h index e578258114..cdb66005d8 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -137,7 +137,7 @@ int log_format_iovec( bool newline_separator, int error, const char *format, - va_list ap); + va_list ap) _printf_(6, 0); /* This modifies the buffer passed! */ int log_dump_internal( diff --git a/src/core/execute.c b/src/core/execute.c index d7798387c5..29988c22ae 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2887,9 +2887,9 @@ static int exec_child( if (line) { log_open(); log_struct(LOG_DEBUG, - LOG_UNIT_ID(unit), "EXECUTABLE=%s", command->path, LOG_UNIT_MESSAGE(unit, "Executing: %s", line), + LOG_UNIT_ID(unit), NULL); log_close(); } @@ -2953,9 +2953,9 @@ int exec_spawn(Unit *unit, return log_oom(); log_struct(LOG_DEBUG, - LOG_UNIT_ID(unit), LOG_UNIT_MESSAGE(unit, "About to execute: %s", line), "EXECUTABLE=%s", command->path, + LOG_UNIT_ID(unit), NULL); pid = fork(); if (pid < 0) diff --git a/src/core/job.c b/src/core/job.c index e2349830a8..a02f1bb2bc 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -800,18 +800,18 @@ static void job_log_status_message(Unit *u, JobType t, JobResult result) { default: log_struct(job_result_log_level[result], - LOG_UNIT_ID(u), LOG_MESSAGE("%s", buf), "RESULT=%s", job_result_to_string(result), + LOG_UNIT_ID(u), NULL); return; } log_struct(job_result_log_level[result], - mid, - LOG_UNIT_ID(u), LOG_MESSAGE("%s", buf), "RESULT=%s", job_result_to_string(result), + LOG_UNIT_ID(u), + mid, NULL); } diff --git a/src/core/service.c b/src/core/service.c index 74054887b9..185173a542 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2693,7 +2693,6 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { log_struct(f == SERVICE_SUCCESS ? LOG_DEBUG : (code == CLD_EXITED ? LOG_NOTICE : LOG_WARNING), - LOG_UNIT_ID(u), LOG_UNIT_MESSAGE(u, "Main process exited, code=%s, status=%i/%s", sigchld_code_to_string(code), status, strna(code == CLD_EXITED @@ -2701,6 +2700,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { : signal_to_string(status))), "EXIT_CODE=%s", sigchld_code_to_string(code), "EXIT_STATUS=%i", status, + LOG_UNIT_ID(u), NULL); if (s->result == SERVICE_SUCCESS) diff --git a/src/core/unit.c b/src/core/unit.c index f76b6c30a8..25ea5a8591 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1497,9 +1497,9 @@ static void unit_status_log_starting_stopping_reloading(Unit *u, JobType t) { * possible, which means we should avoid the low-level unit * name. */ log_struct(LOG_INFO, - mid, - LOG_UNIT_ID(u), LOG_MESSAGE("%s", buf), + LOG_UNIT_ID(u), + mid, NULL); } From 93484b4790e261ac3dab12cba5b20d33c74a9202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 21 Apr 2017 12:52:54 -0400 Subject: [PATCH 4/5] basic/log: expose log_dispatch This is useful when we want to avoid printf formatting on the message. It's nicer than using log_struct with "%s" as the format, because printf is slow and with a large message (like from a backtrace) this would require extra unnecessary memory. I'm not exposing all the fields in the wrapper: only level and errno. Those are the most likely to be useful. --- src/basic/log.c | 13 +++++++------ src/basic/log.h | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 36efc9ac7d..0d0ced00bd 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -553,7 +553,7 @@ static int write_to_journal( return 1; } -static int log_dispatch( +int log_dispatch_internal( int level, int error, const char *file, @@ -653,7 +653,7 @@ int log_dump_internal( if (_likely_(LOG_PRI(level) > log_max_level)) return -error; - return log_dispatch(level, error, file, line, func, NULL, NULL, NULL, NULL, buffer); + return log_dispatch_internal(level, error, file, line, func, NULL, NULL, NULL, NULL, buffer); } int log_internalv( @@ -680,7 +680,7 @@ int log_internalv( vsnprintf(buffer, sizeof(buffer), format, ap); - return log_dispatch(level, error, file, line, func, NULL, NULL, NULL, NULL, buffer); + return log_dispatch_internal(level, error, file, line, func, NULL, NULL, NULL, NULL, buffer); } int log_internal( @@ -744,7 +744,8 @@ int log_object_internalv( vsnprintf(b, l, format, ap); - return log_dispatch(level, error, file, line, func, object_field, object, extra_field, extra, buffer); + return log_dispatch_internal(level, error, file, line, func, + object_field, object, extra_field, extra, buffer); } int log_object_internal( @@ -788,7 +789,7 @@ static void log_assert( log_abort_msg = buffer; - log_dispatch(level, 0, file, line, func, NULL, NULL, NULL, NULL, buffer); + log_dispatch_internal(level, 0, file, line, func, NULL, NULL, NULL, NULL, buffer); } noreturn void log_assert_failed(const char *text, const char *file, int line, const char *func) { @@ -943,7 +944,7 @@ int log_struct_internal( if (!found) return -error; - return log_dispatch(level, error, file, line, func, NULL, NULL, NULL, NULL, buf + 8); + return log_dispatch_internal(level, error, file, line, func, NULL, NULL, NULL, NULL, buf + 8); } int log_set_target_from_string(const char *e) { diff --git a/src/basic/log.h b/src/basic/log.h index cdb66005d8..b3e4060b5d 100644 --- a/src/basic/log.h +++ b/src/basic/log.h @@ -75,6 +75,18 @@ void log_close_console(void); void log_parse_environment(void); +int log_dispatch_internal( + int level, + int error, + const char *file, + int line, + const char *func, + const char *object_field, + const char *object, + const char *extra, + const char *extra_field, + char *buffer); + int log_internal( int level, int error, @@ -167,6 +179,9 @@ void log_assert_failed_return( int line, const char *func); +#define log_dispatch(level, error, buffer) \ + log_dispatch_internal(level, error, __FILE__, __LINE__, __func__, NULL, NULL, NULL, NULL, buffer) + /* Logging with level */ #define log_full_errno(level, error, ...) \ ({ \ From ae2173d66b8b9fc42f311d8586a3cf4abcc65a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Apr 2017 14:28:19 -0400 Subject: [PATCH 5/5] coredump: fix non-literal string used in printf This was exposed by the previous commit. This could be potentially unpleasant, but we are saved by the fact that this code path was only taken for journald crashes, where we control COMM and know that it doesn't contain any special characters. Use log_dispatch which does not do any format processing to push the message out. --- src/coredump/coredump.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 5828e949e3..e9ada8e6db 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -800,12 +800,11 @@ log: if (journald_crash) { /* We cannot log to the journal, so just print the MESSAGE. * The target was set previously to something safe. */ - log_struct(LOG_ERR, core_message, NULL); + log_dispatch(LOG_ERR, 0, core_message); return 0; } - if (core_message) - IOVEC_SET_STRING(iovec[n_iovec++], core_message); + IOVEC_SET_STRING(iovec[n_iovec++], core_message); if (truncated) IOVEC_SET_STRING(iovec[n_iovec++], "COREDUMP_TRUNCATED=1");