[net.bugs.v7] Bug in link

dvk (08/10/82)

Here's a fun one for all you folks in hack-land...

We have a program that tries to rename directories. (For those  who  care,
it  is  the "mv" program, but it could be anything). This program does the
standard "link to a new name, unlink the old name" to rename.  Now  if  we
run  this as root, all is fine, since *only* root can link to a directory.
If we run it as a regular user, the program fails. Now, just failing would
not  be  such a big deal, but, to our dismay, our directories got trashed.
Why? Because the link count got decremented each  time  someone  tried  to
rename  "foo",  until "foo" just disappeared!  Rather than simply make our
program mode 4755, owned by root, we figured it would be  more  reasonable
to  fix  an  obvious  OS  bug. In sys2/link(), there is code that reads as
follows:
==========================================================================

link() {
		<mumble, mumble>

	ip = namei(uchar, 0);
	if (ip == NULL)
		return;
	if ((ip->i_mode & IFMT)==IFDIR && !suser())
		goto out;
	ip->i_nlink++;
	ip->i_flag |= ICHG;

		<mumble, mumble>

out:
	if (u.u_error) {
		ip->i_nlink--;
		ip->i_flag |= ICHG;
		}
	iput(ip);
}
==========================================================================

The problem is that if "namei" detects an error, but does not return NULL,
then  the  link  count, which was unincremented, is decremented anyway. We
don't know why "namei" sets u.u_error, but apparently it does. The  simple
fix is to not decrement the link count, but instead branch directly to the
call to iput. Setting u.u_error to EPERM is a flourish we couldn't resist.

==========================================================================
link() {
		<mumble, mumble>

	ip = namei(uchar, 0);
	if (ip == NULL)
		return;
	if ((ip->i_mode & IFMT)==IFDIR && !suser()) {
		u.u_error = EPERM;
		goto out1;			/* Note new label name! */
		}

		<mumble, mumble>

out:
	if (u.u_error) {
		ip->i_nlink--;
		ip->i_flag |= ICHG;
		}
out1:
	iput(ip);
}
==========================================================================

		Happy hacking!

	Daniel Klein and Tron McConnell, Mellon Institute, Pittsburgh

swatt (08/12/82)

I've just checked our 4.1bsd sources; they have made the same fix.
Note that it is NOT namei which sets u.u_error, but suser.  This is
quite reasonable and it does set it to EPERM.  I've also just checked
an old listing of V7 sources (existed on disk on Jan 10, 1979).  Their
strategy was a bit different and didn't have this error.  Do you
happen to know the geneology of your sources?

It appears the change was intended to eliminate a race condition
involving someone unlinking a directory just as someone else was
linking to it.  The old code had to unlock the inode for the first
directory before calling namei on the second (if the second had a path
through the first, the namei would hang). The new code first updates
the first directory inode with the new link count, unlocks the inode,
and then calls namei on the second name.  Thus if someone else comes
along and unlinks the first name, the inode is not flushed because its
link count is still non-zero.  This of course requires a later
decrementing of the inode in case some error prevents the successful
link operation.

	- Alan S. Watt
	[decvax!]ittvax!swatt

dmr (08/13/82)

The analysis by Alan Watt of the link bug is correct.

The code shown by mi-cec!dvk does have the bug.

V7 does not have it, though it does have a fairly innocuous race.

4BSD does not have it, and it fixes the race correctly.

I doubt if System III has it, since I just looked at a successor
and it is like V7.

So the bug has, I hope, narrow distribution.  It is a serious one.

		Dennis Ritchie