From: Michael Tremer Date: Thu, 15 Dec 2022 16:41:12 +0000 (+0000) Subject: jail: Avoid deadlock when sending data to stdin X-Git-Tag: 0.9.29~417 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=06b864ae930adc8eb6fba5317830a7a10e817969;p=pakfire.git jail: Avoid deadlock when sending data to stdin It could happen that we filled up the buffer the input pipe and the jail communication got deadlocked. This is now being solved by using the event loop to find out when we are ready to send more data. This required some changes into how the callbacks behave. Signed-off-by: Michael Tremer --- diff --git a/src/libpakfire/build.c b/src/libpakfire/build.c index a26e2499c..bc1929cc7 100644 --- a/src/libpakfire/build.c +++ b/src/libpakfire/build.c @@ -261,36 +261,56 @@ static int pakfire_build_run_script( struct pakfire_find_deps_ctx { struct pakfire_package* pkg; int dep; - struct pakfire_filelist* filelist; struct pakfire_scriptlet* scriptlet; const char* pattern; + + struct pakfire_filelist* filelist; + unsigned int i; }; -static int pakfire_build_send_file(struct pakfire* pakfire, - struct pakfire_file* file, void* data) { - int* fd = (int*)data; - int r; +static int pakfire_build_send_filelist(struct pakfire* pakfire, void* data, int fd) { + struct pakfire_find_deps_ctx* ctx = (struct pakfire_find_deps_ctx*)data; + struct pakfire_file* file = NULL; + int r = 0; + + const size_t length = pakfire_filelist_size(ctx->filelist); + + // Check if we have reached the end of the filelist + if (ctx->i >= length) + return EOF; + + // Fetch the next file + file = pakfire_filelist_get(ctx->filelist, ctx->i); + if (!file) { + DEBUG(pakfire, "Could not fetch file %d: %m\n", ctx->i); + r = 1; + goto ERROR; + } // Fetch the path of the file const char* path = pakfire_file_get_path(file); if (!path) { ERROR(pakfire, "Received a file with an empty path\n"); - return 1; + r = 1; + goto ERROR; } // Write path to stdin - r = dprintf(*fd, "%s\n", path); + r = dprintf(fd, "%s\n", path); if (r < 0) return r; - return 0; -} + // Move on to the next file + ctx->i++; -static int pakfire_build_send_filelist(struct pakfire* pakfire, void* data, int fd) { - const struct pakfire_find_deps_ctx* ctx = (struct pakfire_find_deps_ctx*)data; + // Success + r = 0; - return pakfire_filelist_walk(ctx->filelist, ctx->pattern, - pakfire_build_send_file, &fd); +ERROR: + if (file) + pakfire_file_unref(file); + + return r; } static int pakfire_build_process_deps(struct pakfire* pakfire, @@ -341,8 +361,11 @@ static int pakfire_build_find_deps(struct pakfire_build* build, struct pakfire_find_deps_ctx ctx = { .pkg = pkg, .dep = dep, - .filelist = filelist, .pattern = pattern, + + // Filelist + .filelist = filelist, + .i = 0, }; int r; diff --git a/src/libpakfire/jail.c b/src/libpakfire/jail.c index 4e5fce4cd..c53dce035 100644 --- a/src/libpakfire/jail.c +++ b/src/libpakfire/jail.c @@ -533,6 +533,34 @@ static int pakfire_jail_handle_log(struct pakfire_jail* jail, return 0; } +static int pakfire_jail_stream_stdin(struct pakfire_jail* jail, + struct pakfire_jail_exec* ctx, const int fd) { + int r; + + // Nothing to do if there is no stdin callback set + if (!ctx->communicate.in) { + DEBUG(jail->pakfire, "Callback for standard input is not set\n"); + return 0; + } + + DEBUG(jail->pakfire, "Streaming standard input...\n"); + + // Calling the callback + r = ctx->communicate.in(jail->pakfire, ctx->communicate.data, fd); + + DEBUG(jail->pakfire, "Standard input callback finished: %d\n", r); + + // The callback signaled that it has written everything + if (r == EOF) { + DEBUG(jail->pakfire, "Closing standard input pipe\n"); + + close(fd); + r = 0; + } + + 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) { @@ -553,7 +581,7 @@ static void pakfire_jail_close_pipe(struct pakfire_jail* jail, int fds[2]) { 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]) { +static int pakfire_jail_get_pipe_to_read(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]; @@ -568,6 +596,21 @@ static int pakfire_jail_get_pipe(struct pakfire_jail* jail, int (*fds)[2]) { return *fd_read; } +static int pakfire_jail_get_pipe_to_write(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 read end of the pipe + if (*fd_read) { + close(*fd_read); + *fd_read = 0; + } + + // Return the write end + return *fd_write; +} + static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec* ctx) { int epollfd = -1; struct epoll_event ev; @@ -575,18 +618,19 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec int r = 0; // Fetch file descriptors from context - const int stdout = pakfire_jail_get_pipe(jail, &ctx->pipes.stdout); - const int stderr = pakfire_jail_get_pipe(jail, &ctx->pipes.stderr); + const int stdin = pakfire_jail_get_pipe_to_write(jail, &ctx->pipes.stdin); + const int stdout = pakfire_jail_get_pipe_to_read(jail, &ctx->pipes.stdout); + const int stderr = pakfire_jail_get_pipe_to_read(jail, &ctx->pipes.stderr); const int pidfd = ctx->pidfd; // 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); + const int log_INFO = pakfire_jail_get_pipe_to_read(jail, &ctx->pipes.log_INFO); + const int log_ERROR = pakfire_jail_get_pipe_to_read(jail, &ctx->pipes.log_ERROR); + const int log_DEBUG = pakfire_jail_get_pipe_to_read(jail, &ctx->pipes.log_DEBUG); // Make a list of all file descriptors we are interested in int fds[] = { - stdout, stderr, pidfd, log_INFO, log_ERROR, log_DEBUG, + stdin, stdout, stderr, pidfd, log_INFO, log_ERROR, log_DEBUG, }; // Setup epoll @@ -597,7 +641,7 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec goto ERROR; } - ev.events = EPOLLIN|EPOLLHUP; + ev.events = EPOLLIN|EPOLLOUT|EPOLLHUP; // Turn file descriptors into non-blocking mode and add them to epoll() for (unsigned int i = 0; i < sizeof(fds) / sizeof(*fds); i++) { @@ -607,6 +651,17 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec if (fd <= 0) continue; + // Read flags + int flags = fcntl(fd, F_GETFL, 0); + + // Set modified flags + if (fcntl(fd, F_SETFL, flags|O_NONBLOCK) < 0) { + ERROR(jail->pakfire, + "Could not set file descriptor %d into non-blocking mode: %m\n", fd); + r = 1; + goto ERROR; + } + ev.data.fd = fd; if (epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev) < 0) { @@ -702,6 +757,24 @@ static int pakfire_jail_wait(struct pakfire_jail* jail, struct pakfire_jail_exec goto ERROR; } + if (e & EPOLLOUT) { + // Handle standard input + if (fd == stdin) { + r = pakfire_jail_stream_stdin(jail, ctx, fd); + if (r) { + switch (errno) { + // Ignore if we filled up the buffer + case EAGAIN: + break; + + default: + ERROR(jail->pakfire, "Could not write to stdin: %m\n"); + goto ERROR; + } + } + } + } + // Check if any file descriptors have been closed if (e & EPOLLHUP) { // Remove the file descriptor @@ -1163,31 +1236,6 @@ static int pakfire_jail_wait_for_signal(struct pakfire_jail* jail, int fd) { return r; } -static int pakfire_jail_stream_stdin(struct pakfire_jail* jail, struct pakfire_jail_exec* ctx) { - int r; - - // Nothing to do if there is no stdin callback set - if (!ctx->communicate.in) { - DEBUG(jail->pakfire, "Callback for standard input is not set\n"); - return 0; - } - - int* fd = &ctx->pipes.stdin[1]; - - DEBUG(jail->pakfire, "Streaming standard input...\n"); - - // Calling the callback - r = ctx->communicate.in(jail->pakfire, ctx->communicate.data, *fd); - - DEBUG(jail->pakfire, "Standard input callback finished: %d\n", r); - - // Close the file descriptor when we are done - close(*fd); - *fd = 0; - - return r; -} - /* Performs the initialisation that needs to happen in the parent part */ @@ -1217,11 +1265,6 @@ static int pakfire_jail_parent(struct pakfire_jail* jail, struct pakfire_jail_ex if (r) return r; - // Stream standard input - r = pakfire_jail_stream_stdin(jail, ctx); - if (r) - return r; - return 0; } diff --git a/tests/libpakfire/jail.c b/tests/libpakfire/jail.c index cd71f29b5..4b115dd58 100644 --- a/tests/libpakfire/jail.c +++ b/tests/libpakfire/jail.c @@ -344,25 +344,31 @@ FAIL: } static int callback_stdin(struct pakfire* pakfire, void* data, int fd) { - const char* lines[] = { "a", "b", "c", NULL }; + int* lines = (int*)data; int r; - for (const char** line = lines; *line; line++) { - r = dprintf(fd, "%s\n", *line); + while (*lines > 0) { + r = dprintf(fd, "LINE %d\n", *lines); if (r < 0) { - LOG_ERROR("Could not write line (%s) to stdin: %m\n", *line); + LOG_ERROR("Could not write line (%u) to stdin: %m\n", *lines); return 1; } + + // Decrement the lines counter + (*lines)--; } - return 0; + return EOF; } static int test_communicate(const struct test* t) { struct pakfire_jail* jail = NULL; int r = EXIT_FAILURE; + // How many lines to send? + int lines = 65535; + const char* argv[] = { "/command", "pipe", NULL, }; @@ -371,7 +377,7 @@ static int test_communicate(const struct test* t) { ASSERT_SUCCESS(pakfire_jail_create(&jail, t->pakfire, 0)); // Check if the mount actually works - ASSERT_SUCCESS(pakfire_jail_exec(jail, argv, callback_stdin, NULL, NULL)); + ASSERT_SUCCESS(pakfire_jail_exec(jail, argv, callback_stdin, NULL, &lines)); // Success r = EXIT_SUCCESS; diff --git a/tests/stub/command.c b/tests/stub/command.c index 42d005cb3..ca69671d2 100644 --- a/tests/stub/command.c +++ b/tests/stub/command.c @@ -346,21 +346,27 @@ static int stat_ownership(int argc, char* argv[]) { Reads from stdin and writes it back to stdout */ static int _pipe(int argc, char* argv[]) { - char buffer[4096]; - char* p = NULL; int r; for (;;) { - p = fgets(buffer, sizeof(buffer), stdin); - if (!p) + // Read one character from stdin + int c = fgetc(stdin); + + // Break if we are done reading + if (c == EOF) break; - r = fprintf(stdout, "%s", buffer); - if (r < 0) - return 1; + // Write the character to stdout + r = fputc(c, stdout); + if (r == EOF) { + fprintf(stderr, "Could not write to stdout: %m\n"); + break; + } + + r = 0; } - return 0; + return r; } /*