Re: Bug in XSHd3 fdopendir

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Geoff Clare on 6/21/2007 2:42 AM:
> address@hidden <address@hidden> wrote, on 20 Jun 2007:
>> @ page 797 line 27082 section fdopendir objection {ebb.fdopendir}
>>
>> Problem:
>>
>> Line 27013 of fdopendir forbids certain use of a file descriptor
>> after handing it to fdopendir.  The example starting at line 27069
>> currently violates this by using dfd in openat and close after the
>> fact.
> 
> I don't think the openat() call is a problem.  The restriction is on
> calls that modify the state of the open file description, and openat()
> does not modify the state.

I agree that the concept of allowing *at() after fdopendir() sounds nice,
particularly since it would imply less pressure on EMFILE.  However, this
is not explicitly stated, and it is incompatible with the existing gnulib
implementation of openat/fdopendir:
http://cvs.savannah.gnu.org/viewvc/gnulib/lib/openat.c?revision=1.17&root=gnulib&view=markup

The gnulib implementation is used to provide an openat emulation on a wide
variety of platforms for at least GNU coreutils, findutils, and tar;
relying on /proc/self/fd when available, otherwise using
savedir/fchdir/opendir(".")/restoredir (and thus no longer being
threadsafe, but it's the best that can be done).  Note that the
implementation closes the original fd during fdopendir, rather than
closedir.  Which means that the subsequent use in openat will fail.
Perhaps that can be worked around in gnulib, by providing a replacement
closedir that tracks whether the DIR was opened via fdopendir, but that
sounds more complex.

If we do not mandate that it is safe to use the *at() functions following
fdopendir, then I think it is safer to go with the dup() approach in the
example.

> 
> The close(dfd) is not forbidden either, because the file descriptor
> has already been closed by the closedir() call.  The close(dfd) will
> just fail with EBADF.

I noticed the close(dfd) first, so that was my original thought, before I
saw the use of dfd in openat.

> 
> I think the only change needed is to remove the bogus close(dfd) call.
> Although it might be worth adding a comment to the closedir() line
> such as:
> 
>     closedir(d);  // note this closes dfd

If we get consensus that using *at on the fd handed to fdopendir must be
safe, then I agree with that change.  But I think we need something along
the lines of one of these further changes at line 27013:

Option 1 - allow *at functions:
Upon successful return from fdopendir( ), the file descriptor is under the
control of the system, and if any attempt is made to close the file
descriptor, or to modify the state of the associated description other
than by means of closedir( ), readdir( ), readdir_r( ), or rewinddir( ),
the behavior is implementation-defined. Use of the file descriptor in a
directory-relative function shall be permitted, since it does not modify
the state of the associated description.  Upon calling closedir( ) the
file descriptor shall be closed.

Option 2 - include *at functions in the implementation-defined list:
Upon successful return from fdopendir( ), the file descriptor is under the
control of the system, and if any attempt is made to close the file
descriptor, to use the file descriptor in any directory-relative function,
or to modify the state of the associated description other than by means
of closedir( ), readdir( ), readdir_r( ), or rewinddir( ), the behavior is
implementation-defined. Upon calling closedir( ) the file descriptor shall
be closed.

Option 2 should also be expanded with an Application use note that
explains the usage pattern of dup().

Either way, it may also be helpful to provide a definition near line 1600
in XBD:
Directory-relative Function
Any one of the functions faccessat(), fchmodat(), fchownat(), fstatat(),
linkat(), mkdirat(), mkfifoat(), mknodat(), openat(), readlinkat(),
renameat(), symlinkat(), unlinkat(), or utimensat() which can perform file
access operations relative to an open directory file descriptor.

Note also that there are various incomplete lists of the *at functions
elsewhere in the standard that would benefit from this definition, such as
 XRAT D.2.3 line 123541 (also note that it has a typo, sylimkat).

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGeni284KuGfSFAYARApN7AJsETS7TCof1LZMbieag3v8htL9zZQCfeLoF
OHvyxjvNrbZl/XM7guAQ65A=
=4LR8
-----END PGP SIGNATURE-----