From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 27938 invoked from network); 4 May 2022 20:10:27 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 4 May 2022 20:10:27 -0000 Received: (qmail 21753 invoked by uid 550); 4 May 2022 20:10:24 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 21718 invoked from network); 4 May 2022 20:10:23 -0000 Date: Wed, 4 May 2022 16:10:10 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20220504201010.GZ7074@brightrain.aerifal.cx> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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).