Ian Jackson writes ("dpkg fd and error handling bugs (with patch to fix)"):
> The attached patch fixes various error-handling bugs in dpkg. Most of
> them are minor but there is one particularly important bug which is
> the erroneous repeated closure of the fsys tarfile pipe fd, which
> in principle could cause arbitrary weirdness.
It turns out that that patch was wrong. tarfn.c didn't #include
dpkg.h and as a result you get implicit declarations of m_malloc. I
wish we could have -Werror back.
Anyway, here's the revised patch; disregard the previous one.
Ian.
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/debian/changelog
dpkg-1.14.5ubuntu12/debian/changelog
--- old/dpkg-1.14.5ubuntu14/debian/changelog 2007-09-18 17:21:02.000000000
+0100
+++ dpkg-1.14.5ubuntu12/debian/changelog 2007-09-21 19:14:07.000000000
+0100
@@ -1,3 +1,33 @@
+dpkg (1.14.5ubuntu16) gutsy; urgency=low
+
+ * Fix some portability problems revealed by compiler warnings:
+ - missing <dpkg.h> in tarfn.c, implicit declaration of m_malloc
+ - missing cast for %ld ohshite at info.c:98
+ - unused yyunput (missing %option nounput) in trigdeferred.l
+
+ -- Ian Jackson <[EMAIL PROTECTED]> Fri, 21 Sep 2007 19:03:36 +0100
+
+dpkg (1.14.5ubuntu15) gutsy; urgency=low
+
+ * Bugfixes to fd cleanup handling:
+ - avoid closing fsys tarfile pipe twice even in normal
+ operation - normally EBADF but might sometimes close some other
+ desired fd and cause hideous doom. (LP: #137191.)
+ - avoid duplicate attempts to [f]close in obscure error
+ situations which might conceiveably close wrong fds
+ - cast &fd to void* when passing to push_cleanup cu_closefd
+ - fix parse.c:parsedb to use ehflag_normaltidy in a sane way
+ - when passing &fd to push_cleanup cu_closefd, make fd always static
+ * Bugfix in trigger deferred file processing: reset lexer start state
+ when calling yyrestart (has no effect except after parsing/reading
+ errors in the deferred file).
+ * Fix some error handling bugs in tarfn.c:
+ - Avoid freeing uninitialised h.[Link]Name (can cause crash if .deb
+ becomes unreadable while we start up). (LP: #138887.)
+ - Use m_malloc instead of malloc (and ditch ad-hoc error handling).
+
+ -- Ian Jackson <[EMAIL PROTECTED]> Thu, 20 Sep 2007 18:12:20 +0100
+
dpkg (1.14.5ubuntu14) gutsy; urgency=low
* Change syntax of `processing:...' status fd outputs so
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/dpkg-deb/info.c
dpkg-1.14.5ubuntu12/dpkg-deb/info.c
--- old/dpkg-1.14.5ubuntu14/dpkg-deb/info.c 2007-07-04 15:26:33.000000000
+0100
+++ dpkg-1.14.5ubuntu12/dpkg-deb/info.c 2007-09-21 18:58:14.000000000 +0100
@@ -95,7 +95,7 @@
pathlen = strlen(directory) + strlen(component) + 2;
controlfile = (void *) realloc((void *) controlfile, pathlen);
if (!controlfile)
- ohshite(_("realloc failed (%ld bytes)"), pathlen);
+ ohshite(_("realloc failed (%lu bytes)"), (unsigned long)pathlen);
memset(controlfile, 0, sizeof(controlfile));
strcat(controlfile, directory);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/dpkg.h
dpkg-1.14.5ubuntu12/lib/dpkg.h
--- old/dpkg-1.14.5ubuntu14/lib/dpkg.h 2007-09-18 12:13:17.000000000 +0100
+++ dpkg-1.14.5ubuntu12/lib/dpkg.h 2007-09-20 16:52:46.000000000 +0100
@@ -236,6 +236,16 @@
void cu_closefd(int argc, void **argv);
void cu_closedir(int argc, void **argv);
+int ferror_fclose_pop_cleanup(FILE *f);
+ /* calls ferror and fclose on f, and pop_cleanup
+ * file= fopen
+ * push_cleanup(cu_closefile,~ehflag_normaltidy, 0,0, 1,(void*)file
+ * return is 0 on success or EOF if fclose or ferror failed
+ * all three calls are always made regardless, and in the right order
+ * so you can just call ohshite if this returns EOF
+ */
+
+
/*** from mlib.c ***/
void setcloexec(int fd, const char* fn);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/ehandle.c
dpkg-1.14.5ubuntu12/lib/ehandle.c
--- old/dpkg-1.14.5ubuntu14/lib/ehandle.c 2007-09-18 12:13:49.000000000
+0100
+++ dpkg-1.14.5ubuntu12/lib/ehandle.c 2007-09-20 17:03:31.000000000 +0100
@@ -330,6 +330,14 @@
if (f) fclose(f);
}
+int ferror_fclose_pop_cleanup(FILE *f) {
+ int r1, r2;
+ r1= ferror(f);
+ pop_cleanup(ehflag_normaltidy);
+ r2= fclose(f);
+ return r1 ? r1 : r2;
+}
+
void cu_closedir(int argc, void **argv) {
DIR *d= (DIR*)(argv[0]);
if (d) closedir(d);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/parse.c
dpkg-1.14.5ubuntu12/lib/parse.c
--- old/dpkg-1.14.5ubuntu14/lib/parse.c 2007-08-16 00:44:00.000000000 +0100
+++ dpkg-1.14.5ubuntu12/lib/parse.c 2007-09-20 17:19:53.000000000 +0100
@@ -87,7 +87,7 @@
* If donep is not null only one package's information is expected.
*/
- int fd;
+ static int fd;
struct pkginfo newpig, *pigp;
struct pkginfoperfile *newpifp, *pifp;
struct arbitraryfield *arp, **larpp;
@@ -109,7 +109,7 @@
fd= open(filename, O_RDONLY);
if (fd == -1) ohshite(_("failed to open package info file `%.255s' for
reading"),filename);
- push_cleanup(cu_parsedb,~0, NULL,0, 1,&fd);
+ push_cleanup(cu_parsedb,~ehflag_normaltidy, NULL,0, 1,&fd);
if (fstat(fd, &stat) == -1)
ohshite(_("can't stat package info file `%.255s'"),filename);
@@ -355,7 +355,6 @@
if (EOF_mmap(dataptr, endptr)) break;
if (c == '\n') lno++;
}
- pop_cleanup(0);
if (data != NULL) {
#ifdef HAVE_MMAP
munmap(data, stat.st_size);
@@ -364,6 +363,7 @@
#endif
}
free(value);
+ pop_cleanup(ehflag_normaltidy);
if (close(fd)) ohshite(_("failed to close after read: `%.255s'"),filename);
if (donep && !pdone) ohshit(_("no package information in
`%.255s'"),filename);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/tarfn.c
dpkg-1.14.5ubuntu12/lib/tarfn.c
--- old/dpkg-1.14.5ubuntu14/lib/tarfn.c 2007-07-04 15:26:32.000000000 +0100
+++ dpkg-1.14.5ubuntu12/lib/tarfn.c 2007-09-21 18:54:31.000000000 +0100
@@ -14,6 +14,7 @@
#include <grp.h>
#include <errno.h>
#include <tarfn.h>
+#include <dpkg.h>
struct TarHeader {
char Name[100];
@@ -61,7 +62,7 @@
char * str;
len = strnlen(s, size);
- str = malloc(len + 1);
+ str = m_malloc(len + 1);
memcpy(str, s, len);
str[len] = 0;
@@ -137,9 +138,11 @@
next_long_name = NULL;
next_long_link = NULL;
long_read = 0;
- symListBottom = symListPointer = symListTop = malloc(sizeof(symlinkList));
+ symListBottom = symListPointer = symListTop = m_malloc(sizeof(symlinkList));
symListTop->next = NULL;
+ h.Name = NULL;
+ h.LinkName = NULL;
h.UserData = userData;
while ( (status = functions->Read(userData, buffer, 512)) == 512 ) {
@@ -206,13 +209,7 @@
errno = 0;
break;
}
- if ((symListBottom->next = malloc(sizeof(symlinkList))) == NULL) {
- free(symListBottom->h.LinkName);
- free(symListBottom->h.Name);
- status = -1;
- errno = 0;
- break;
- }
+ symListBottom->next = m_malloc(sizeof(symlinkList));
symListBottom = symListBottom->next;
symListBottom->next = NULL;
status = 0;
@@ -233,12 +230,7 @@
if (*longp)
free(*longp);
- if (NULL == (*longp = (char *)malloc(h.Size))) {
- /* malloc failed, so bail */
- errno = 0;
- status = -1;
- break;
- }
+ *longp = (char *)m_malloc(h.Size);
bp = *longp;
// the way the GNU long{link,name} stuff works is like this:
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/trigdeferred.l
dpkg-1.14.5ubuntu12/lib/trigdeferred.l
--- old/dpkg-1.14.5ubuntu14/lib/trigdeferred.l 2007-08-21 15:22:28.000000000
+0100
+++ dpkg-1.14.5ubuntu12/lib/trigdeferred.l 2007-09-21 18:58:31.000000000
+0100
@@ -24,6 +24,7 @@
%option prefix="trigdef_yy"
%option outfile="trigdeferred.c"
%option noyywrap
+%option nounput
%option batch
%option nodefault
%option perf-report
@@ -142,6 +143,7 @@
return 1;
trigdef_yyrestart(old_deferred);
+ BEGIN(0);
return 2;
}
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/lib/triglib.c
dpkg-1.14.5ubuntu12/lib/triglib.c
--- old/dpkg-1.14.5ubuntu14/lib/triglib.c 2007-08-16 16:52:53.000000000
+0100
+++ dpkg-1.14.5ubuntu12/lib/triglib.c 2007-09-20 17:38:53.000000000 +0100
@@ -296,9 +296,8 @@
}
if (signum > 0)
fprintf(nf,"%s\n",pkg->name);
- if (ferror(nf) || fclose(nf))
+ if (ferror_fclose_pop_cleanup(nf))
ohshite(_("unable to write new trigger interest file `%.250s'"),newfn.buf);
- pop_cleanup(ehflag_normaltidy);
if (rename(newfn.buf,trk_explicit_fn.buf))
ohshite(_("unable to install new trigger interest file `%.250s'"),
@@ -369,10 +368,9 @@
for (tfi=filetriggers.head; tfi; tfi=tfi->inoverall.next)
fprintf(nf, "%s %s\n", trigh.namenode_name(tfi->fnn), tfi->pkg->name);
- if (ferror(nf) || fclose(nf))
+ if (ferror_fclose_pop_cleanup(nf))
ohshite(_("unable to write new file triggers file `%.250s'"),
triggersnewfilefile);
- pop_cleanup(ehflag_normaltidy);
if (rename(triggersnewfilefile, triggersfilefile))
ohshite(_("unable to install new file triggers file as `%.250s'"),
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/src/archives.c
dpkg-1.14.5ubuntu12/src/archives.c
--- old/dpkg-1.14.5ubuntu14/src/archives.c 2007-09-18 12:17:35.000000000
+0100
+++ dpkg-1.14.5ubuntu12/src/archives.c 2007-09-20 16:56:46.000000000 +0100
@@ -367,11 +367,12 @@
int tarobject(struct TarInfo *ti) {
static struct varbuf conffderefn, hardlinkfn, symlinkfn;
+ static int fd;
const char *usename;
struct conffile *conff;
struct tarcontext *tc= (struct tarcontext*)ti->UserData;
- int statr, fd, i, existingdirectory, keepexisting;
+ int statr, i, existingdirectory, keepexisting;
size_t r;
struct stat stab, stabtmp;
char databuf[TARBLKSZ];
@@ -641,7 +642,7 @@
*/
fd= open(fnamenewvb.buf, (O_CREAT|O_EXCL|O_WRONLY), 0);
if (fd < 0) ohshite(_("unable to create `%.255s'"),ti->Name);
- push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,&fd);
+ push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,(void*)&fd);
debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
(unsigned long)ti->Size);
{ char fnamebuf[256];
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/src/configure.c
dpkg-1.14.5ubuntu12/src/configure.c
--- old/dpkg-1.14.5ubuntu14/src/configure.c 2007-09-18 12:10:00.000000000
+0100
+++ dpkg-1.14.5ubuntu12/src/configure.c 2007-09-20 16:59:11.000000000 +0100
@@ -425,7 +425,7 @@
fd=open(fn,O_RDONLY);
if (fd>=0) {
- push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,&fd);
+ push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,(void*)&fd);
fd_md5(fd, hashbuf, -1, _("md5hash"));
pop_cleanup(ehflag_normaltidy); /* fd= open(cdr.buf) */
close(fd);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/src/filesdb.c
dpkg-1.14.5ubuntu12/src/filesdb.c
--- old/dpkg-1.14.5ubuntu14/src/filesdb.c 2007-08-16 00:44:00.000000000
+0100
+++ dpkg-1.14.5ubuntu12/src/filesdb.c 2007-09-20 16:59:11.000000000 +0100
@@ -133,7 +133,7 @@
return;
}
- push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,&fd);
+ push_cleanup(cu_closefd,ehflag_bombout, 0,0, 1,(void*)&fd);
if(fstat(fd, &stat_buf))
ohshite("unable to stat files list file for package `%.250s'",pkg->name);
diff --exclude='*~' -ru old/dpkg-1.14.5ubuntu14/src/processarc.c
dpkg-1.14.5ubuntu12/src/processarc.c
--- old/dpkg-1.14.5ubuntu14/src/processarc.c 2007-09-18 11:35:37.000000000
+0100
+++ dpkg-1.14.5ubuntu12/src/processarc.c 2007-09-20 15:46:48.000000000
+0100
@@ -562,13 +562,12 @@
execlp(BACKEND, BACKEND, "--fsys-tarfile", filename, (char*)0);
ohshite(_("unable to exec dpkg-deb to get filesystem archive"));
}
- close(p1[1]);
+ close(p1[1]); p1[1]= -1;
newfileslist= 0; tc.newfilesp= &newfileslist;
push_cleanup(cu_fileslist,~0, 0, 0, 0);
tc.pkg= pkg;
tc.backendpipe= p1[0];
- push_cleanup(cu_closefd,~ehflag_bombout, 0,0, 1,&tc.backendpipe);
r= TarExtractor((void*)&tc, &tf);
if (r) {
@@ -578,8 +577,8 @@
ohshit(_("corrupted filesystem tarfile - corrupted package archive"));
}
}
- fd_null_copy(tc.backendpipe,-1,_("dpkg-deb: zap possible trailing zeros"));
- close(tc.backendpipe);
+ fd_null_copy(p1[0],-1,_("dpkg-deb: zap possible trailing zeros"));
+ close(p1[0]); p1[0]= -1;
waitsubproc(c1,BACKEND " --fsys-tarfile",PROCPIPE);
if (oldversionstatus == stat_halfinstalled || oldversionstatus ==
stat_unpacked) {
