Re: [BUG] Generic syscalls -- chmod vs. fchmodat

My feeling is that it should be in glibc: as Mike mentioned, we don't normally
change the behavior of existing system calls unless they are obviously
broken to start with. If we want to keep fchmodat getting the implicit
"." directory, and at the same time keep fchmod returning an error, the fchmod
wrapper around fchmodat is the only place that can enforce this.

My point was that it's quite arguable that the *at syscall interfaces were
broken to begin with.  I've never seen anything suggesting their intent was
other than to permit relative pathnames, and the empty string has never
been a valid relative pathname.  To fit the POSIX requirements as I read
them, the *at functions must refuse to resolve the empty string.  So if the
kernel does not change and my interpretation of POSIX stands, then libc
must wrap all the *at syscalls with a function that checks for the empty
string and fails with ENOENT as a special case.

I don't have any strong opinion about this subject, but it makes the most
sense to me for the kernel's behavior to change.  I know of no reason to
think that the current treatment of the empty string was ever intended at
the creation of the *at interfaces.


Thanks,
Roland

On Tuesday 25 January 2011 18:45:15 Roland McGrath wrote:

 I know of no reason to
think that the current treatment of the empty string was ever intended at
the creation of the *at interfaces.

I always assumed that this was done so that the *at syscalls can replace
both the ones that take a file descriptor (e.g. fstat) and the ones that
take a pathname (e.g. stat), which is sensible for the non-AT_FDCWD case,
although not documented in the man pages.

Treating the empty string special for AT_FDCWD is rather pointless, but
at least consistent.

	Arnd

On Tuesday, January 25, 2011 13:21:14 Arnd Bergmann wrote:

On Tuesday 25 January 2011 18:45:15 Roland McGrath wrote:

quoted

 I know of no reason to

think that the current treatment of the empty string was ever intended at
the creation of the *at interfaces.

I always assumed that this was done so that the *at syscalls can replace
both the ones that take a file descriptor (e.g. fstat) and the ones that
take a pathname (e.g. stat), which is sensible for the non-AT_FDCWD case,
although not documented in the man pages.

Treating the empty string special for AT_FDCWD is rather pointless, but
at least consistent.

i dont know if the gnulib peeps are on these lists, but i think their 
implementations of some of the *at funcs leverage the extended behavior that 
is available under Linux.  or at least, i'm certain they'll have some insight 
into some of these nuances.
-mike

On 01/25/2011 11:52 AM, Mike Frysinger wrote:

On Tuesday, January 25, 2011 13:21:14 Arnd Bergmann wrote:

quoted

On Tuesday 25 January 2011 18:45:15 Roland McGrath wrote:

quoted

 I know of no reason to

think that the current treatment of the empty string was ever intended at
the creation of the *at interfaces.

I always assumed that this was done so that the *at syscalls can replace
both the ones that take a file descriptor (e.g. fstat) and the ones that
take a pathname (e.g. stat), which is sensible for the non-AT_FDCWD case,
although not documented in the man pages.

For futimesat, which is not specified by POSIX, gnulib already requires
the behavior of calling futimesat(fd, NULL, times) as a way to directly
modify the (possibly non-directory) fd, in contrast to futimesat(fd,
"name", times), which sets the times on "name" relative to the directory fd.

For all other *at interfaces, they are specified by POSIX as requiring
the same behavior as the non-*at interface when given arguments within
the bounds required by POSIX.  That is, fstatat(fd, "", buf, 0) SHALL
fail with ENOENT, just the same as stat("", buf).  However, since POSIX
requires that the second argument be a valid string, and NULL is not a
valid string, then passing NULL as the second argument means that you
are no longer bound by POSIX and that you could (if you wanted) make
fstatat(fd, NULL, buf, 0) behave the same as fstat(fd, buf).  Right now,
none of the other *at interfaces in Linux currently do this; futimesat
is the only interface explicitly documented as having this dual behavior.

quoted

Treating the empty string special for AT_FDCWD is rather pointless, but
at least consistent.

No, treating an empty string name argument to a *at function as a
synonym for AT_FDCWD is a violation of POSIX.

i dont know if the gnulib peeps are on these lists, but i think their 
implementations of some of the *at funcs leverage the extended behavior that 
is available under Linux.  or at least, i'm certain they'll have some insight 
into some of these nuances.

Gnulib has code to explicitly work around bugs in earlier glibc/Linux
implementations that mistakenly treated fd, "" the same as fd, "." (at
least modern kernels get it right, and when glibc defers to the kernel,
those workarounds in gnulib are not needed on newer systems).

-- 
Eric Blake   eblake-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

On 01/25/2011 12:56 PM, Eric Blake wrote:

quoted

quoted

Treating the empty string special for AT_FDCWD is rather pointless, but
at least consistent.

No, treating an empty string name argument to a *at function as a
synonym for AT_FDCWD is a violation of POSIX.

quoted

i dont know if the gnulib peeps are on these lists, but i think their 
implementations of some of the *at funcs leverage the extended behavior that 
is available under Linux.  or at least, i'm certain they'll have some insight 
into some of these nuances.

Gnulib has code to explicitly work around bugs in earlier glibc/Linux
implementations that mistakenly treated fd, "" the same as fd, "." (at
least modern kernels get it right, and when glibc defers to the kernel,
those workarounds in gnulib are not needed on newer systems).

One other thing to point out - this is not the first time glibc has
added code around *at kernel syscalls in order to provide POSIX
semantics where the Linux syscall does not.  Remember that both futimens
and utimensat are implemented on top of the same syscall, and that
futimens(AT_FDCWD, times) must fail rather than set the times on ".".
See glibc commit 4286fa41 where glibc had to add code to work around the
kernel's choice of the syscall utimensat(fd, NULL, times, 0) as being
the way to implement futimens, in response to
http://sourceware.org/bugzilla/show_bug.cgi?id=10992, which was raised
because gnulib detected the POSIX compliance bug, in much the same way
that gnulib is now detecting the chmod("") bug of not failing with
ENOENT when chmod is incorrectly implemented around the sys_fchmodat
syscall.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

On 01/25/2011 01:31 PM, Eric Blake wrote:

One other thing to point out - this is not the first time glibc has
added code around *at kernel syscalls in order to provide POSIX
semantics where the Linux syscall does not.  Remember that both futimens
and utimensat are implemented on top of the same syscall, and that
futimens(AT_FDCWD, times) must fail rather than set the times on ".".

I guess an executive summary of the issue, from my point of view, is
that I like the idea of making both simple versions of a command call
into the same *at version of the syscall [chmod via
sys_fchmodat(AT_FDCWD,name), and fchmod via sys_fchmodat(fd, NULL)],
provided that:

1. chmod("") must continue to fail with ENOENT (here, it might make
sense for the kernel to do the filtering, since there is an obvious
difference between "" and NULL; but if the kernel call does not change,
then the burden is on glibc instead)

2. fchmod(AT_FDCWD) must continue to fail with EBADF (here, it might
make sense for the kernel to do the filtering - if the name argument is
NULL, then the fd argument must be non-negative; but precedence with
futimens vs. sys_utimensat puts the burden on glibc instead)

3. chmod(NULL) is undefined.  Currently it fails with EFAULT, but I see
no reason why it can't start failing with EBADF (assuming the kernel
does filtering as for point 2) or start operating on ".", because it's
only a buggy program that would be making that call in the first place
(actually, failing with EBADF is a little safer, as silent conversion
between two types of failures is a bit easier to audit for consequences
than is silent conversion from error to success).

4. fchmodat(fd, NULL) is undefined.  For non-negative fd, POSIX allows
failure, but it also allows behaving like fchmod.  Portable programs
can't rely on a particular behavior, but glibc is more than welcome to
rely on a particular syscall behavior for implementing the simpler
functions.

5. fchmodat(fd, "") must fail with ENOENT.  For non-negative fd,
apparently the kernel currently modifies fd, although point 1 says it
might make more sense to fail with ENOENT so that glibc doesn't have to
do as much work in chmod and fchmodat (if the kernel call does not
change, then the burden is on glibc).

6. fchmodat(AT_FDCWD, NULL) is undefined.  For AT_FDCWD, the kernel
currently modifies ".", although point 2 says it might make more sense
to fail with EBADF so that glibc doesn't have to do as much work in fchmod.

11 more lines

Hi,

On 25 January 2011 15:32, Eric Blake [off-list ref] wrote:

7. fchmodat(AT_FDCWD, "") must fail with ENOENT.  Apparently, the kernel
currently modifies ".", although point 1 says it might make more sense
to fail with ENOENT so that glibc doesn't have to do as much work in
chmod and fchmodat (if the kernel call does not change, then the burden
is on glibc).

Interesting discussion, but I'm very embarrassed to say I started it
due to a mis-diagnosis of a bug :-(

In fact, the current kernels *do* check for an empty string, and do so
in order to maintain POSIX compliance, per Eric's points 5 and 7.
See fs/namei.c in do_getname, circa line 114,  where we read:

 * POSIX.1 2.4: an empty pathname is invalid (ENOENT).

This is backed up with code, circa line 129 ff:

   retval = strncpy_from_user(page, filename, len);
...
else if (!retval)
      retval = -ENOENT;
   return retval;

So an empty string will have length zero, and the ENOENT is indeed
set.

This handles cases 5 & 7.  I've no idea how any of the other cases
work out.

Note that getname() is used in a variety of places; its not just chmod,
but also in symlinks and many others.

--linas

On Tue, Jan 25, 2011 at 09:45:15AM -0800, Roland McGrath wrote:

the empty string has never been a valid relative pathname.

Hmm. I definitely recall otherwise.

The old Unix definition is that the empty string stands for "."
so that 'ls ""' means the same as 'ls .'
and 'ls /tmp/""' the same as 'ls /tmp/.'.

Let me try.

On a recent Linux system:

% ls -l ""
ls: cannot access : No such file or directory

On an old Unix system:

# ls -l ""
drwxr-xr-x  2 bin      1040 Jan  1  1970 bin
drwxr-xr-x  2 bin       352 Jan  1  1970 dev
drwxr-xr-x  2 bin       304 Aug 20 12:39 etc
...

Andries