From: Tim Kientzle Date: Wed, 1 Apr 2009 01:42:20 +0000 (-0400) Subject: Improve the error handling here just a bit by harvesting X-Git-Tag: v2.7.0~57 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b392c33c94d8d8fa97cd82e1e935c919f2fdca27;p=thirdparty%2Flibarchive.git Improve the error handling here just a bit by harvesting the child (and collecting its exit status) whenever the child closes stdout. By not waiting until archive_close, we catch most cases of a failed child at the first real I/O (usually archive_read_next_header). I'm still investigating whether it makes sense to try pushing this even earlier... It may be possible to detect some child failures as early as read_open() time. SVN-Revision: 884 --- diff --git a/libarchive/archive_read_support_compression_program.c b/libarchive/archive_read_support_compression_program.c index 22ab0e0d9..c81779bf6 100644 --- a/libarchive/archive_read_support_compression_program.c +++ b/libarchive/archive_read_support_compression_program.c @@ -38,6 +38,9 @@ __FBSDID("$FreeBSD: src/lib/libarchive/archive_read_support_compression_program. #ifdef HAVE_LIMITS_H # include #endif +#ifdef HAVE_SIGNAL_H +# include +#endif #ifdef HAVE_STDLIB_H # include #endif @@ -119,6 +122,8 @@ static int program_bidder_free(struct archive_read_filter_bidder *); struct program_filter { char *description; pid_t child; + int exit_status; + int waitpid_return; int child_stdin, child_stdout; char *out_buf; @@ -210,6 +215,71 @@ program_bidder_bid(struct archive_read_filter_bidder *self, return (INT_MAX); } +/* + * Shut down the child, return ARCHIVE_OK if it exited normally. + * + * Note that the return value is sticky; if we're called again, + * we won't reap the child again, but we will return the same status + * (including error message if the child came to a bad end). + */ +static int +child_stop(struct archive_read_filter *self, struct program_filter *state) +{ + /* Close our side of the I/O with the child. */ + if (state->child_stdin != -1) { + close(state->child_stdin); + state->child_stdin = -1; + } + if (state->child_stdout != -1) { + close(state->child_stdout); + state->child_stdout = -1; + } + + if (state->child != 0) { + /* Reap the child. */ + do { + state->waitpid_return + = waitpid(state->child, &state->exit_status, 0); + } while (state->waitpid_return == -1 && errno == EINTR); + state->child = 0; + } + + if (state->waitpid_return < 0) { + /* waitpid() failed? This is ugly. */ + archive_set_error(&self->archive->archive, ARCHIVE_ERRNO_MISC, + "Child process exited badly"); + return (ARCHIVE_WARN); + } + + if (WIFSIGNALED(state->exit_status)) { + /* If the child died because we stopped reading before + * it was done, that's okay. Some archive formats + * have padding at the end that we routinely ignore. */ + /* The alternative to this would be to add a step + * before close(child_stdout) above to read from the + * child until the child has no more to write. */ + if (WTERMSIG(state->exit_status) == SIGPIPE) + return (ARCHIVE_OK); + archive_set_error(&self->archive->archive, ARCHIVE_ERRNO_MISC, + "Child process exited with signal %d", + WTERMSIG(state->exit_status)); + return (ARCHIVE_WARN); + } + + if (WIFEXITED(state->exit_status)) { + if (WEXITSTATUS(state->exit_status) == 0) + return (ARCHIVE_OK); + + archive_set_error(&self->archive->archive, + ARCHIVE_ERRNO_MISC, + "Child process exited with status %d", + WEXITSTATUS(state->exit_status)); + return (ARCHIVE_WARN); + } + + return (ARCHIVE_WARN); +} + /* * Use select() to decide whether the child is ready for read or write. */ @@ -229,11 +299,10 @@ child_read(struct archive_read_filter *self, char *buf, size_t buf_len) if (ret > 0) return (ret); - if (ret == 0 || (ret == -1 && errno == EPIPE)) { - close(state->child_stdout); - state->child_stdout = -1; - return (0); - } + if (ret == 0 || (ret == -1 && errno == EPIPE)) + /* Child has closed its output; reap the child + * and return the status. */ + return (child_stop(self, state)); if (ret == -1 && errno != EAGAIN) return (-1); @@ -352,8 +421,11 @@ program_filter_read(struct archive_read_filter *self, const void **buff) while (state->child_stdout != -1 && total < state->out_buf_len) { bytes = child_read(self, p, state->out_buf_len - total); if (bytes < 0) - return (bytes); + /* No recovery is possible if we can no longer + * read from the child. */ + return (ARCHIVE_FATAL); if (bytes == 0) + /* We got EOF from the child. */ break; total += bytes; p += bytes; @@ -367,45 +439,16 @@ static int program_filter_close(struct archive_read_filter *self) { struct program_filter *state; - int status; - int r, e = ARCHIVE_OK; + int e; state = (struct program_filter *)self->data; - - /* Shut down the child. */ - if (state->child_stdin != -1) - close(state->child_stdin); - if (state->child_stdout != -1) - close(state->child_stdout); - - r = waitpid(state->child, &status, 0); - while (r == -1 && errno == EINTR) - r = waitpid(state->child, &status, 0); + e = child_stop(self, state); /* Release our private data. */ free(state->out_buf); free(state->description); free(state); - if (r < 0) { - archive_set_error(&self->archive->archive, ARCHIVE_ERRNO_MISC, - "Child process exited badly"); - e = ARCHIVE_WARN; - } else if (WIFSIGNALED(status)) { - archive_set_error(&self->archive->archive, ARCHIVE_ERRNO_MISC, - "Child process exited with signal %d", WTERMSIG(status)); - e = ARCHIVE_WARN; - } else if (WIFEXITED(status)) { - if (WEXITSTATUS(status) != 0) { - archive_set_error(&self->archive->archive, - ARCHIVE_ERRNO_MISC, - "Child process exited with status %d", - WEXITSTATUS(status)); - e = ARCHIVE_WARN; - } - } else { - e = ARCHIVE_WARN; - } return (e); }