Re: [musl] [PATCH] pack posix_spawn file actions into extraneous padding

[Rich Felker <dalias@libc.org>]

On Fri, Feb 04, 2022 at 08:09:13PM +0100, Joakim Sindholt wrote:
> 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.

Since we can already have "spurious" failures anyway from insufficient
free fd space to shuffle the pipe fd, I don't think the thing with
FDOP_CHDIR matters. If we do ever decide we care, you've already
presented a way we could solve the problem.

(I noted on IRC that we could also solve the problem of the "density"
of the coding space by using the 32 bits of __pad0[1] to store type
information for the 16 slots, but this does not seem necessary.)

One change I think I would like to make is not using the names __pad0
and __pad directly in code. It's ugly. Instead, fdop.h could do:

#define fa_cnt __pad0[0]
#define fa_ops __pad

It might also make sense to define a macro for the fa_ops array limit

#define FA_OPS_MAX_CNT ...

using sizeof on a compound literal as appropriate.

We could also for consistency (not using __ junk names from the public
header directly) do

#define fa_chain __actions

Not sure if that's the best name for it but it seems to make things
make more sense than writing __actions in the code that's no longer
using the pointed-to data as the actions list.

I think I'm happy at this point with just doing "strace tests", but
sometime between now and next release I would like to get a better set
of tests into libc-test that cover usage of sufficiently many ops to
require allocations. Something like a test function that takes a list
of operations and builds both the file actions object and a shell
command to evaluate that the file actions were executed correctly,
using:

	read x <&%d && test "$x" = %s

and:

	! 2>/dev/null <&%d

to assert that each fd is either closed or reads the expected content
(expected content could be temp files with different content in each).