Re: Some minor fixes to some memory issues
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 <kolan_n@mail.ru>
> 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
Reply to: