diff --git a/src/journal-remote/journal-gatewayd.c b/src/journal-remote/journal-gatewayd.c index 9b1085bb4c..bb1f901d9c 100644 --- a/src/journal-remote/journal-gatewayd.c +++ b/src/journal-remote/journal-gatewayd.c @@ -184,9 +184,12 @@ static ssize_t request_reader_entries( m->n_entries <= 0) return MHD_CONTENT_READER_END_OF_STREAM; - if (m->n_skip < 0) + if (m->n_skip < 0) { + /* request_parse_range_skip_and_n_entries() rejects INT64_MIN, so the negation + * below cannot overflow. */ + assert(m->n_skip >= -INT64_MAX); r = sd_journal_previous_skip(m->journal, (uint64_t) -m->n_skip + 1); - else if (m->n_skip > 0) { + } else if (m->n_skip > 0) { r = sd_journal_next_skip(m->journal, (uint64_t) m->n_skip + 1); if (r < 0) { log_error_errno(r, "Failed to skip journal entries: %m"); @@ -194,7 +197,7 @@ static ssize_t request_reader_entries( } /* We skipped beyond the end, make sure entries between the cursor and n_skip offset * from it are not returned. */ - if (r < m->n_skip + 1) { + if ((uint64_t) r < (uint64_t) m->n_skip + 1) { m->n_skip -= r; if (!m->follow) @@ -348,6 +351,11 @@ static int request_parse_range_skip_and_n_entries( r = safe_atoi64(t, &m->n_skip); if (r < 0) return r; + + /* The consumer negates n_skip, and negating INT64_MIN in signed arithmetic is undefined + * behaviour, so reject it up front. */ + if (m->n_skip == INT64_MIN) + return -ERANGE; } p = (colon2 ?: colon) + 1; diff --git a/src/journal-remote/journal-remote-main.c b/src/journal-remote/journal-remote-main.c index 2100190bda..95d28ba1a5 100644 --- a/src/journal-remote/journal-remote-main.c +++ b/src/journal-remote/journal-remote-main.c @@ -221,21 +221,23 @@ static int build_accept_encoding(char **ret) { return 0; } -static int request_meta(void **connection_cls, int fd, char *hostname) { +static int request_meta(void **connection_cls, int fd, char *_hostname) { int r; assert(connection_cls); + /* This takes ownership of the hostname in all cases, including on failure. */ + _cleanup_free_ char *hostname = TAKE_PTR(_hostname); + if (*connection_cls) return 0; /* already assigned. */ Writer *writer; r = journal_remote_get_writer(journal_remote_server_global, hostname, &writer); if (r < 0) - return log_warning_errno(r, "Failed to get writer for source %s: %m", - hostname); + return log_warning_errno(r, "Failed to get writer for source %s: %m", hostname); - _cleanup_(source_freep) RemoteSource *source = source_new(fd, true, hostname, writer); + _cleanup_(source_freep) RemoteSource *source = source_new(fd, true, TAKE_PTR(hostname), writer); if (!source) return log_oom(); @@ -445,13 +447,12 @@ static mhd_result request_handler( assert(hostname); - r = request_meta(connection_cls, fd, hostname); + r = request_meta(connection_cls, fd, TAKE_PTR(hostname)); if (r == -ENOMEM) return respond_oom(connection); else if (r < 0) return mhd_respondf(connection, r, MHD_HTTP_INTERNAL_SERVER_ERROR, "%m"); - hostname = NULL; return MHD_YES; } diff --git a/src/journal-remote/journal-remote-parse.c b/src/journal-remote/journal-remote-parse.c index 4794393d93..00f14c765a 100644 --- a/src/journal-remote/journal-remote-parse.c +++ b/src/journal-remote/journal-remote-parse.c @@ -24,7 +24,8 @@ RemoteSource* source_free(RemoteSource *source) { /** * Initialize zero-filled source with given values. On success, takes - * ownership of fd, name, and writer, otherwise does not touch them. + * ownership of fd and writer, otherwise does not touch them. Always takes + * ownership of name, even on failure. */ RemoteSource* source_new(int fd, bool passive_fd, char *name, Writer *writer) { RemoteSource *source; @@ -35,8 +36,10 @@ RemoteSource* source_new(int fd, bool passive_fd, char *name, Writer *writer) { assert(fd >= 0); source = new0(RemoteSource, 1); - if (!source) + if (!source) { + free(name); return NULL; + } source->importer = JOURNAL_IMPORTER_MAKE(fd); source->importer.passive_fd = passive_fd; diff --git a/src/journal-remote/journal-remote.c b/src/journal-remote/journal-remote.c index 47d3881a5d..12886f512f 100644 --- a/src/journal-remote/journal-remote.c +++ b/src/journal-remote/journal-remote.c @@ -162,11 +162,12 @@ static int dispatch_raw_connection_event(sd_event_source *event, void *userdata); static int get_source_for_fd(RemoteServer *s, - int fd, char *name, RemoteSource **source) { + int fd, char *_name, RemoteSource **source) { + _cleanup_free_ char *name = TAKE_PTR(_name); Writer *writer; int r; - /* This takes ownership of name, but only on success. */ + /* This takes ownership of the name in all cases, including on failure. */ assert(s); assert(fd >= 0); @@ -177,11 +178,10 @@ static int get_source_for_fd(RemoteServer *s, r = journal_remote_get_writer(s, name, &writer); if (r < 0) - return log_warning_errno(r, "Failed to get writer for source %s: %m", - name); + return log_warning_errno(r, "Failed to get writer for source %s: %m", name); if (!s->sources[fd]) { - s->sources[fd] = source_new(fd, false, name, writer); + s->sources[fd] = source_new(fd, false, TAKE_PTR(name), writer); if (!s->sources[fd]) { writer_unref(writer); return log_oom(); @@ -211,29 +211,29 @@ static int remove_source(RemoteServer *s, int fd) { return 0; } -int journal_remote_add_source(RemoteServer *s, int fd, char *name, bool own_name) { +int journal_remote_add_source(RemoteServer *s, int fd, char *_name, bool own_name) { + _cleanup_free_ char *name = NULL; RemoteSource *source = NULL; int r; - /* This takes ownership of name, even on failure, if own_name is true. */ + /* This takes ownership of _name, even on failure, if own_name is true. */ assert(s); assert(fd >= 0); - assert(name); + assert(_name); - if (!own_name) { - name = strdup(name); + if (own_name) + name = TAKE_PTR(_name); + else { + name = strdup(_name); if (!name) return log_oom(); } - r = get_source_for_fd(s, fd, name, &source); - if (r < 0) { - log_error_errno(r, "Failed to create source for fd:%d (%s): %m", - fd, name); - free(name); - return r; - } + /* get_source_for_fd() takes ownership of the name in all cases, so it must not be touched below. */ + r = get_source_for_fd(s, fd, TAKE_PTR(name), &source); + if (r < 0) + return log_error_errno(r, "Failed to create source for fd:%d: %m", fd); r = sd_event_add_io(s->event, &source->event, fd, EPOLLIN|EPOLLRDHUP|EPOLLPRI, @@ -246,7 +246,7 @@ int journal_remote_add_source(RemoteServer *s, int fd, char *name, bool own_name if (r == 0) r = sd_event_source_set_enabled(source->buffer_event, SD_EVENT_OFF); } else if (r == -EPERM) { - log_debug("Falling back to sd_event_add_defer for fd:%d (%s)", fd, name); + log_debug("Falling back to sd_event_add_defer for fd:%d (%s)", fd, source->importer.name); r = sd_event_add_defer(s->event, &source->event, dispatch_blocking_source_event, source); if (r == 0) @@ -258,7 +258,7 @@ int journal_remote_add_source(RemoteServer *s, int fd, char *name, bool own_name goto error; } - r = sd_event_source_set_description(source->event, name); + r = sd_event_source_set_description(source->event, source->importer.name); if (r < 0) { log_error_errno(r, "Failed to set source name for fd:%d: %m", fd); goto error; diff --git a/test/units/TEST-04-JOURNAL.journal-gatewayd.sh b/test/units/TEST-04-JOURNAL.journal-gatewayd.sh index 617976e752..64af25e960 100755 --- a/test/units/TEST-04-JOURNAL.journal-gatewayd.sh +++ b/test/units/TEST-04-JOURNAL.journal-gatewayd.sh @@ -88,6 +88,12 @@ curl -LSfs \ --header "Range: entries=$TEST_CURSOR:1:1" \ http://localhost:19531/entries?SYSLOG_IDENTIFIER="$TEST_TAG" >"$LOG_FILE" jq -se "length == 0" "$LOG_FILE" +# A skip of INT64_MIN must not crash the daemon +curl -LSs \ + --header "Accept: application/json" \ + --header "Range: entries=:-9223372036854775808:1" \ + http://localhost:19531/entries >/dev/null || true +curl -LSfs http://localhost:19531/entries >"$LOG_FILE" # Check if the specified cursor refers to an existing entry and return just that entry curl -LSfs \ --header "Accept: application/json" \