NetBSD Problem Report #19722

Received: (qmail 16046 invoked by uid 605); 7 Jan 2003 10:20:50 -0000
Message-Id: <20030107102037.GA1255@gothmog.gr>
Date: Tue, 7 Jan 2003 12:20:37 +0200
From: Giorgos Keramidas <keramida@freebsd.org>
Sender: gnats-bugs-owner@netbsd.org
Reply-To: Giorgos Keramidas <keramida@freebsd.org>
To: gnats-bugs@gnats.netbsd.org
Subject: fix for 3 uncompress(1) bugs that delete/truncate the wrong file
X-Send-Pr-Version: 3.95

>Number:         19722
>Category:       bin
>Synopsis:       fix for 3 uncompress(1) bugs that delete/truncate the wrong file
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    rillig
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 07 10:21:00 +0000 2003
>Closed-Date:    Sun May 22 21:42:35 +0000 2022
>Last-Modified:  Tue May 24 21:08:39 +0000 2022
>Originator:     
>Release:        NetBSD 1.6
>Organization:
>Environment:
System: NetBSD spe143.testdrive.compaq.com 1.6 NetBSD 1.6 (GENERIC) #0: Sun Sep 8 19:43:40 UTC 2002 autobuild@tgm.daemon.org:/autobuild/i386/OBJ/autobuild/src/sys/arch/i386/compile/GENERIC i386
Architecture: i386
Machine: i386
>Description:

The attached diff fixes two bugs in uncompress(1) that are present in
both FreeBSD & NetBSD.  The output file of decompress() in compress.c
should not be unlinked in the following cases:
	- if the input file (the .Z one) cannot be opened for reading.
	- if the input file exists but fread() after zopen() fails.
The attached diff also fixes a minor bug that is caused by trying to
open the output file before checking if the input file exists.  The
output file shouldn't be opened first, because it will be truncated.
By moving the fopen() that truncates the output file further down, we
make sure that the output file isn't unnecessarily truncated if the
input file doesn't exist.

>How-To-Repeat:

The following two sample sessions from a NetBSD 1.6 installation at
Compaq's testdrive installations show the bugs in action:

	spe143-> rm lala.Z
	spe143-> ls -l lala*
	ls: No match.
	spe143-> touch lala
	spe143-> ls -l lala*
	-rw-r--r--  1 gk736  nis  0 Jan  6 23:22 lala
	spe143-> ln -s compress uncompress
	spe143-> ./uncompress -f lala
	uncompress: lala.Z: No such file or directory
	spe143-> ls -l lala*
	ls: No match.
	spe143->

	spe143-> cp /etc/rc.conf lala.Z
	spe143-> touch lala
	spe143-> ls -l lala*
	-rw-r--r--  1 gk736  nis    0 Jan  6 23:24 lala
	-rw-r--r--  1 gk736  nis  815 Jan  6 23:24 lala.Z
	spe143-> ./uncompress -f lala
	uncompress: lala.Z: Inappropriate file type or format
	spe143-> ls -l lala*
	-rw-r--r--  1 gk736  nis  815 Jan  6 23:24 lala.Z
	spe143->

>Fix:

%%%
Index: compress.c
===================================================================
RCS file: /cvsroot/src/usr.bin/compress/compress.c,v
retrieving revision 1.19
diff -u -r1.19 compress.c
--- compress.c	2002/05/26 22:21:22	1.19
+++ compress.c	2003/01/07 04:32:57
@@ -317,19 +317,15 @@
 		oreg = 0;

 	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 (!isstdin) {
 		if (stat(in, &sb)) {
 			cwarn("%s", in);
-			goto err;
+			(void)fclose(ifp);
+			return;
 		}
 		if (!S_ISREG(sb.st_mode))
 			isreg = 0;
@@ -337,6 +333,21 @@
 			isreg = 1;
 	} else
 		isreg = 0;
+
+	if ((nr = fread(buf, 1, sizeof(buf), ifp)) == 0) {
+		cwarn("%s", in);
+		(void)fclose(ifp);
+		return;
+	}
+	if ((ofp = fopen(out, "w")) == NULL) {
+		cwarn("%s", out);
+		(void)fclose(ifp);
+		return;
+	}
+	if (fwrite(buf, 1, nr, ofp) != nr) {
+		cwarn("%s", out);
+		goto err;
+	}

 	while ((nr = fread(buf, 1, sizeof(buf), ifp)) != 0)
 		if (fwrite(buf, 1, nr, ofp) != nr) {
%%%
>Release-Note:
>Audit-Trail:

From: Christian Biere <christianbiere@gmx.de>
To: Giorgos Keramidas <keramida@freebsd.org>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19722: fix for 3 uncompress(1) bugs that delete/truncate
 the wrong file
Date: Tue, 7 Jan 2003 21:09:19 +0100

 --=.K'1jRdUVZuuEHf
 Content-Type: text/plain; charset=US-ASCII
 Content-Transfer-Encoding: 7bit

 Giorgos Keramidas <keramida@freebsd.org> wrote:

 >  	if ((ifp = zopen(in, "r", bits)) == NULL) {
 >  		cwarn("%s", in);
 > -		goto err;
 > +		return;
 >  	}

 Isn't *here* a race condition?

 >  	if (!isstdin) {
 >  		if (stat(in, &sb)) {
 >  			cwarn("%s", in);
 > -			goto err;
 > +			(void)fclose(ifp);
 > +			return;
 >  		}
 >  		if (!S_ISREG(sb.st_mode))
 >  			isreg = 0;
 > @@ -337,6 +333,21 @@
 >  			isreg = 1;
 >  	} else
 >  		isreg = 0;

 Could someone give me a hint about the reason for using stat() instead
 of fstat()?

 Christian

 --=.K'1jRdUVZuuEHf
 Content-Type: application/pgp-signature

 -----BEGIN PGP SIGNATURE-----
 Version: GnuPG v1.2.1 (NetBSD)

 iD8DBQE+GzP60KQix3oyIMcRAnFWAKC+2b5Ve5Oe3WKgL7dMc/E7rcKCZACfSAg5
 w5939ge0apUuzTVEKwwev18=
 =fs23
 -----END PGP SIGNATURE-----

 --=.K'1jRdUVZuuEHf--

From: Giorgos Keramidas <keramida@freebsd.org>
To: Christian Biere <christianbiere@gmx.de>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19722: fix for 3 uncompress(1) bugs that delete/truncate the wrong file
Date: Wed, 8 Jan 2003 00:18:13 +0200

 On 2003-01-07 21:09, Christian Biere <christianbiere@gmx.de> wrote:
 > Giorgos Keramidas <keramida@freebsd.org> wrote:
 > >  	if ((ifp = zopen(in, "r", bits)) == NULL) {
 > >  		cwarn("%s", in);
 > > -		goto err;
 > > +		return;
 > >  	}
 >
 > Isn't *here* a race condition?

 Hmmm, why?  Can you elaborate a bit?

 The only difference this part has from the err: labeled snipet of
 decompress() is that at this point, neither input nor output files are
 open, so we don't need to run the current error handling, which is:

 	if (ofp) {
                 if (oreg)
                         (void)unlink(out);
                 (void)fclose(ofp);
         }
         if (ifp)
                 (void)fclose(ifp);

 Nothing of this part needs to run until at least one of the streams is
 open.  There is no race condition that wasn't there before :-/

 > >  	if (!isstdin) {
 > >  		if (stat(in, &sb)) {
 > >  			cwarn("%s", in);
 > > -			goto err;
 > > +			(void)fclose(ifp);
 > > +			return;
 > >  		}
 > >  		if (!S_ISREG(sb.st_mode))
 > >  			isreg = 0;
 > > @@ -337,6 +333,21 @@
 > >  			isreg = 1;
 > >  	} else
 > >  		isreg = 0;
 >
 > Could someone give me a hint about the reason for using stat() instead
 > of fstat()?

 The zopen() function returns a FILE* but the real fp from stdin is not
 the same as the FILE* returned by zopen().  The real stdio FILE* is
 saved in zs->zs_fp.  Instead of breaking the clean interface of
 zopen.c and abusing our knowledge of the way it works internally, it's
 cleaner and easier to just use stat().

 - Giorgos


From: Christian Biere <christianbiere@gmx.de>
To: Giorgos Keramidas <keramida@freebsd.org>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19722: fix for 3 uncompress(1) bugs that delete/truncate
 the wrong file
Date: Wed, 8 Jan 2003 11:46:15 +0100

 Giorgos Keramidas <keramida@freebsd.org> wrote:

 > On 2003-01-07 21:09, Christian Biere <christianbiere@gmx.de> wrote:
 > There is no race condition that wasn't there before :-/

 Right.

 > The zopen() function returns a FILE* but the real fp from stdin is not
 > the same as the FILE* returned by zopen().  The real stdio FILE* is
 > saved in zs->zs_fp.  Instead of breaking the clean interface of
 > zopen.c and abusing our knowledge of the way it works internally, it's
 > cleaner and easier to just use stat().

 It might be easier and *look* cleaner but there's no guarantee that you
 stat() the same file you've opened with zopen() because you use the
 filename to identify the file. It might be a flaw of the interface that
 there's no"clean" way to use stat(). I wonder why one would prefer
 "clean" over safe although both should be the case.

 Christian

From: Giorgos Keramidas <keramida@ceid.upatras.gr>
To: Christian Biere <christianbiere@gmx.de>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19722: fix for 3 uncompress(1) bugs that delete/truncate the wrong file
Date: Wed, 8 Jan 2003 14:24:37 +0200

 On 2003-01-08 11:46, Christian Biere <christianbiere@gmx.de> wrote:
 > Giorgos Keramidas <keramida@freebsd.org> wrote:
 > > The zopen() function returns a FILE* but the real fp from stdin is not
 > > the same as the FILE* returned by zopen().  The real stdio FILE* is
 > > saved in zs->zs_fp.  Instead of breaking the clean interface of
 > > zopen.c and abusing our knowledge of the way it works internally, it's
 > > cleaner and easier to just use stat().
 >
 > It might be easier and *look* cleaner but there's no guarantee that you
 > stat() the same file you've opened with zopen() because you use the
 > filename to identify the file.

 Right.

 > It might be a flaw of the interface that there's no"clean" way to
 > use stat(). I wonder why one would prefer "clean" over safe although
 > both should be the case.

 We could probably add a zstat() function to zopen.c that uses fstat()
 on zs->zs_fp.  This is something that belongs to another PR maybe?

From: Roland Illig <rillig@NetBSD.org>
To: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19722: fix for 3 uncompress(1) bugs that delete/truncate the wrong file
Date: Sun, 22 May 2022

 Module Name:   src
 Committed By:  rillig
 Date:          Sun May 22 17:55:09 UTC 2022

 Modified Files:
        src/distrib/sets/lists/tests: mi
        src/etc/mtree: NetBSD.dist.tests
        src/tests/usr.bin: Makefile
 Added Files:
        src/tests/usr.bin/compress: Makefile t_pr_19722.sh

 Log Message:
 tests/compress: demonstrate truncation of target file

 Reported by Giorgos Keramidas in PR bin/19722.


 To generate a diff of this commit:
 cvs rdiff -u -r1.1205 -r1.1206 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.191 -r1.192 src/etc/mtree/NetBSD.dist.tests
 cvs rdiff -u -r1.35 -r1.36 src/tests/usr.bin/Makefile
 cvs rdiff -u -r0 -r1.1 src/tests/usr.bin/compress/Makefile \
      src/tests/usr.bin/compress/t_pr_19722.sh

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: bin-bug-people->rillig
Responsible-Changed-By: rillig@NetBSD.org
Responsible-Changed-When: Sat, 21 May 2022 23:53:02 +0000
Responsible-Changed-Why:
Still reproducible as of NetBSD 9.99.x, I'll handle it.


State-Changed-From-To: open->closed
State-Changed-By: rillig@NetBSD.org
State-Changed-When: Sun, 22 May 2022 21:42:35 +0000
State-Changed-Why:
Fixed for NetBSD 10. Thanks for the PR.


 https://mail-index.netbsd.org/source-changes/2022/05/22/msg138786.html
 https://mail-index.netbsd.org/source-changes/2022/05/22/msg138787.html
 https://mail-index.netbsd.org/source-changes/2022/05/22/msg138790.html

>Unformatted:

Home	PR Database Search