Bug in unlink error return

Bug in unlink error return

[Andries.Brouwer]

Someone complained a moment ago about the error return in unlink.
And indeed, it used to be correct but since 2.1.132 we return a
buggy (or at least non-POSIX) error for unlink(directory).

Just changed the man page to say

unlink(2)
...
       EPERM  The system does not allow unlinking of directories,
              or unlinking  of  directories  requires  privileges
              that  the  current  process doesn't have.  (This is
              the POSIX prescribed error return.)

       EISDIR pathname refers to a directory.  (This is the  non-
              POSIX value returned by Linux since 2.1.132.)
...

Probably this should be fixed again, both in 2.2 and 2.4.
2.0 is still correct (I checked only ext2).

Andries


[The EISDIR is correct for rename(), and the cleanup that
made a nice uniform may_delete() in namei.c introduced this bug.
The very simple but slightly ugly fix is to write (in vfs_unlink)
	error = may_delete(dir, dentry, 0);
	if (error == -EISDIR)
		error = -EPERM;
]

Re: Bug in unlink error return

[Alan Cox]

>        EISDIR pathname refers to a directory.  (This is the  non-
>               POSIX value returned by Linux since 2.1.132.)

it isnnt that simple -ac does the right thing now I believe

> Probably this should be fixed again, both in 2.2 and 2.4.
> 2.0 is still correct (I checked only ext2).

I'll check 2.2.20pre and fix it if so.

Alan

Re: Bug in unlink error return

[Alexander Viro]

On Thu, 17 May 2001 Andries.Brouwer@cwi.nl wrote:

> Someone complained a moment ago about the error return in unlink.
> And indeed, it used to be correct but since 2.1.132 we return a
> buggy (or at least non-POSIX) error for unlink(directory).
 
> [The EISDIR is correct for rename(), and the cleanup that
> made a nice uniform may_delete() in namei.c introduced this bug.
> The very simple but slightly ugly fix is to write (in vfs_unlink)
> 	error = may_delete(dir, dentry, 0);
> 	if (error == -EISDIR)
> 		error = -EPERM;
> ]
 
IMO that's the case of POSIX being misapplied. Rationale:

	* historically, unlink(2) _did_ work on directories. It was root-only
and thus normal error value was EPERM. It was inherently racy - rmdir(1) was
implemented (suid-root) as 3 calls of unlink(2) (., .. and link from parent),
but it lacked atomicity and could leave corrupted filesystem if killed in
the middle of operation.

	* introduction of rmdir(2) made unlink(2) on directories not only
dangerous, but completely unnecessary. Eventually, operation became prohibited
for everyone.

	* rename(2) is BSDism, especially - overwriting one (dmr considered
it as a bad idea and in the hindsight he was absolutely right on that). In
case of mismatch they (most likely - Kirk) had chosen to use more informative
error value. They could use EPERM, but since nobody was permitted to do
that it would be rather silly.

	Notice that situation with unlink(2) is the same. It's not a matter
of insufficient permissions - operation is simply not allowed, whoever you
are.

	In other words, POSIX describes behaviour that used to make sense on
systems with different semantics of unlink(2). Notice that standard meaning
of EPERM is "privileges are insufficient for requested operation". No longer
accurate in that case and had been inaccurate for many years.

	We can revert to returning EPERM in that case. However, if you could
get Austin Group to accept EISDIR as legitimate error value for that case
the world would make more sense. We had this behaviour for quite a while.
So far userland seems to be OK with that and it definitely makes more sense
than EPERM.

	Moreover, it's consistent with behaviour of rmdir() and rename() -
both use ENOTDIR/EISDIR to report the type mismatch for victim.

Re: Bug in unlink error return

[Andries.Brouwer]

> IMO that's the case of POSIX being misapplied. Rationale:
> * historically, ...

Yes, I know all about that.
Nevertheless the facts are here:

       EPERM  The system does not allow unlinking of directories,
              or unlinking  of  directories  requires  privileges
              that  the  current  process doesn't have.  (This is
              the POSIX prescribed error return.)

       EISDIR pathname refers to a directory.  (This is the  non-
              POSIX value returned by Linux since 2.1.132.)

At first I wrote "buggy" instead of "non-POSIX", but in fact
I prefer EISDIR myself. On the other hand, Linux follows POSIX,
even in the cases where we don't like POSIX very much.

Btw - this change in 2.1.132 actually broke programs, so
at that time is was really the introduction of a bug.

Andries