[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220504201010.GZ7074@brightrain.aerifal.cx>
Date: Wed, 4 May 2022 16:10:10 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] pack posix_spawn file actions into extraneous
padding
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).
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
