[PATCH] posix: Handle negative fds in spawn's addfchdir

[Adhemerval Zanella Netto]

On 25/03/26 16:15, Adhemerval Zanella Netto wrote:
> 
> 
> On 25/03/26 15:27, Collin Funk wrote:
>> Lucas Chollet <lucas.chollet@free.fr> writes:
>>
>>> Hello all,
>>> This is my first contribution here, so I hope I got everything right.
>>> I tested this patch on my Linux x86_64 machine with `make check` and
>>> got no regressions compared to master.
>>>
>>> 8<
>>>
>>> This patch makes `posix_spawn_file_actions_addfchdir` return EBADF on
>>> negative file descriptors. This is mandated by POSIX and similar to what
>>> is already done in addopen/addclose.
>>>
>>> Signed-off-by: Lucas Chollet <lucas.chollet@free.fr>
>>> ---
>>>  posix/spawn_faction_addfchdir.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/posix/spawn_faction_addfchdir.c b/posix/spawn_faction_addfchdir.c
>>> index a3df6df489..b51139a61f 100644
>>> --- a/posix/spawn_faction_addfchdir.c
>>> +++ b/posix/spawn_faction_addfchdir.c
>>> @@ -28,6 +28,9 @@ posix_spawn_file_actions_addfchdir_np (posix_spawn_file_actions_t *actions,
>>>  {
>>>    struct __spawn_action *rec;
>>>  
>>> +  if (!__spawn_valid_fd (fd))
>>> +    return EBADF;
>>> +
>>>    /* Allocate more memory if needed.  */
>>>    if (actions->__used == actions->__allocated
>>>        && __posix_spawn_file_actions_realloc (actions) != 0)
>>
>> Thank you for the patch!
>>
>> However, I want to note here that __spawn_valid_fd() checks if the file
>> descriptor is greater than getdtablesize(). Using it here, along with
>> some (all?) other places it is used, is incorrect. POSIX states [1]:
>>
>>     [EBADF]
>>         The value specified by fildes is negative.
>>
>>     It shall not be considered an error for the path or fildes argument
>>     passed to these functions to specify a pathname or file descriptor
>>     for which the specified operation could not be performed at the time
>>     of the call. Any such error shall be detected when the associated
>>     file actions object is later used during a posix_spawn() or
>>     posix_spawnp() operation.
>>
>> I noticed glibc's misbehavior when working on Gnulib last year, but
>> evidently forgot to fix it...
> 
> Indeed it seems that POSIX 2018 [1] changed the error handling for possible
> invalid file descriptors.   It seems that __spawn_valid_fd checks are not
> required anymore.
> 
> [1] https://austingroupbugs.net/view.php?id=418 

However, the POSIX requirements seems conflicting.  It states that

  [EBADF]
  The value specified by fildes is negative or greater than or equal to {OPEN_MAX}.

But at the same time it states that values potentially greater than
OPEN_MAX are not necessary an error:

  "It shall not be considered an error for the fildes argument passed to these
   functions to specify a file descriptor for which the specified operation could 
   not be performed"

So I think the 'EBADF' is a 'shall fail' only for negative values,
which seems the interpretation of musl and *BSD. I think it makes more sense.