Re: [PATCH v2 0/3] Support opening a symlink with O_PATH | O_NOFOLLOW

[Ken Brown <kbrown at cornell dot edu>]

On 12/30/2019 6:09 PM, Brian Inglis wrote:
> On 2019-12-30 14:47, Ken Brown wrote:
>> On 12/30/2019 3:55 PM, Brian Inglis wrote:
>>> On 2019-12-30 12:53, Ken Brown wrote:
>>>> On 12/30/2019 2:18 PM, Brian Inglis wrote:
>>>>> On 2019-12-29 10:56, Ken Brown wrote:
>>>>>> Currently, opening a symlink with O_NOFOLLOW fails with ELOOP.
>>>>>> Following Linux, the first patch in this series allows the call to
>>>>>> succeed if O_PATH is also specified.
>>>>>>
>>>>>> According to the Linux man page for 'open', the file descriptor
>>>>>> returned by the call should be usable as the dirfd argument in calls
>>>>>> to fstatat and readlinkat with an empty pathname, to have
>>>>>> the calls operate on the symbolic link.  The second and third patches
>>>>>> achieve this.  For fstatat, we do this by adding support
>>>>>> for the AT_EMPTY_PATH flag.
>>>>>>
>>>>>> Note: The man page mentions fchownat and linkat also.  linkat already
>>>>>> supports the AT_EMPTY_PATH flag, so nothing needs to be done.  But I
>>>>>> don't understand how this could work for fchownat, because fchown
>>>>>> fails with EBADF if its fd argument was opened with O_PATH.  So I
>>>>>> haven't touched fchownat.
>>>>>>
>>>>>> Am I missing something?
>>>>>
>>>>> WSL $ man 2 chown
>>>>> ...
>>>>> "AT_EMPTY_PATH (since Linux 2.6.39)
>>>>> If pathname is an empty string, operate on the file referred to
>>>>> by dirfd (which may have been obtained using the open(2) O_PATH
>>>>> flag). In  this case, dirfd can refer to any type of file, not
>>>>> just a directory. If dirfd is AT_FDCWD, the  call operates on
>>>>> the current working directory. This flag is Linux-specific; de‐
>>>>> fine _GNU_SOURCE to obtain its definition."
>>>>>
>>>>> says chown the dirfd, regardless of what it is,
>>>>> except if AT_FDCWD, chown the CWD.
>>>>>
>>>>> WSL $ man 2 open
>>>>> "O_PATH (since Linux 2.6.39)
>>>>> Obtain a file descriptor that can be used for two purposes: to
>>>>> indicate a location in the filesystem tree and to perform
>>>>> operations that act purely at the file descriptor level.  The
>>>>> file itself is not opened, and other file operations (e.g.,
>>>>> read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
>>>>> ioctl(2), mmap(2)) fail with the error EBADF."
>>>>>
>>>>> O_PATH does not open the file, so fchown returns EBADF,
>>>>> as it requires an fd of an open file.
>>>>
>>>> I think you've just confirmed what I already said: If fchownat is called with
>>>> AT_EMPTY_PATH, with an empty pathname, and with dirfd referring to a file that
>>>> was opened with O_PATH, then fchownat will fail with EBADF.
>>>>
>>>> So for the purposes of this patch series, I don't see the point of adding
>>>> support for AT_EMPTY_PATH in fchownat.
>>>>
>>>> Am I missing something?
>>>
>>> That is the user's problem: it is their responsibility to pass an fd open for
>>> reading or searching, not one opened with O_PATH (on Linux or Cygwin), or
>>> AT_FDCWD; it is Cygwin's responsibility to ensure that valid args succeed and
>>> invalid args return the expected errno.
>>
>> Yes, but Cygwin doesn't claim to support the AT_EMPTY_PATH flag except in
>> linkat.  So there is no expected errno.  The only way there would be an expected
>> errno is if we decide to add support for AT_EMPTY_PATH to fchownat.  I'm saying
>> that I don't see the point in doing that, and I'm asking whether I'm missing
>> something.  If you think I should add that support, please explain why.
> 
> To allow perms changed on the cwd, directories or files with an open fd, to
> avoid race conditions, like the other ...at functions.
> I don't get why you don't see those as useful cases.

I think we're mis-communicating.  This is a patch series whose purpose is to add 
support for opening a symlink with O_PATH | O_NOFOLLOW.  In that connection I 
modified readlinkat and fstatat to allow the resulting fd to be used as the 
dirfd argument in those calls, with an empty pathname.  I didn't do the same for 
fchownat because it seems to me that it would always fail with EBADF in that 
setting.

It's not relevant that AT_EMPTY_PATH might be useful for fchownat in a different 
setting.  That could be the subject of a different patch.  If you think it would 
be useful *in the context of this patch series*, please explain why.

Ken