This is an automated email from the git hooks/post-receive script. guillem pushed a commit to branch master in repository dpkg.
View the commit online: https://git.dpkg.org/cgit/dpkg/dpkg.git/commit/?id=0118b3c7052327f657b9a05cd0b8988775b24a42 commit 0118b3c7052327f657b9a05cd0b8988775b24a42 Author: Guillem Jover <[email protected]> AuthorDate: Mon Jan 30 03:04:07 2017 +0100 libdpkg: Add proper tar error handling This makes the tar extractor track and report back parse errors, so that we can give more descriptive messages. --- debian/changelog | 3 ++ lib/dpkg/t/c-tarextract.c | 28 ++++++++++------- lib/dpkg/tarfn.c | 80 ++++++++++++++++++++++++++--------------------- lib/dpkg/tarfn.h | 24 +++++++++++--- src/archives.c | 10 +++--- src/archives.h | 6 ++-- src/unpack.c | 18 ++++++----- 7 files changed, 104 insertions(+), 65 deletions(-) diff --git a/debian/changelog b/debian/changelog index 71bf15826..0cb18d8ed 100644 --- a/debian/changelog +++ b/debian/changelog @@ -48,6 +48,9 @@ dpkg (1.19.3) UNRELEASED; urgency=medium end up trying to process triggers with yet unsatisifiable dependencies. Closes: #810724, #854478, #911620 * dpkg: Fix --help output, to clarify which arguments are optional. + * libdpkg: Add proper tar error handling. This makes the tar extractor + track and report back parse errors, so that we can give more descriptive + messages. * Perl modules: - Dpkg::Changelog::Debian: Preserve modelines at EOF. Closes: #916056 Thanks to Chris Lamb <[email protected]> for initial test cases. diff --git a/lib/dpkg/t/c-tarextract.c b/lib/dpkg/t/c-tarextract.c index 4781c0237..34f3d08fa 100644 --- a/lib/dpkg/t/c-tarextract.c +++ b/lib/dpkg/t/c-tarextract.c @@ -42,16 +42,17 @@ struct tar_context { }; static int -tar_read(void *ctx, char *buffer, int size) +tar_read(struct tar_archive *tar, char *buffer, int size) { - struct tar_context *tc = ctx; + struct tar_context *tc = tar->ctx; return fd_read(tc->tar_fd, buffer, size); } static int -tar_object_skip(struct tar_context *tc, struct tar_entry *te) +tar_object_skip(struct tar_archive *tar, struct tar_entry *te) { + struct tar_context *tc = tar->ctx; off_t size; size = (te->size + TARBLKSZ - 1) / TARBLKSZ * TARBLKSZ; @@ -62,7 +63,7 @@ tar_object_skip(struct tar_context *tc, struct tar_entry *te) } static int -tar_object(void *ctx, struct tar_entry *te) +tar_object(struct tar_archive *tar, struct tar_entry *te) { printf("%s mode=%o time=%ld.%.9d uid=%d gid=%d", te->name, te->stat.mode, te->mtime, 0, te->stat.uid, te->stat.gid); @@ -74,7 +75,7 @@ tar_object(void *ctx, struct tar_entry *te) switch (te->type) { case TAR_FILETYPE_FILE0: case TAR_FILETYPE_FILE: - tar_object_skip(ctx, te); + tar_object_skip(tar, te); printf(" type=file size=%jd", (intmax_t)te->size); break; case TAR_FILETYPE_HARDLINK: @@ -116,7 +117,8 @@ struct tar_operations tar_ops = { int main(int argc, char **argv) { - struct tar_context ctx; + struct tar_archive tar; + struct tar_context tar_ctx; const char *tar_name = argv[1]; setvbuf(stdout, NULL, _IOLBF, 0); @@ -124,18 +126,22 @@ main(int argc, char **argv) push_error_context(); if (tar_name) { - ctx.tar_fd = open(tar_name, O_RDONLY); - if (ctx.tar_fd < 0) + tar_ctx.tar_fd = open(tar_name, O_RDONLY); + if (tar_ctx.tar_fd < 0) ohshite("cannot open file '%s'", tar_name); } else { - ctx.tar_fd = STDIN_FILENO; + tar_ctx.tar_fd = STDIN_FILENO; } - if (tar_extractor(&ctx, &tar_ops)) + tar.err = DPKG_ERROR_OBJECT; + tar.ctx = &tar_ctx; + tar.ops = &tar_ops; + + if (tar_extractor(&tar)) ohshite("extracting tar"); if (tar_name) - close(ctx.tar_fd); + close(tar_ctx.tar_fd); pop_error_context(ehflag_normaltidy); diff --git a/lib/dpkg/tarfn.c b/lib/dpkg/tarfn.c index 16005e6f8..8e3b6e545 100644 --- a/lib/dpkg/tarfn.c +++ b/lib/dpkg/tarfn.c @@ -38,6 +38,8 @@ #include <dpkg/macros.h> #include <dpkg/dpkg.h> +#include <dpkg/i18n.h> +#include <dpkg/error.h> #include <dpkg/tarfn.h> #define TAR_MAGIC_USTAR "ustar\0" "00" @@ -121,7 +123,7 @@ tar_atol8(const char *s, size_t size) if (s < end) return tar_ret_errno(EINVAL, 0); - return n; + return tar_ret_errno(0, n); } /** @@ -162,7 +164,7 @@ tar_atol256(const char *s, size_t size, intmax_t min, uintmax_t max) c = *s++; } - return n; + return tar_ret_errno(0, n); } static uintmax_t @@ -272,7 +274,7 @@ tar_header_checksum(struct tar_header *h) } static int -tar_header_decode(struct tar_header *h, struct tar_entry *d) +tar_header_decode(struct tar_header *h, struct tar_entry *d, struct dpkg_error *err) { long checksum; @@ -299,7 +301,11 @@ tar_header_decode(struct tar_header *h, struct tar_entry *d) /* Even though off_t is signed, we use an unsigned parser here because * negative offsets are not allowed. */ d->size = TAR_ATOUL(h->size, off_t); + if (errno) + return dpkg_put_errno(err, _("invalid tar header size field")); d->mtime = TAR_ATOSL(h->mtime, time_t); + if (errno) + return dpkg_put_errno(err, _("invalid tar header mtime field")); if (d->type == TAR_FILETYPE_CHARDEV || d->type == TAR_FILETYPE_BLOCKDEV) d->dev = makedev(TAR_ATOUL(h->devmajor, dev_t), @@ -312,19 +318,25 @@ tar_header_decode(struct tar_header *h, struct tar_entry *d) else d->stat.uname = NULL; d->stat.uid = TAR_ATOUL(h->uid, uid_t); + if (errno) + return dpkg_put_errno(err, _("invalid tar header uid field")); if (*h->group) d->stat.gname = m_strndup(h->group, sizeof(h->group)); else d->stat.gname = NULL; d->stat.gid = TAR_ATOUL(h->gid, gid_t); + if (errno) + return dpkg_put_errno(err, _("invalid tar header gid field")); checksum = tar_atol8(h->checksum, sizeof(h->checksum)); - - /* Check for parse errors. */ if (errno) - return 0; - return tar_header_checksum(h) == checksum; + return dpkg_put_errno(err, _("invalid tar header checksum field")); + + if (tar_header_checksum(h) != checksum) + return dpkg_put_error(err, _("invalid tar header checksum")); + + return 0; } /** @@ -339,8 +351,7 @@ tar_header_decode(struct tar_header *h, struct tar_entry *d) * bogus name or link. */ static int -tar_gnu_long(void *ctx, const struct tar_operations *ops, struct tar_entry *te, - char **longp) +tar_gnu_long(struct tar_archive *tar, struct tar_entry *te, char **longp) { char buf[TARBLKSZ]; char *bp; @@ -353,14 +364,15 @@ tar_gnu_long(void *ctx, const struct tar_operations *ops, struct tar_entry *te, for (long_read = te->size; long_read > 0; long_read -= TARBLKSZ) { int copysize; - status = ops->read(ctx, buf, TARBLKSZ); + status = tar->ops->read(tar, buf, TARBLKSZ); if (status == TARBLKSZ) status = 0; else { /* Read partial header record? */ if (status > 0) { errno = 0; - status = -1; + status = dpkg_put_error(&tar->err, + _("partially read tar header")); } /* If we didn't get TARBLKSZ bytes read, punt. */ @@ -427,7 +439,7 @@ tar_entry_update_from_system(struct tar_entry *te) } int -tar_extractor(void *ctx, const struct tar_operations *ops) +tar_extractor(struct tar_archive *tar) { int status; char buffer[TARBLKSZ]; @@ -445,18 +457,13 @@ tar_extractor(void *ctx, const struct tar_operations *ops) h.stat.uname = NULL; h.stat.gname = NULL; - while ((status = ops->read(ctx, buffer, TARBLKSZ)) == TARBLKSZ) { + while ((status = tar->ops->read(tar, buffer, TARBLKSZ)) == TARBLKSZ) { int name_len; - if (!tar_header_decode((struct tar_header *)buffer, &h)) { + if (tar_header_decode((struct tar_header *)buffer, &h, &tar->err) < 0) { if (h.name[0] == '\0') { - /* End of tape. */ + /* End Of Tape. */ status = 0; - } else { - /* Indicates broken tarfile: - * “Header checksum error”. */ - errno = 0; - status = -1; } tar_entry_destroy(&h); break; @@ -474,9 +481,9 @@ tar_extractor(void *ctx, const struct tar_operations *ops) } if (h.name[0] == '\0') { - /* Indicates broken tarfile: “Bad header data”. */ + status = dpkg_put_error(&tar->err, + _("invalid tar header with empty name field")); errno = 0; - status = -1; tar_entry_destroy(&h); break; } @@ -487,7 +494,7 @@ tar_extractor(void *ctx, const struct tar_operations *ops) case TAR_FILETYPE_FILE: /* Compatibility with pre-ANSI ustar. */ if (h.name[name_len - 1] != '/') { - status = ops->extract_file(ctx, &h); + status = tar->ops->extract_file(tar, &h); break; } /* Else, fall through. */ @@ -495,10 +502,10 @@ tar_extractor(void *ctx, const struct tar_operations *ops) if (h.name[name_len - 1] == '/') { h.name[name_len - 1] = '\0'; } - status = ops->mkdir(ctx, &h); + status = tar->ops->mkdir(tar, &h); break; case TAR_FILETYPE_HARDLINK: - status = ops->link(ctx, &h); + status = tar->ops->link(tar, &h); break; case TAR_FILETYPE_SYMLINK: symlink_node = m_malloc(sizeof(*symlink_node)); @@ -515,18 +522,19 @@ tar_extractor(void *ctx, const struct tar_operations *ops) case TAR_FILETYPE_CHARDEV: case TAR_FILETYPE_BLOCKDEV: case TAR_FILETYPE_FIFO: - status = ops->mknod(ctx, &h); + status = tar->ops->mknod(tar, &h); break; case TAR_FILETYPE_GNU_LONGLINK: - status = tar_gnu_long(ctx, ops, &h, &next_long_link); + status = tar_gnu_long(tar, &h, &next_long_link); break; case TAR_FILETYPE_GNU_LONGNAME: - status = tar_gnu_long(ctx, ops, &h, &next_long_name); + status = tar_gnu_long(tar, &h, &next_long_name); break; default: - /* Indicates broken tarfile: “Bad header field”. */ + status = dpkg_put_error(&tar->err, + _("unknown tar header type '%c'"), + h.type); errno = 0; - status = -1; } tar_entry_destroy(&h); if (status != 0) @@ -537,7 +545,7 @@ tar_extractor(void *ctx, const struct tar_operations *ops) while (symlink_head) { symlink_node = symlink_head->next; if (status == 0) - status = ops->symlink(ctx, &symlink_head->h); + status = tar->ops->symlink(tar, &symlink_head->h); tar_entry_destroy(&symlink_head->h); free(symlink_head); symlink_head = symlink_node; @@ -548,11 +556,11 @@ tar_extractor(void *ctx, const struct tar_operations *ops) free(next_long_link); if (status > 0) { - /* Indicates broken tarfile: “Read partial header record”. */ + status = dpkg_put_error(&tar->err, + _("partially read tar header")); errno = 0; - return -1; - } else { - /* Return whatever I/O function returned. */ - return status; } + + /* Return whatever I/O function returned. */ + return status; } diff --git a/lib/dpkg/tarfn.h b/lib/dpkg/tarfn.h index 37269de02..06685a720 100644 --- a/lib/dpkg/tarfn.h +++ b/lib/dpkg/tarfn.h @@ -26,6 +26,7 @@ #include <stdint.h> +#include <dpkg/error.h> #include <dpkg/file.h> /** @@ -58,7 +59,7 @@ enum tar_filetype { }; struct tar_entry { - /** Tar archive format. */ + /** Tar entry format. */ enum tar_format format; /** File type. */ enum tar_filetype type; @@ -76,8 +77,10 @@ struct tar_entry { struct file_stat stat; }; -typedef int tar_read_func(void *ctx, char *buffer, int length); -typedef int tar_make_func(void *ctx, struct tar_entry *h); +struct tar_archive; + +typedef int tar_read_func(struct tar_archive *tar, char *buffer, int length); +typedef int tar_make_func(struct tar_archive *tar, struct tar_entry *h); struct tar_operations { tar_read_func *read; @@ -89,6 +92,18 @@ struct tar_operations { tar_make_func *mknod; }; +struct tar_archive { + /* Global tar archive error. */ + struct dpkg_error err; + + /** Tar archive format. */ + enum tar_format format; + + /* Operation functions and context. */ + const struct tar_operations *ops; + void *ctx; +}; + uintmax_t tar_atoul(const char *s, size_t size, uintmax_t max); intmax_t @@ -97,7 +112,8 @@ tar_atosl(const char *s, size_t size, intmax_t min, intmax_t max); void tar_entry_update_from_system(struct tar_entry *te); -int tar_extractor(void *ctx, const struct tar_operations *ops); +int +tar_extractor(struct tar_archive *tar); /** @} */ diff --git a/src/archives.c b/src/archives.c index 5a6fc0838..ab93551b8 100644 --- a/src/archives.c +++ b/src/archives.c @@ -264,8 +264,10 @@ void cu_pathname(int argc, void **argv) { path_remove_tree((char*)(argv[0])); } -int tarfileread(void *ud, char *buf, int len) { - struct tarcontext *tc= (struct tarcontext*)ud; +int +tarfileread(struct tar_archive *tar, char *buf, int len) +{ + struct tarcontext *tc = (struct tarcontext *)tar->ctx; int r; r = fd_read(tc->backendpipe, buf, len); @@ -661,14 +663,14 @@ linktosameexistingdir(const struct tar_entry *ti, const char *fname, } int -tarobject(void *ctx, struct tar_entry *ti) +tarobject(struct tar_archive *tar, struct tar_entry *ti) { static struct varbuf conffderefn, symlinkfn; const char *usename; struct fsys_namenode *usenode; struct conffile *conff; - struct tarcontext *tc = ctx; + struct tarcontext *tc = tar->ctx; bool existingdir, keepexisting; bool refcounting; char oldhash[MD5HASHLEN + 1]; diff --git a/src/archives.h b/src/archives.h index 19c450c11..2ceab9b1b 100644 --- a/src/archives.h +++ b/src/archives.h @@ -70,8 +70,10 @@ void ok_prermdeconfigure(int argc, void **argv); void setupfnamevbs(const char *filename); -int tarobject(void *ctx, struct tar_entry *ti); -int tarfileread(void *ud, char *buf, int len); +int +tarobject(struct tar_archive *tar, struct tar_entry *ti); +int +tarfileread(struct tar_archive *tar, char *buf, int len); void tar_deferred_extract(struct fsys_namenode_list *files, struct pkginfo *pkg); diff --git a/src/unpack.c b/src/unpack.c index e8b06d289..ad1e05f56 100644 --- a/src/unpack.c +++ b/src/unpack.c @@ -1077,6 +1077,7 @@ void process_archive(const char *filename) { static enum pkgstatus oldversionstatus; static struct tarcontext tc; + struct tar_archive tar; struct dpkg_error err; enum parsedbflags parsedb_flags; int rc; @@ -1431,14 +1432,15 @@ void process_archive(const char *filename) { tc.backendpipe= p1[0]; tc.pkgset_getting_in_sync = pkgset_getting_in_sync(pkg); - rc = tar_extractor(&tc, &tf); - if (rc) { - if (errno) { - ohshite(_("error reading dpkg-deb tar output")); - } else { - ohshit(_("corrupted filesystem tarfile - corrupted package archive")); - } - } + /* Setup the tar archive. */ + tar.err = DPKG_ERROR_OBJECT; + tar.ctx = &tc; + tar.ops = &tf; + + rc = tar_extractor(&tar); + if (rc) + dpkg_error_print(&tar.err, + _("corrupted filesystem tarfile in package archive")); if (fd_skip(p1[0], -1, &err) < 0) ohshit(_("cannot zap possible trailing zeros from dpkg-deb: %s"), err.str); close(p1[0]); -- Dpkg.Org's dpkg
