bug#61095: possible misuse of posix_spawn API on non-linux OSes

["Ludovic Courtès" <ludo@gnu.org>]

Hi!

Josselin Poiret <dev@jpoiret.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> -      posix_spawn_file_actions_addclose (actions, fd);
>> +      /* Adding invalid file descriptors to an 'addclose' action leads
>> +         to 'posix_spawn' failures on some operating systems:
>> +         <https://bugs.gnu.org/61095>.  Hence the extra check.  */
>> +      int flags = fcntl (max_fd, F_GETFD, NULL);
>> +      if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
>> +        posix_spawn_file_actions_addclose (actions, max_fd);
>>      }
>
> I'm worried about TOCTOU in multi-threaded contexts here,

Yes, that’s a problem.  The current /proc/self/fd optimization has that
problem too.  :-/

> which is why I opted for the heavy handed-approach here.  In general I
> don't think we can know in advance which fdes to close :/ It's a shame
> that the posix_spawn actions fails on other kernels, since I don't
> really see a way to mitigate this problem (apart from the new
> posix_spawn_file_actions_addclosefrom_np as you mentioned).  I don't
> know what we could do here.  Maybe not provide spawn?  Or provide it
> in spite of the broken fd closing?

Not providing ‘spawn’ is no longer an option.

We can expect the problem to practically vanish “soon” on GNU variants
with ‘closefrom’ (glibc 2.34 was released in Aug. 2021).

On Linux with glibc < 2.34, we’d keep the current code (maybe without
the /proc/self/fd optimization?).

On other systems, we can have the racy code above as a last resort or
OS-specific tricks, like Omar was suggesting for OpenBSD.  It sucks but
what else can we do?

(BTW,
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_addclose.html>
reads:

  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 at the time of the call. Any such
  error will be detected when the associated file actions object is
  later used during a posix_spawn() or posix_spawnp() operation.

OpenBSD and GNU/Hurd follow this to the letter.

OTOH ‘linux/spawni.c’ in glibc is purposefully more liberal:

  /* Signal errors only for file descriptors out of range.  */
)

>> +  /* Duplicate IN, OUT, and ERR unconditionally to clear their
>> +     FD_CLOEXEC flag, if any.  */
>> +  posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO);
>> +  posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO);
>> +  posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO);
>
> This won't work, and actually this was one of the original logic bugs I
> was trying to fix.  If err is equal to, let's say, STDIN_FILENO, then
> the first call will overwrite the initial file descriptor at
> STDIN_FILENO, and the second call won't do what the caller intended.
> This is why I was moving them out of the way first, so that they would
> not overwrite each other.

Oh, my bad.

>> +  /* TODO: Use 'closefrom' where available.  */
>> +#if 0
>> +  /* Version 2.34 of the GNU libc provides this function.  */
>> +  posix_spawn_file_actions_addclosefrom_np (&actions, 3);
>> +#else
>> +  if (in > 2)
>> +    posix_spawn_file_actions_addclose (&actions, in);
>> +  if (out > 2 && out != in)
>> +    posix_spawn_file_actions_addclose (&actions, out);
>> +  if (err > 2 && err != out && err != in)
>> +    posix_spawn_file_actions_addclose (&actions, err);
>
> Isn't this unneeded given we call close_inherited_fds below?

No, because of the FD_CLOEXEC selection.

Coming next is an updated patch series addressing this as proposed
above.  Let me know what y’all think!

I tested the ‘posix_spawn_file_actions_addclosefrom_np’ path by building in:

  guix time-machine --branch=core-updates -- shell -CP -D -f guix.scm

… which gives us glibc 2.35.

Ludo’.