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
--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)