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 2100190bda035e0c84d36bdccf427e9e7e1c57c8..95d28ba1a5c82d39a49c8afa2bd94efbdab84b6f 100644 (file)
--- 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 4794393d93165fa7a428aa8639d267c261d40930..00f14c765aae12ab7f58d0aef6f91a20fe438852 100644 (file)
--- 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 47d3881a5d7540479e0445406fd65e619d0eb56b..12886f512fd9bf6e79c3e55bc8f041b3dc32271e 100644 (file)
--- 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;