Hi! On Sun, 2020-07-26 at 23:54:44 +0300, KOLANICH wrote: > I have implemented some fixes for some issues found by clang 12 > UBSan (or MemSan, os ASan, I don't remember now) and valgrind: > https://github.com/guillemj/dpkg/pull/2.
Thanks for the patches/report. It would have been more helpful to send each different fix in a separate patch, but no worries. See below for a review. > From 62d2d15b57af5150965913c4fd98edd0c2d97dfe Mon Sep 17 00:00:00 2001 > From: KOLANICH <[email protected]> > Date: Thu, 23 Jul 2020 16:37:06 +0300 > Subject: [PATCH] Fixed some memory leaks and UB > diff --git a/lib/dpkg/tarfn.c b/lib/dpkg/tarfn.c > index a0821f217..c759918e8 100644 > --- a/lib/dpkg/tarfn.c > +++ b/lib/dpkg/tarfn.c > @@ -458,10 +458,13 @@ tar_extractor(struct tar_archive *tar) > h.linkname = NULL; > h.stat.uname = NULL; > h.stat.gname = NULL; > + tar->err.str = NULL; > > while ((status = tar->ops->read(tar, buffer, TARBLKSZ)) == TARBLKSZ) { > int name_len; > > + if(tar->err.str != NULL) > + free(tar->err.str); > if (tar_header_decode((struct tar_header *)buffer, &h, > &tar->err) < 0) { > if (h.name[0] == '\0') { > /* End Of Tape. */ > @@ -485,6 +488,8 @@ tar_extractor(struct tar_archive *tar) > } > > if (h.name[0] == '\0') { > + if(tar->err.str != NULL) > + free(tar->err.str); > status = dpkg_put_error(&tar->err, > _("invalid tar header with > empty name field")); > errno = 0; These seem incorrect. tar->err is supposed to be initialized on the call site (with DPKG_ERROR_OBJECT), which it is. Then there's never a need to check for NULL before calling free(). And in this case freeing these is unnecessary as any code path here that sets tar->err is supposed to make the loop stop and the function return. > diff --git a/lib/dpkg/trigdeferred.c b/lib/dpkg/trigdeferred.c > index b9b028f73..71d050318 100644 > --- a/lib/dpkg/trigdeferred.c > +++ b/lib/dpkg/trigdeferred.c > @@ -85,6 +85,7 @@ trigdef_update_start(enum trigdef_update_flags uf) > ohshite(_("unable to open/create " > "triggers lock file > '%.250s'"), > fn.buf); > + free(triggersdir); > return TDUS_ERROR_NO_DIR; > } > } > @@ -121,6 +122,7 @@ trigdef_update_start(enum trigdef_update_flags uf) > if (stab.st_size == 0 && !(uf & TDUF_WRITE_IF_EMPTY)) { > if (uf & TDUF_WRITE) > pop_cleanup(ehflag_normaltidy); > + free(triggersdir); > return TDUS_ERROR_EMPTY_DEFERRED; > } > } > @@ -136,6 +138,7 @@ trigdef_update_start(enum trigdef_update_flags uf) > > setcloexec(fileno(trig_new_deferred), newfn.buf); > } > + free(triggersdir); > > if (!old_deferred) > return TDUS_NO_DEFERRED; These are not really correct either, as the variable needs to be set at least for the _done function to be able to work. I've instead added a free() just immediately before the assignment. > diff --git a/lib/dpkg/varbuf.c b/lib/dpkg/varbuf.c > index ee757bcc7..cce61f612 100644 > --- a/lib/dpkg/varbuf.c > +++ b/lib/dpkg/varbuf.c > @@ -95,9 +95,11 @@ varbuf_vprintf(struct varbuf *v, const char *fmt, va_list > args) > void > varbuf_add_buf(struct varbuf *v, const void *s, size_t size) > { > - varbuf_grow(v, size); > - memcpy(v->buf + v->used, s, size); > - v->used += size; > + if(size){ > + varbuf_grow(v, size); > + memcpy(v->buf + v->used, s, size); > + v->used += size; > + } > } I don't see why the old code was wrong? All these should handle size being 0 just fine. > void > diff --git a/src/main.c b/src/main.c > index 3482ed1e2..5d3eaf0ea 100644 > --- a/src/main.c > +++ b/src/main.c > @@ -781,8 +781,10 @@ int main(int argc, const char *const *argv) { > ohshite(_("unable to setenv for subprocesses")); > if (setenv("DPKG_ROOT", instdir, 1) < 0) > ohshite(_("unable to setenv for subprocesses")); > - if (setenv("DPKG_FORCE", get_force_string(), 1) < 0) > + char *force_string = get_force_string(); > + if (setenv("DPKG_FORCE", force_string, 1) < 0) > ohshite(_("unable to setenv for subprocesses")); > + free(force_string); > > if (!f_triggers) > f_triggers = (cipaction->arg_int == act_triggers && *argv) ? -1 : 1; Thanks, fixed locally now. > diff --git a/src/unpack.c b/src/unpack.c > index 4a143666e..e73fa3e2e 100644 > --- a/src/unpack.c > +++ b/src/unpack.c > @@ -1114,7 +1114,7 @@ void process_archive(const char *filename) { > deb_verify(filename); > > /* Get the control information directory. */ > - cidir = get_control_dir(cidir); > + cidir = get_control_dir(cidir); // todo: Memory leak!!! cannot free > because it is used after exitting this func. > cidirrest = cidir + strlen(cidir); > push_cleanup(cu_cidir, ~0, 2, (void *)cidir, (void *)cidirrest); Right, thanks, I've added a free() within cu_cidir(). > @@ -1443,9 +1443,13 @@ void process_archive(const char *filename) { > tar.ops = &tf; > > rc = tar_extractor(&tar); > - if (rc) > + if (rc){ > dpkg_error_print(&tar.err, > _("corrupted filesystem tarfile in package archive")); > + } > + if(tar.err.str) // todo: during normal operation no error strings should > be set! > + free(tar.err.str); > + > if (fd_skip(p1[0], -1, &err) < 0) > ohshit(_("cannot zap possible trailing zeros from dpkg-deb: %s"), > err.str); > close(p1[0]); This seems incorrect. If there's an error, dpkg_error_print() will also error out (and there are no warnings being generated here). The function name is a bit unfortunate, and I think I've pondered renaming it to something like dpkg_error_emit() or similar, but given that it could warn or error out depending on the error variable, that's still a bit fuzzy. Thanks, Guillem
