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

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

[Arnd Bergmann]

On Monday 24 January 2011, Linas Vepstas wrote:
> Chris, Arnd, all,

[adding linux-api list]

> Found a bug/incompatibility in the generic syscalls chmod implementation;
> not sure if this is a kernel bug or a glibc bug, or how to correctly resolve it.
> 
> The new "generic chmod" implementation for glibc sends chmod to the
> kernel call sys_fchmodat with  AT_FDCWD, instead of using the older
> "deprecated" chmod syscall.  These two behave slightly differently:  with
> the new implementation, the file "" (i.e. string of length zero) gets
> interpreted
> as . and so the syscall succeeds, setting perms on .  The old syscall would
> return an errno=2 No such file or directory for this filename.
> 
> My gut instinct is that this is a kernel bug, but am not so sure; perhaps this
> is "working as designed".  I thought of submitting a patch to fs/namei.c to
> fix this, but then got lost in the details: there didn't seem to be
> any particularly
> good place to add this check.  Meanwhile, a glibc test case (posix/tst-chmod.c)
> is failing as a result.
> 
> Should we put a check for this funky non-filename into the glibc
> generic code, or into sys_chmodat? Recommendations?

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.

	Arnd

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

[Roland McGrath]

> 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

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

[Arnd Bergmann]

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

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

[Roland McGrath]

> 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.

I see your point.  That is, having the empty string relative to a file
descriptor work means it can replace f* calls on non-directories, whereas
the standard method of passing "." for descriptor-relative resolution can
only work on a file descriptor open on a directory.  Is that what you mean?

I don't think this was part of the original intent when the calls were
added, but I suppose it makes sense.

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

I agree about the consistency point.  However, one could also call it
consistent if the empty string fails to resolve when operating on either a
directory file descriptor or AT_FDCWD but works on a non-directory file
descriptor.  POSIX does not mandate that *at calls fail with ENOTDIR when
passed a non-directory file descriptor (it's a "may fail" error, not a
"shall fail" error).  So that behavior would be consistent both with the
POSIX requirements as I read them, and with the desire you mentioned to let
the fblahat system call serve to implement fblah as well as blah.  Then
libc would not have to wrap the *at calls with any special check to conform
to POSIX.


Thanks,
Roland

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

[Arnd Bergmann]

On Tuesday 25 January 2011 19:34:37 Roland McGrath wrote:
> I don't think this was part of the original intent when the calls were
> added, but I suppose it makes sense.

More importantly, even if it was never meant this way, anyone could have
assumed that it was and started using the system call in this way.

> > Treating the empty string special for AT_FDCWD is rather pointless, but
> > at least consistent.
> 
> I agree about the consistency point.  However, one could also call it
> consistent if the empty string fails to resolve when operating on either a
> directory file descriptor or AT_FDCWD but works on a non-directory file
> descriptor. 

Yes.

> POSIX does not mandate that *at calls fail with ENOTDIR when
> passed a non-directory file descriptor (it's a "may fail" error, not a
> "shall fail" error).  So that behavior would be consistent both with the
> POSIX requirements as I read them, and with the desire you mentioned to let
> the fblahat system call serve to implement fblah as well as blah.  Then
> libc would not have to wrap the *at calls with any special check to conform
> to POSIX.

Makes sense.

	Arnd

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

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 926 bytes --]

On Tuesday, January 25, 2011 13:21:14 Arnd Bergmann wrote:
> 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.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

On 01/25/2011 11:52 AM, Mike Frysinger wrote:
> On Tuesday, January 25, 2011 13:21:14 Arnd Bergmann wrote:
>> 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.

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.

>>
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On 01/25/2011 12:56 PM, Eric Blake wrote:
>>> 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).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

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.

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).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Linas Vepstas]

Hi,

On 25 January 2011 15:32, Eric Blake <eblake@redhat.com> 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

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

[Andries Brouwer]

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

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

[Roland McGrath]

> 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/.'.

I'm aware of that.  I was talking about Linux and POSIX.


Thanks,
Roland

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

[Andreas Schwab]

Andries Brouwer <aebr@win.tue.nl> writes:

> and 'ls /tmp/""' the same as 'ls /tmp/.'.

/tmp/"" is not an empty file name, and it is indistinguishable from /tmp/ .

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."