From 731e10f3c588a31936454d3207155c497219d99a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 13 May 2017 22:42:08 -0400 Subject: [PATCH 1/6] journald: use unaligned_read instead of memcpy --- src/journal/journald-native.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index c9bf3832c7..d839e04488 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -40,6 +40,7 @@ #include "selinux-util.h" #include "socket-util.h" #include "string-util.h" +#include "unaligned.h" bool valid_user_field(const char *p, size_t l, bool allow_protected) { const char *a; @@ -218,7 +219,6 @@ void server_process_native_message( p = e + 1; continue; } else { - le64_t l_le; uint64_t l; char *k; @@ -227,8 +227,7 @@ void server_process_native_message( break; } - memcpy(&l_le, e + 1, sizeof(uint64_t)); - l = le64toh(l_le); + l = unaligned_read_le64(e + 1); if (l > DATA_SIZE_MAX) { log_debug("Received binary data block of %"PRIu64" bytes is too large, ignoring.", l); From 2ddb70d26f9a284fbf38bca37ea857f24c3f39de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 13 May 2017 22:42:14 -0400 Subject: [PATCH 2/6] journal-remote: fix memleak of the name of the remote source --- src/basic/journal-importer.c | 1 + src/journal-remote/journal-remote-parse.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/journal-importer.c b/src/basic/journal-importer.c index 66119d2de1..7d72effdea 100644 --- a/src/basic/journal-importer.c +++ b/src/basic/journal-importer.c @@ -69,6 +69,7 @@ void journal_importer_cleanup(JournalImporter *imp) { safe_close(imp->fd); } + free(imp->name); free(imp->buf); iovw_free_contents(&imp->iovw); } diff --git a/src/journal-remote/journal-remote-parse.c b/src/journal-remote/journal-remote-parse.c index 79afe6604c..d61d1c18f6 100644 --- a/src/journal-remote/journal-remote-parse.c +++ b/src/journal-remote/journal-remote-parse.c @@ -41,7 +41,7 @@ void source_free(RemoteSource *source) { /** * Initialize zero-filled source with given values. On success, takes - * ownerhship of fd and writer, otherwise does not touch them. + * ownership of fd, name, and writer, otherwise does not touch them. */ RemoteSource* source_new(int fd, bool passive_fd, char *name, Writer *writer) { From 8924973ae2e1f0a0c131dcec0578669dc26e5e26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 13 May 2017 16:40:09 -0400 Subject: [PATCH 3/6] shared/logs-show: avoid printing "(null)" when timestamp is not specified $ perl -e 'print("MESSAGE\n", pack("q<", 1), "A\n\nMESSAGE=test2\n")' > message.bin $ systemd-journal-remote -o /tmp/out.journal message.bin $ journalctl -o export --file /tmp/out.journal __CURSOR=s=b16c464c2db44384b29e75a564d8388e;i=1;b=6b0be47627bd4932913dc126012c21c0;m=0;t=0;x=b04263a253e357a __REALTIME_TIMESTAMP=0 __MONOTONIC_TIMESTAMP=0 _BOOT_ID=6b0be47627bd4932913dc126012c21c0 MESSAGE=A $ journalctl -o verbose --file /tmp/out.journal (null) [s=b16c464c2db44384b29e75a564d8388e;i=1;b=6b0be47627bd4932913dc126012c21c0;m=0;t=0;x=b04263a253e357a] MESSAGE=A This is changed to $ build/journalctl -o verbose --file /tmp/out.journal (no timestamp) [s=b16c464c2db44384b29e75a564d8388e;i=1;b=6b0be47627bd4932913dc126012c21c0;m=0;t=0;x=b04263a253e357a] MESSAGE=A We should deal gracefully with unexpected input. --- src/shared/logs-show.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/shared/logs-show.c b/src/shared/logs-show.c index a8c15d93b6..c735fa159d 100644 --- a/src/shared/logs-show.c +++ b/src/shared/logs-show.c @@ -485,6 +485,7 @@ static int output_verbose( _cleanup_free_ char *cursor = NULL; uint64_t realtime = 0; char ts[FORMAT_TIMESTAMP_MAX + 7]; + const char *timestamp; int r; assert(f); @@ -520,10 +521,10 @@ static int output_verbose( if (r < 0) return log_error_errno(r, "Failed to get cursor: %m"); + timestamp = flags & OUTPUT_UTC ? format_timestamp_us_utc(ts, sizeof ts, realtime) + : format_timestamp_us(ts, sizeof ts, realtime); fprintf(f, "%s [%s]\n", - flags & OUTPUT_UTC ? - format_timestamp_us_utc(ts, sizeof(ts), realtime) : - format_timestamp_us(ts, sizeof(ts), realtime), + timestamp ?: "(no timestamp)", cursor); JOURNAL_FOREACH_DATA_RETVAL(j, data, length, r) { From 68944f196bc85b067de71c4fe1631d824d0aded5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 13 May 2017 22:34:40 -0400 Subject: [PATCH 4/6] journald: properly process multiple entries in the same native packet For all except the last entry in a single packet, we would dispatch the message to the journal, but not forward it, nor perform proper cleanup. Rewrite the code to process each entry in a helper function, and make server_process_native_message() just call this function in a loop. Fixes #5643. v2: - properly decrement *remaining when processing entry separator --- src/journal/journald-native.c | 83 +++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 34 deletions(-) diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index d839e04488..83250c34e1 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -81,60 +81,52 @@ static bool allow_object_pid(const struct ucred *ucred) { return ucred && ucred->uid == 0; } -void server_process_native_message( +static int server_process_entry( Server *s, - const void *buffer, size_t buffer_size, + const void *buffer, size_t *remaining, const struct ucred *ucred, const struct timeval *tv, const char *label, size_t label_len) { + /* Process a single entry from a native message. + * Returns 0 if nothing special happened and the message processing should continue, + * and a negative or positive value otherwise. + * + * Note that *remaining is altered on both success and failure. */ + struct iovec *iovec = NULL; unsigned n = 0, j, tn = (unsigned) -1; const char *p; - size_t remaining, m = 0, entry_size = 0; + size_t m = 0, entry_size = 0; int priority = LOG_INFO; char *identifier = NULL, *message = NULL; pid_t object_pid = 0; - - assert(s); - assert(buffer || buffer_size == 0); + int r = 0; p = buffer; - remaining = buffer_size; - while (remaining > 0) { + while (*remaining > 0) { const char *e, *q; - e = memchr(p, '\n', remaining); + e = memchr(p, '\n', *remaining); if (!e) { /* Trailing noise, let's ignore it, and flush what we collected */ log_debug("Received message with trailing noise, ignoring."); + r = 1; /* finish processing of the message */ break; } if (e == p) { /* Entry separator */ - - if (entry_size + n + 1 > ENTRY_SIZE_MAX) { /* data + separators + trailer */ - log_debug("Entry is too big with %u properties and %zu bytes, ignoring.", n, entry_size); - continue; - } - - server_dispatch_message(s, iovec, n, m, ucred, tv, label, label_len, NULL, priority, object_pid); - n = 0; - priority = LOG_INFO; - entry_size = 0; - - p++; - remaining--; - continue; + *remaining -= 1; + break; } if (*p == '.' || *p == '#') { /* Ignore control commands for now, and * comments too. */ - remaining -= (e - p) + 1; + *remaining -= (e - p) + 1; p = e + 1; continue; } @@ -143,7 +135,7 @@ void server_process_native_message( /* n existing properties, 1 new, +1 for _TRANSPORT */ if (!GREEDY_REALLOC(iovec, m, n + 2 + N_IOVEC_META_FIELDS + N_IOVEC_OBJECT_FIELDS)) { - log_oom(); + r = log_oom(); break; } @@ -160,7 +152,7 @@ void server_process_native_message( * field */ iovec[n].iov_base = (char*) p; iovec[n].iov_len = l; - entry_size += iovec[n].iov_len; + entry_size += l; n++; /* We need to determine the priority @@ -210,19 +202,18 @@ void server_process_native_message( memcpy(buf, p + strlen("OBJECT_PID="), l - strlen("OBJECT_PID=")); buf[l-strlen("OBJECT_PID=")] = '\0'; - /* ignore error */ - parse_pid(buf, &object_pid); + (void) parse_pid(buf, &object_pid); } } - remaining -= (e - p) + 1; + *remaining -= (e - p) + 1; p = e + 1; continue; } else { uint64_t l; char *k; - if (remaining < e - p + 1 + sizeof(uint64_t) + 1) { + if (*remaining < e - p + 1 + sizeof(uint64_t) + 1) { log_debug("Failed to parse message, ignoring."); break; } @@ -234,7 +225,7 @@ void server_process_native_message( break; } - if ((uint64_t) remaining < e - p + 1 + sizeof(uint64_t) + l + 1 || + if ((uint64_t) *remaining < e - p + 1 + sizeof(uint64_t) + l + 1 || e[1+sizeof(uint64_t)+l] != '\n') { log_debug("Failed to parse message, ignoring."); break; @@ -258,13 +249,15 @@ void server_process_native_message( } else free(k); - remaining -= (e - p) + 1 + sizeof(uint64_t) + l + 1; + *remaining -= (e - p) + 1 + sizeof(uint64_t) + l + 1; p = e + 1 + sizeof(uint64_t) + l + 1; } } - if (n <= 0) + if (n <= 0) { + r = 1; goto finish; + } tn = n++; IOVEC_SET_STRING(iovec[tn], "_TRANSPORT=journal"); @@ -298,13 +291,35 @@ finish: continue; if (iovec[j].iov_base < buffer || - (const uint8_t*) iovec[j].iov_base >= (const uint8_t*) buffer + buffer_size) + (const char*) iovec[j].iov_base >= p + *remaining) free(iovec[j].iov_base); } free(iovec); free(identifier); free(message); + + return r; +} + +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) { + + int r; + size_t remaining = buffer_size; + + assert(s); + assert(buffer || buffer_size == 0); + + do { + r = server_process_entry(s, + (const uint8_t*) buffer + (buffer_size - remaining), &remaining, + ucred, tv, label, label_len); + } while (r == 0); } void server_process_native_file( From 4b29a7f41fa1f418520d66362fd904a504f67c02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 May 2017 11:59:47 -0400 Subject: [PATCH 5/6] journald: process "binary" fields the same as text fields MESSAGE=data\n and MESSAGE\n40000000data\n are both valid serializations, so they should be stored in the journal. Before, MESSAGE, SYSLOG_FACILITY, SYSLOG_IDENTIFIER, PRIORITY, and OBJECT_PID would be only honoured if they were given in the first form. Fixed #5973. --- src/journal/journald-native.c | 120 ++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 50 deletions(-) diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index 83250c34e1..db3fdcf1df 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -81,6 +81,64 @@ static bool allow_object_pid(const struct ucred *ucred) { return ucred && ucred->uid == 0; } +static void server_process_entry_meta( + const char *p, size_t l, + const struct ucred *ucred, + int *priority, + char **identifier, + char **message, + pid_t *object_pid) { + + /* We need to determine the priority of this entry for the rate limiting logic */ + + if (l == 10 && + startswith(p, "PRIORITY=") && + p[9] >= '0' && p[9] <= '9') + *priority = (*priority & LOG_FACMASK) | (p[9] - '0'); + + else if (l == 17 && + startswith(p, "SYSLOG_FACILITY=") && + p[16] >= '0' && p[16] <= '9') + *priority = (*priority & LOG_PRIMASK) | ((p[16] - '0') << 3); + + else if (l == 18 && + startswith(p, "SYSLOG_FACILITY=") && + p[16] >= '0' && p[16] <= '9' && + p[17] >= '0' && p[17] <= '9') + *priority = (*priority & LOG_PRIMASK) | (((p[16] - '0')*10 + (p[17] - '0')) << 3); + + else if (l >= 19 && + startswith(p, "SYSLOG_IDENTIFIER=")) { + char *t; + + t = strndup(p + 18, l - 18); + if (t) { + free(*identifier); + *identifier = t; + } + + } else if (l >= 8 && + startswith(p, "MESSAGE=")) { + char *t; + + t = strndup(p + 8, l - 8); + if (t) { + free(*message); + *message = t; + } + + } else if (l > strlen("OBJECT_PID=") && + l < strlen("OBJECT_PID=") + DECIMAL_STR_MAX(pid_t) && + startswith(p, "OBJECT_PID=") && + allow_object_pid(ucred)) { + char buf[DECIMAL_STR_MAX(pid_t)]; + memcpy(buf, p + strlen("OBJECT_PID="), l - strlen("OBJECT_PID=")); + buf[l-strlen("OBJECT_PID=")] = '\0'; + + (void) parse_pid(buf, object_pid); + } +} + static int server_process_entry( Server *s, const void *buffer, size_t *remaining, @@ -148,62 +206,18 @@ static int server_process_entry( /* If the field name starts with an * underscore, skip the variable, - * since that indidates a trusted + * since that indicates a trusted * field */ iovec[n].iov_base = (char*) p; iovec[n].iov_len = l; entry_size += l; n++; - /* We need to determine the priority - * of this entry for the rate limiting - * logic */ - if (l == 10 && - startswith(p, "PRIORITY=") && - p[9] >= '0' && p[9] <= '9') - priority = (priority & LOG_FACMASK) | (p[9] - '0'); - - else if (l == 17 && - startswith(p, "SYSLOG_FACILITY=") && - p[16] >= '0' && p[16] <= '9') - priority = (priority & LOG_PRIMASK) | ((p[16] - '0') << 3); - - else if (l == 18 && - startswith(p, "SYSLOG_FACILITY=") && - p[16] >= '0' && p[16] <= '9' && - p[17] >= '0' && p[17] <= '9') - priority = (priority & LOG_PRIMASK) | (((p[16] - '0')*10 + (p[17] - '0')) << 3); - - else if (l >= 19 && - startswith(p, "SYSLOG_IDENTIFIER=")) { - char *t; - - t = strndup(p + 18, l - 18); - if (t) { - free(identifier); - identifier = t; - } - - } else if (l >= 8 && - startswith(p, "MESSAGE=")) { - char *t; - - t = strndup(p + 8, l - 8); - if (t) { - free(message); - message = t; - } - - } else if (l > strlen("OBJECT_PID=") && - l < strlen("OBJECT_PID=") + DECIMAL_STR_MAX(pid_t) && - startswith(p, "OBJECT_PID=") && - allow_object_pid(ucred)) { - char buf[DECIMAL_STR_MAX(pid_t)]; - memcpy(buf, p + strlen("OBJECT_PID="), l - strlen("OBJECT_PID=")); - buf[l-strlen("OBJECT_PID=")] = '\0'; - - (void) parse_pid(buf, &object_pid); - } + server_process_entry_meta(p, l, ucred, + &priority, + &identifier, + &message, + &object_pid); } *remaining -= (e - p) + 1; @@ -246,6 +260,12 @@ static int server_process_entry( iovec[n].iov_len = (e - p) + 1 + l; entry_size += iovec[n].iov_len; n++; + + server_process_entry_meta(k, (e - p) + 1 + l, ucred, + &priority, + &identifier, + &message, + &object_pid); } else free(k); From c6e9e16f7702188127bf8dfbac45a87f8d1a3512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 19 May 2017 12:52:05 -0400 Subject: [PATCH 6/6] journald: fix trivial memleak Fixes #5516. --- src/journal/journald-server.c | 2 ++ src/journal/journald-server.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index 667dfa00ff..08289200c5 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -2177,6 +2177,8 @@ void server_done(Server *s) { free(s->tty_path); free(s->cgroup_root); free(s->hostname_field); + free(s->runtime_storage.path); + free(s->system_storage.path); if (s->mmap) mmap_cache_unref(s->mmap); diff --git a/src/journal/journald-server.h b/src/journal/journald-server.h index 75ac114d24..203460c50a 100644 --- a/src/journal/journald-server.h +++ b/src/journal/journald-server.h @@ -61,7 +61,7 @@ typedef struct JournalStorageSpace { typedef struct JournalStorage { const char *name; - const char *path; + char *path; JournalMetrics metrics; JournalStorageSpace space;