On Sat, Jan 29, 2022 at 02:43:28PM +0100, Joakim Sindholt wrote:
> I've written this patch that packs posix_spawn file actions into the
> posix_spawn_file_actions struct padding. I'm not sure if the way I've
> split the code up is desirable. Maybe the faexpand() function needs a
> different name and to be in its own .c file; or maybe a different
> approach is preferable.

Here's a candidate v2 patch to simplify some things. Changes since the
first one:
* fa->__actions is initialized to point to itself. This makes faexpand a
  little simpler and means that ->__actions->__actions always points to
  the last file_actions struct.
* The previous version caused an increase of 32 bytes in used stack
  space in child() on amd64. This one only causes a 16 byte increase.
  I'm not sure how big of a deal-breaker that is.
* The two switches have been merged into one. This makes it simpler IMO
  but also has some consequences:

The current implementation in musl uses a pipe to communicate
success/failure back to the parent process. This means that the
file_actions struct can accidentally succeed if it tries to use the pipe
fd as a source fd or worse, overwrite the pipe fd when targeting it in
either the dup2 or open actions.
Musl solves this by moving the pipe fd if it's the target of an
operation. Currently musl will move the pipe if it's the source fd of
close or fchdir, or if it's the target fd of dup2 or open. You'll note
that the close and fchdir cases mean that instead of failing
immediately, it will dup the pipe fd, close the old pipe fd, and then do
the actual close/fchdir syscall which then fails. It's presumably
implemented this way because it's simple and doesn't really cause a
problem. The operation will result in failure regardless.

This patch, in an attempt to be very simple, introduces a similar but
perhaps slightly worse bug. The encoding of every operation is
essentially [fd, other stuff], meaning it can do the pipe move check on
the fd in the first slot. It won't work for close since the fd is
actually -fd-1, so close gets its own check. If the check is
unconditional then we might get an unnecessary move with chdir, which is
encoded as [dummy fd, -FDOP_CHDIR]. I've chosen an fd that's unlikely to
collide (INT_MAX) however if you have every fd from 0 to INT_MAX-1 open
then the pipe fd will be on fd INT_MAX and it will not be movable. The
result is that the unnecessary pipe move fails and an otherwise
perfectly legal and possible chdir action will fail.

The fix is very simple but does end up once again causing some
duplication in the op logic:
-if (fd == p) {
+if (fd == p && fa->__pad[i] != -FDOP_CHDIR) {

Alternatively you could do the whole command check twice, only move the
fd if it's the target of dup2 or open, and then have a check in fchdir
for whether it's trying to use the pipe fd as a source, like the ones
already in close and dup2.

Or maybe this bug is acceptable.

Joakim