From: Luca Boccassi Date: Wed, 24 Jun 2026 12:41:06 +0000 (+0100) Subject: journal-remote: fix hostname double-free on request_meta() error paths X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=94d49182a0ff3dc328f87c36152227e5ab82a0b2;p=thirdparty%2Fsystemd.git journal-remote: fix hostname double-free on request_meta() error paths request_handler() owns the hostname var and passes it by value to request_meta(), which hands it to source_new(), which stores it in source->importer.name without copying. If build_accept_encoding() then fails, the hostname var is freed, and then the caller's _cleanup_free_ frees it a second time. Follow-up for 9ff48d0982fcb97923955685fe9fa4e0e67cb238 --- diff --git a/src/journal-remote/journal-remote-main.c b/src/journal-remote/journal-remote-main.c index 2100190bda0..95d28ba1a5c 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 4794393d931..00f14c765aa 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 47d3881a5d7..12886f512fd 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;