NetBSD Problem Report #35619

From www@NetBSD.org  Thu Feb 15 23:41:52 2007
Return-Path: <www@NetBSD.org>
Received: by narn.NetBSD.org (Postfix, from userid 31301)
	id 76D5763BCA6; Thu, 15 Feb 2007 23:41:52 +0000 (UTC)
Message-Id: <20070215234152.76D5763BCA6@narn.NetBSD.org>
Date: Thu, 15 Feb 2007 23:41:52 +0000 (UTC)
From: uwe@NetBSD.org
Reply-To: uwe@NetBSD.org
To: gnats-bugs@NetBSD.org
Subject: unlink+fopen race in makewhatis
X-Send-Pr-Version: www-1.0

>Number:         35619
>Category:       bin
>Synopsis:       unlink+fopen race in makewhatis
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    dholland
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Feb 15 23:45:00 +0000 2007
>Closed-Date:    Sun Nov 16 06:26:46 +0000 2008
>Last-Modified:  Sun Nov 16 06:26:46 +0000 2008
>Originator:     Valery Ushakov
>Release:        any
>Organization:
>Environment:
>Description:
makewhatis() uses

	(void)unlink(whatisdb);
	if ((out = fopen(whatisdb, "w")) == NULL)
		err(EXIT_FAILURE, "Cannot open `%s'", whatisdb);

to create the whatis database.  If two hosts share the man dir
via NFS and if both run makehwatis, there is a race that might
cause two instances of makewhatis to dump whatis database into
the same file.

Even if we don't consider the race, there's a window in which the
whatis database is incompelte (as makewhatis is still running).

>How-To-Repeat:
A smaller example:

    host1$ (echo A; sleep 10; echo B) > a &
    host2$ (echo 1; sleep 10; echo 2) > a &

There's a window in which you can see a to contain "1\bB\b"

>Fix:
makewhatis should use temp file + rename(2) so that the whatis database
is consistent at all times.

>Release-Note:

>Audit-Trail:
From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/35619 CVS commit: src/libexec/makewhatis
Date: Tue, 27 May 2008 01:25:46 +0000 (UTC)

 Module Name:	src
 Committed By:	dholland
 Date:		Tue May 27 01:25:46 UTC 2008

 Modified Files:
 	src/libexec/makewhatis: makewhatis.c

 Log Message:
 Update whatis.db atomically, by dumping out into whatis.db.new and
 renaming. PR 35619 from uwe@.


 To generate a diff of this commit:
 cvs rdiff -r1.41 -r1.42 src/libexec/makewhatis/makewhatis.c

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

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/35619 CVS commit: src/libexec/makewhatis
Date: Tue, 27 May 2008 01:42:40 +0000 (UTC)

 Module Name:	src
 Committed By:	dholland
 Date:		Tue May 27 01:42:40 UTC 2008

 Modified Files:
 	src/libexec/makewhatis: makewhatis.c

 Log Message:
 Urgh, part of another patch for PR 35619 (which is for after the freeze)
 snuck in by accident, corrupting an error message. Fix.


 To generate a diff of this commit:
 cvs rdiff -r1.42 -r1.43 src/libexec/makewhatis/makewhatis.c

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

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org, uwe@NetBSD.org
Cc: 
Subject: Re: PR/35619 CVS commit: src/libexec/makewhatis
Date: Tue, 27 May 2008 02:21:19 +0000

 On Tue, May 27, 2008 at 01:45:01AM +0000, David A. Holland wrote:
  > Urgh, part of another patch for PR 35619 (which is for after the freeze)
  > [...]

 Here's that patch, which I'll commit after the freeze is over. This
 should thoroughly take care of the race condition.

 diff -r f75028e978ef makewhatis.c
 --- a/makewhatis.c	Mon May 26 21:52:34 2008 -0400
 +++ b/makewhatis.c	Mon May 26 22:24:53 2008 -0400
 @@ -248,6 +248,8 @@
  	whatis	*dest;
  	FILE	*out;
  	size_t	sdoff, sdlen;
 +	int	outfd;
 +	struct stat st_before, st_after;

  	if ((fts = fts_open(manpath, FTS_LOGICAL, NULL)) == NULL)
  		err(EXIT_FAILURE, "Cannot open `%s'", *manpath);
 @@ -328,15 +330,74 @@
  	if (chdir(manpath[0]) == -1)
  		err(EXIT_FAILURE, "Cannot change dir to `%s'", manpath[0]);

 -	(void)unlink(whatisdb_new);
 -	if ((out = fopen(whatisdb_new, "w")) == NULL)
 +	/*
 +	 * makewhatis runs unattended, so it needs to be able to
 +	 * recover if the last run crashed out. Therefore, if
 +	 * whatisdb_new exists and is more than (arbitrarily) sixteen
 +	 * hours old, nuke it. If it exists but is not so old, refuse
 +	 * to run until it's cleaned up, in case another makewhatis is
 +	 * already running. Also, open the output with O_EXCL to make
 +	 * sure we get our own, in case two copies start exactly at
 +	 * once. (Unlikely? Maybe, maybe not, if two copies of cron
 +	 * end up running.)
 +	 *
 +	 * Similarly, before renaming the file after we finish writing
 +	 * to it, make sure it's still the same file we opened. This
 +	 * can't be completely race-free, but getting caught by it
 +	 * would require an unexplained sixteen-hour-or-more lag
 +	 * between the last mtime update when we wrote to it and when
 +	 * we get to the stat call *and* another makewhatis starting
 +	 * out to write at exactly the wrong moment. Not impossible,
 +	 * but not likely enough to worry about.
 +	 *
 +	 * This is maybe unnecessarily elaborate, but generating
 +	 * corrupted output isn't so good either.
 +	 */
 +
 +	if (stat(whatisdb_new, &st_before) == 0) {
 +		if (st_before.st_mtime - time(NULL) > 16*60*60) {
 +			/* Don't complain if someone else just removed it. */
 +			if (unlink(whatisdb_new) == -1 && errno != ENOENT) {
 +				err(EXIT_FAILURE, "Could not remove `%s'",
 +				    whatisdb_new);
 +			} else {
 +				warnx("Removed stale `%s'", whatisdb_new);
 +			}
 +		} else {
 +			errx(EXIT_FAILURE, "The file `%s' already exists "
 +			    "-- am I already running?", whatisdb_new);
 +		}
 +	} else if (errno != ENOENT) {
 +		/* Something unexpected happened. */
 +		err(EXIT_FAILURE, "Cannot stat `%s'", whatisdb_new);
 +	}
 +
 +	outfd = open(whatisdb_new, O_WRONLY|O_CREAT|O_EXCL,
 +	    S_IRUSR|S_IRGRP|S_IROTH);
 +	if (outfd < 0)
  		err(EXIT_FAILURE, "Cannot open `%s'", whatisdb_new);

 +	if (fstat(outfd, &st_before) == -1)
 +		err(EXIT_FAILURE, "Cannot fstat `%s'", whatisdb_new);
 +
 +	if ((out = fdopen(outfd, "w")) == NULL)
 +		err(EXIT_FAILURE, "Cannot fdopen `%s'", whatisdb_new);
 +
  	dumpwhatis(out, dest);
  	if (fchmod(fileno(out), S_IRUSR|S_IRGRP|S_IROTH) == -1)
  		err(EXIT_FAILURE, "Cannot chmod `%s'", whatisdb_new);
  	if (fclose(out) != 0)
  		err(EXIT_FAILURE, "Cannot close `%s'", whatisdb_new);
 +
 +	if (stat(whatisdb_new, &st_after) == -1)
 +		err(EXIT_FAILURE, "Cannot stat `%s' (after writing)",
 +		    whatisdb_new);
 +
 +	if (st_before.st_dev != st_after.st_dev ||
 +	    st_before.st_ino != st_after.st_ino) {
 +		errx(EXIT_FAILURE, "The file `%s' changed under me; giving up",
 +		    whatisdb_new);
 +	}

  	if (rename(whatisdb_new, whatisdb) == -1)
  		err(EXIT_FAILURE, "Could not rename `%s' to `%s'",



 -- 
 David A. Holland
 dholland@netbsd.org

Responsible-Changed-From-To: bin-bug-people->dholland
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Tue, 27 May 2008 02:29:03 +0000
Responsible-Changed-Why:
I've prepared patches.


From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/35619 CVS commit: src/libexec/makewhatis
Date: Sun, 16 Nov 2008 06:17:05 +0000 (UTC)

 Module Name:	src
 Committed By:	dholland
 Date:		Sun Nov 16 06:17:05 UTC 2008

 Modified Files:
 	src/libexec/makewhatis: makewhatis.c

 Log Message:
 Close possible race conditions if multiple copies of makewhatis end up
 running concurrently. Other half of the fix for PR 35619.


 To generate a diff of this commit:
 cvs rdiff -r1.44 -r1.45 src/libexec/makewhatis/makewhatis.c

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

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 16 Nov 2008 06:26:46 +0000
State-Changed-Why:
fixed.


>Unformatted:

Home	PR Database Search