Date: Tue, 7 Jan 2003 06:02:16 +0200 From: Giorgos Keramidas <keramida@freebsd.org> To: freebsd-audit@freebsd.org Cc: Peter Pentchev <roam@ringlet.net> Subject: Re: hopefully, a fix for an old uncompress(1) bug Message-ID: <20030107040216.GB4071@gothmog.gr> In-Reply-To: <20030106082154.GE1094@gothmog.gr> References: <20030106062309.GA37109@gothmog.gr> <20030106080949.GA382@straylight.oblivion.bg> <20030106082154.GE1094@gothmog.gr>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2003-01-06 10:21, Giorgos Keramidas <keramida@FreeBSD.ORG> wrote:
> On 2003-01-06 10:09, Peter Pentchev <roam@ringlet.net> wrote:
> > > if ((ifp = zopen(in, "r", bits)) == NULL) {
> > > cwarn("%s", in);
> > > - goto err;
> > > + return;
> >
> > Is this change really needed? It is true that the code at 'err' would
> > be a no-op at this point, when neither ifp nor ofp has been opened, but
> > it strikes me as a bit more semantically correct to invoke the "real"
> > error-handling procedures at any error, just in case something changes
> > in the future and some error-handling does indeed become necessary.
>
> Probably not. I'm happy with any of them two versions. Your diff is
> even better since it reduces the changes to the absolute minimum.
Hmmm, it seems that I spoke too hastily. No, we shouldn't `goto err'
in compress() or decompress() before we successfully open the output
file. This is because unlink() is run to the output file under err:
and we don't know if we are permitted to run unlink() on the output
file. Not until we are sure that it was *us* that created it. I'm
still not 100% happy with the fact that failures during the
compression or decompression of files will cause the `output' file to
be removed, but solving that would require a temporary file and I'm
nont sure I like that either :-(
When decompress() runs there are two bugs with the repository version
right now. When uncompress(1) is run as:
% uncompress -f foo
the decompress() function runs with `in' set to "foo.Z" and `out' set
to "foo".
The first problem is that decompress() tries to open the output file
first and then notice that its input file (foo.Z) fails to zopen().
When this happens, it promptly goes ahead and tries to unlink `foo'
which is supposed to be the outfile. If a file happens to be named
`foo' it is unconditionally removed. This is the behavior shown
below:
% ls -l
-rw-rw-r-- 1 giorgos wheel - 0 Jan 6 08:04 lala
% uncompress -f lala
uncompress: lala.Z: No such file or directory
% ls -l
%
Reversing the order of `in' and `out' file opening fixes this problem,
but there is one more which is a bit more difficult to notice.
The second problem. If the input file of decompress() does exist but
is not a valid .Z file the zopen() call will work fine only to fail
later on when we try to fread() data from it. This is shown below:
$ cp /COPYRIGHT lala.Z
$ touch lala
$ ls -l lala*
-rw-rw-r-- 1 giorgos giorgos - 0 Jan 7 05:42 lala
-r--r--r-- 1 giorgos giorgos - 4735 Jan 7 05:42 lala.Z
$ ./uncompress -f lala
uncompress: lala.Z: Inappropriate file type or format
$ ls -l lala*
-r--r--r-- 1 giorgos giorgos - 4735 Jan 7 05:42 lala.Z
$
I tried to solve this, by attempting to successfully read at least one
byte of data from the .Z file before even attempting to open the
output file. I also changed all the `goto err' parts before the
output file is opened to avoid unlinking() a file that we haven't
previously fopen()ed.
Hopefully, this fixes both the problems :-/
%%%
Index: compress.c
===================================================================
RCS file: /home/ncvs/src/usr.bin/compress/compress.c,v
retrieving revision 1.20
diff -u -r1.20 compress.c
--- compress.c 28 Jul 2002 15:32:17 -0000 1.20
+++ compress.c 7 Jan 2003 03:57:26 -0000
@@ -300,14 +300,9 @@
isreg = oreg = !exists || S_ISREG(sb.st_mode);
ifp = ofp = NULL;
- if ((ofp = fopen(out, "w")) == NULL) {
- cwarn("%s", out);
- return;
- }
-
if ((ifp = zopen(in, "r", bits)) == NULL) {
cwarn("%s", in);
- goto err;
+ return;
}
if (stat(in, &sb)) {
cwarn("%s", in);
@@ -315,6 +310,18 @@
}
if (!S_ISREG(sb.st_mode))
isreg = 0;
+
+ /*
+ * Try to read the first few uncompressed bytes from the input file
+ * before blindly truncating the output file.
+ */
+ if ((nr = fread(buf, 1, sizeof(buf), ifp)) == 0 ||
+ (ofp = fopen(out, "w")) == NULL ||
+ (nr != 0 && fwrite(buf, 1, nr, ofp) != nr)) {
+ cwarn("%s", out);
+ (void)fclose(ifp);
+ return;
+ }
while ((nr = fread(buf, 1, sizeof(buf), ifp)) != 0)
if (fwrite(buf, 1, nr, ofp) != nr) {
%%%
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030107040216.GB4071>