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:
(Contact us)
$NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.