NetBSD Problem Report #19849

Received: (qmail 18864 invoked by uid 605); 14 Jan 2003 10:35:46 -0000
Message-Id: <20030114113543.6dc26a60.christianbiere@gmx.de>
Date: Tue, 14 Jan 2003 11:35:43 +0100
From: Christian Biere <christianbiere@gmx.de>
Sender: gnats-bugs-owner@netbsd.org
To: gnats-bugs@gnats.netbsd.org
Subject: Race conditions in compress

>Number:         19849
>Category:       bin
>Synopsis:       Race conditions in compress
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 14 10:36:00 +0000 2003
>Closed-Date:    
>Last-Modified:  Thu Jan 23 19:08:01 +0000 2003
>Originator:     Christian Biere
>Release:        NetBSD 1.6K
>Organization:
>Environment:

>Description:

By reading PR bin/19722 I discovered

/usr/src/usr.bin/compress/compress.c:

In compress:

        if (!isstdout) {
                exists = !stat(out, &sb);
                if (!force && exists && S_ISREG(sb.st_mode) &&
!permission(out))                        return;
                oreg = !exists || S_ISREG(sb.st_mode);
        } else
                oreg = 0;

IMHO, it's better to open the file and use ENOENT == errno to decide
whether the file exists or not.

        if ((ifp = fopen(in, "r")) == NULL) {
                cwarn("%s", in);
                return;
        }

IMO here's a real race condition:

        if (!isstdin) {
                if (stat(in, &isb)) {           /* DON'T FSTAT! */
                        cwarn("%s", in);
                        goto err;
                }

Why "DON'T FSTAT"? This would be the only way to prevent a race
condition. What's the reason for not using "fstat(fileno(ifp), &isb)"?
If you don't, there's no guarantee that you stat() the same file you've
opened with fopen() before.

Another example for the same kind of race condition:

        if (fclose(ofp)) {
                cwarn("%s", out);
                goto err;
        }
        ofp = NULL;

        if (isreg && oreg) {
                if (stat(out, &sb)) {
                        cwarn("%s", out);
                        goto err;

[...]

Here, compress might modify the mode for the wrong file as it's only
identified by its name:

	setfile(out, &isb);

In decompress:

Again, the file used with ofp is not guaranteed to be the same out
points to.

        if (isreg && oreg) {
                setfile(out, &sb);

>How-To-Repeat:


>Fix:

Giorgos Keramidas suggested to add a function zstat() to the zopen
interface. This would keep the code clean and wouldn't break the
interface. 
>Release-Note:
>Audit-Trail:

From: Hubert Feyrer <hubert.feyrer@informatik.fh-regensburg.de>
To: Christian Biere <christianbiere@gmx.de>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19849: Race conditions in compress
Date: Tue, 14 Jan 2003 12:38:45 +0100 (MET)

 On Tue, 14 Jan 2003, Christian Biere wrote:
 >                 if (stat(in, &isb)) {           /* DON'T FSTAT! */

 I think the idea here is to not (try to) compress symlinks.


  - Hubert

 -- 
 Want to get a clue on IPv6 but don't know where to start? Try this:
 * Basics -> http://www.onlamp.com/pub/a/onlamp/2001/05/24/ipv6_tutorial.html
 * Setup  -> http://www.onlamp.com/pub/a/onlamp/2001/06/01/ipv6_tutorial.html
 Of course with your #1 IPv6 ready operating system -> http://www.NetBSD.org/

From: David Laight <david@l8s.co.uk>
To: netbsd-bugs@netbsd.org
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19849: Race conditions in compress
Date: Wed, 15 Jan 2003 17:47:10 +0000

 > IMHO, it's better to open the file and use ENOENT == errno to decide
 > whether the file exists or not.
 > 
 >         if ((ifp = fopen(in, "r")) == NULL) {
 >                 cwarn("%s", in);
 >                 return;
 >         }

 You need to do the underlying open with O_NONBLOCK set just in
 case a char special is specified (which might block - eg a tty
 line waiting for carrier).

 This starts getting hard if you want (need?) to keep portability
 with non-unix systems.

 OTOH there are an awful lot of these sort of race conditions
 lurking, and fixing them will do no harm.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: Christian Biere <christianbiere@gmx.de>
To: Hubert Feyrer <hubert.feyrer@informatik.fh-regensburg.de>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19849: Race conditions in compress
Date: Thu, 23 Jan 2003 20:06:25 +0100

 Hubert Feyrer <hubert.feyrer@informatik.fh-regensburg.de> wrote:

 > I think the idea here is to not (try to) compress symlinks.

 I was told, the following would be an appropriate method to prevent
 accessing symlinks in this case:

 Use fstat() to get the (effective) device id and inode, then use lstat()
 to determine whether the file is a symlink, compare device id and inode
 to check whether the file was exchanged between both calls.

 Christian
>Unformatted:

Home	PR Database Search