]> git.ipfire.org Git - people/ms/pakfire.git/commitdiff
jail: Implement better logging for the child process
authorMichael Tremer <michael.tremer@ipfire.org>
Thu, 4 Aug 2022 15:18:54 +0000 (15:18 +0000)
committerMichael Tremer <michael.tremer@ipfire.org>
Thu, 4 Aug 2022 15:18:54 +0000 (15:18 +0000)
The child process used to dump any logging to the standard output/error
which is not useful when we want to collect any actual output of the
process that was called.

This patch adds extra pipes (one for each log level - I know) and passes
those log messages on to the regular logger (past the jail log
callback).

Signed-off-by: Michael Tremer <michael.tremer@ipfire.org>
src/libpakfire/jail.c

index 758443383f1dad49e212d5e74e7f5b89b3e231df..2d1778d31427f79aa12412c5b87acf7ac4df0ec6 100644 (file)
@@ -96,15 +96,25 @@ struct pakfire_jail_exec {
        int completed_fd;
 
        // Log pipes
-       struct {
+       struct pakfire_jail_pipes {
                int stdout[2];
                int stderr[2];
+
+               // Logging
+               int log_INFO[2];
+               int log_ERROR[2];
+               int log_DEBUG[2];
        } pipes;
 
        // Log buffers
-       struct {
+       struct pakfire_jail_buffers {
                struct pakfire_log_buffer stdout;
                struct pakfire_log_buffer stderr;
+
+               // Logging
+               struct pakfire_log_buffer log_INFO;
+               struct pakfire_log_buffer log_ERROR;
+               struct pakfire_log_buffer log_DEBUG;
        } buffers;
 };
 
@@ -123,6 +133,9 @@ static void pakfire_jail_free(struct pakfire_jail* jail) {
        free(jail);
 }
 
+/*
+       Passes any log messages on to the default pakfire log callback
+*/
 static int pakfire_jail_default_log_callback(struct pakfire* pakfire, void* data,
                int priority, const char* line, size_t length) {
        switch (priority) {
@@ -133,6 +146,12 @@ static int pakfire_jail_default_log_callback(struct pakfire* pakfire, void* data
                case LOG_ERR:
                        ERROR(pakfire, "%s", line);
                        break;
+
+#ifdef ENABLE_DEBUG
+               case LOG_DEBUG:
+                       DEBUG(pakfire, "%s", line);
+                       break;
+#endif
        }
 
        return 0;
@@ -354,6 +373,41 @@ PAKFIRE_EXPORT int pakfire_jail_set_log_callback(struct pakfire_jail* jail,
        return 0;
 }
 
+/*
+       This function replaces any logging in the child process.
+
+       All log messages will be sent to the parent process through their respective pipes.
+*/
+static void pakfire_jail_log(void* data, int priority, const char* file,
+               int line, const char* fn, const char* format, va_list args) {
+       struct pakfire_jail_pipes* pipes = (struct pakfire_jail_pipes*)data;
+       int fd;
+
+       switch (priority) {
+               case LOG_INFO:
+                       fd = pipes->log_INFO[1];
+                       break;
+
+               case LOG_ERR:
+                       fd = pipes->log_ERROR[1];
+                       break;
+
+#ifdef ENABLE_DEBUG
+               case LOG_DEBUG:
+                       fd = pipes->log_DEBUG[1];
+                       break;
+#endif /* ENABLE_DEBUG */
+
+               // Ignore any messages of an unknown priority
+               default:
+                       return;
+       }
+
+       // Send the log message
+       if (fd)
+               vdprintf(fd, format, args);
+}
+
 static int pakfire_jail_log_buffer_is_full(const struct pakfire_log_buffer* buffer) {
        return (sizeof(buffer->data) == buffer->used);
 }
@@ -364,7 +418,8 @@ static int pakfire_jail_log_buffer_is_full(const struct pakfire_log_buffer* buff
        If not newline character is found, it will try to read more data until it finds one.
 */
 static int pakfire_jail_handle_log(struct pakfire_jail* jail,
-                       struct pakfire_jail_exec* ctx, int priority, int fd, struct pakfire_log_buffer* buffer) {
+               struct pakfire_jail_exec* ctx, int priority, int fd,
+               struct pakfire_log_buffer* buffer, pakfire_jail_log_callback callback, void* data) {
        char line[BUFFER_SIZE + 1];
 
        // Fill up buffer from fd
@@ -411,8 +466,8 @@ static int pakfire_jail_handle_log(struct pakfire_jail* jail,
                line[length] = '\0';
 
                // Log the line
-               if (jail->log_callback) {
-                       int r = jail->log_callback(jail->pakfire, jail->log_data, priority, line, length);
+               if (callback) {
+                       int r = callback(jail->pakfire, data, priority, line, length);
                        if (r) {
                                ERROR(jail->pakfire, "The logging callback returned an error: %d\n", r);
                                return r;
@@ -427,6 +482,25 @@ static int pakfire_jail_handle_log(struct pakfire_jail* jail,
        return 0;
 }
 
+/*
+       This is a convenience function to fetch the reading end of a pipe and
+       closes the write end.
+*/
+static int pakfire_jail_get_pipe(struct pakfire_jail* jail, int (*fds)[2]) {
+       // Give the variables easier names to avoid confusion
+       int* fd_read  = &(*fds)[0];
+       int* fd_write = &(*fds)[1];
+
+       // Close the write end of the pipe
+       if (*fd_write) {
+               close(*fd_write);
+               *fd_write = 0;
+       }
+
+       // Return the read end
+       return *fd_read;
+}
+
 static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec* ctx) {
        int epollfd = -1;
        struct epoll_event ev;
@@ -434,23 +508,18 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec
        int r = 0;
 
        // Fetch file descriptors from context
-       const int stdout = ctx->pipes.stdout[0];
-       const int stderr = ctx->pipes.stderr[0];
+       const int stdout = pakfire_jail_get_pipe(jail, &ctx->pipes.stdout);
+       const int stderr = pakfire_jail_get_pipe(jail, &ctx->pipes.stderr);
        const int pidfd  = ctx->pidfd;
 
-       // Close any unused file descriptors
-       if (ctx->pipes.stdout[1]) {
-               close(ctx->pipes.stdout[1]);
-               ctx->pipes.stdout[1] = 0;
-       }
-       if (ctx->pipes.stderr[1]) {
-               close(ctx->pipes.stderr[1]);
-               ctx->pipes.stderr[1] = 0;
-       }
+       // Logging
+       const int log_INFO  = pakfire_jail_get_pipe(jail, &ctx->pipes.log_INFO);
+       const int log_ERROR = pakfire_jail_get_pipe(jail, &ctx->pipes.log_ERROR);
+       const int log_DEBUG = pakfire_jail_get_pipe(jail, &ctx->pipes.log_DEBUG);
 
        // Make a list of all file descriptors we are interested in
        int fds[] = {
-               stdout, stderr, pidfd,
+               stdout, stderr, pidfd, log_INFO, log_ERROR, log_DEBUG,
        };
 
        // Setup epoll
@@ -496,12 +565,14 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec
                        goto ERROR;
                }
 
-               struct pakfire_log_buffer* buffer;
-               int priority;
-
                for (int i = 0; i < num; i++) {
                        int fd = events[i].data.fd;
 
+                       struct pakfire_log_buffer* buffer = NULL;
+                       pakfire_jail_log_callback callback = NULL;
+                       void* data = NULL;
+                       int priority;
+
                        // Handle any changes to the PIDFD
                        if (fd == pidfd) {
                                // Call waidid() and store the result
@@ -516,22 +587,47 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec
                                ended = 1;
                                continue;
 
+                       // Handle logging messages
+                       } else if (fd == log_INFO) {
+                               buffer = &ctx->buffers.log_INFO;
+                               priority = LOG_INFO;
+
+                               callback = pakfire_jail_default_log_callback;
+
+                       } else if (fd == log_ERROR) {
+                               buffer = &ctx->buffers.log_ERROR;
+                               priority = LOG_ERR;
+
+                               callback = pakfire_jail_default_log_callback;
+
+                       } else if (fd == log_DEBUG) {
+                               buffer = &ctx->buffers.log_DEBUG;
+                               priority = LOG_DEBUG;
+
+                               callback = pakfire_jail_default_log_callback;
+
                        // Handle anything from the log pipes
                        } else if (fd == stdout) {
                                buffer = &ctx->buffers.stdout;
                                priority = LOG_INFO;
 
+                               callback = jail->log_callback;
+                               data = jail->log_data;
+
                        } else if (fd == stderr) {
                                buffer = &ctx->buffers.stderr;
                                priority = LOG_ERR;
 
+                               callback = jail->log_callback;
+                               data = jail->log_data;
+
                        } else {
                                DEBUG(jail->pakfire, "Received invalid file descriptor %d\n", fd);
                                continue;
                        }
 
                        // Handle log event
-                       r = pakfire_jail_handle_log(jail, ctx, priority, fd, buffer);
+                       r = pakfire_jail_handle_log(jail, ctx, priority, fd, buffer, callback, data);
                        if (r)
                                goto ERROR;
                }
@@ -955,7 +1051,8 @@ static int pakfire_jail_child(struct pakfire_jail* jail, struct pakfire_jail_exe
                const char* argv[]) {
        int r;
 
-       // XXX do we have to reconfigure logging here?
+       // Redirect any logging to our log pipe
+       pakfire_set_log_callback(jail->pakfire, pakfire_jail_log, &ctx->pipes);
 
        // Fetch my own PID
        pid_t pid = getpid();
@@ -1035,6 +1132,13 @@ static int pakfire_jail_child(struct pakfire_jail* jail, struct pakfire_jail_exe
                }
        }
 
+       // Close other end of log pipes
+       close(ctx->pipes.log_INFO[0]);
+       close(ctx->pipes.log_ERROR[0]);
+#ifdef ENABLE_DEBUG
+       close(ctx->pipes.log_DEBUG[0]);
+#endif /* ENABLE_DEBUG */
+
        // Connect standard output and error
        if (ctx->pipes.stdout[1] && ctx->pipes.stderr[1]) {
                r = dup2(ctx->pipes.stdout[1], STDOUT_FILENO);
@@ -1092,6 +1196,22 @@ static int pakfire_jail_child(struct pakfire_jail* jail, struct pakfire_jail_exe
        return r;
 }
 
+static int pakfire_jail_setup_pipe(struct pakfire_jail* jail, int (*fds)[2], const int flags) {
+       int r = pipe2(*fds, flags);
+       if (r < 0) {
+               ERROR(jail->pakfire, "Could not setup pipe: %m\n");
+               return 1;
+       }
+
+       return 0;
+}
+
+static void pakfire_jail_close_pipe(struct pakfire_jail* jail, int fds[2]) {
+       for (unsigned int i = 0; i < 2; i++)
+               if (fds[i])
+                       close(fds[i]);
+}
+
 // Run a command in the jail
 static int __pakfire_jail_exec(struct pakfire_jail* jail, const char* argv[]) {
        int exit = -1;
@@ -1106,8 +1226,8 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, const char* argv[]) {
        // Initialize context for this call
        struct pakfire_jail_exec ctx = {
                .pipes = {
-                       .stdout = { 0, 0, },
-                       .stderr = { 0, 0, },
+                       .stdout   = { 0, 0 },
+                       .stderr   = { 0, 0 },
                },
        };
 
@@ -1126,20 +1246,34 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, const char* argv[]) {
        // Create pipes to communicate with child process if we are not running interactively
        if (!pakfire_jail_has_flag(jail, PAKFIRE_JAIL_INTERACTIVE)) {
                // stdout
-               r = pipe(ctx.pipes.stdout);
-               if (r < 0) {
-                       ERROR(jail->pakfire, "Could not create file descriptors for stdout: %m\n");
+               r = pakfire_jail_setup_pipe(jail, &ctx.pipes.stdout, 0);
+               if (r)
                        goto ERROR;
-               }
 
                // stderr
-               r = pipe(ctx.pipes.stderr);
-               if (r < 0) {
-                       ERROR(jail->pakfire, "Could not create file descriptors for stderr: %m\n");
+               r = pakfire_jail_setup_pipe(jail, &ctx.pipes.stderr, 0);
+               if (r)
                        goto ERROR;
-               }
        }
 
+       // Setup pipes for logging
+       // INFO
+       r = pakfire_jail_setup_pipe(jail, &ctx.pipes.log_INFO, O_CLOEXEC);
+       if (r)
+               goto ERROR;
+
+       // ERROR
+       r = pakfire_jail_setup_pipe(jail, &ctx.pipes.log_ERROR, O_CLOEXEC);
+       if (r)
+               goto ERROR;
+
+#ifdef ENABLE_DEBUG
+       // DEBUG
+       r = pakfire_jail_setup_pipe(jail, &ctx.pipes.log_DEBUG, O_CLOEXEC);
+       if (r)
+               goto ERROR;
+#endif /* ENABLE_DEBUG */
+
        // Configure child process
        struct clone_args args = {
                .flags =
@@ -1201,16 +1335,13 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, const char* argv[]) {
 
 ERROR:
        // Close any file descriptors
-       if (ctx.pipes.stdout[0])
-               close(ctx.pipes.stdout[0]);
-       if (ctx.pipes.stdout[1])
-               close(ctx.pipes.stdout[1]);
-       if (ctx.pipes.stderr[0])
-               close(ctx.pipes.stderr[0]);
-       if (ctx.pipes.stderr[1])
-               close(ctx.pipes.stderr[1]);
+       pakfire_jail_close_pipe(jail, ctx.pipes.stdout);
+       pakfire_jail_close_pipe(jail, ctx.pipes.stderr);
        if (ctx.pidfd)
                close(ctx.pidfd);
+       pakfire_jail_close_pipe(jail, ctx.pipes.log_INFO);
+       pakfire_jail_close_pipe(jail, ctx.pipes.log_ERROR);
+       pakfire_jail_close_pipe(jail, ctx.pipes.log_DEBUG);
 
        // Umount everything
        if (!pakfire_on_root(jail->pakfire))