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=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 18222 invoked from network); 3 Sep 2020 16:38:22 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 3 Sep 2020 16:38:22 -0000 Received: (qmail 9850 invoked by uid 550); 3 Sep 2020 16:38:20 -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 9832 invoked from network); 3 Sep 2020 16:38:19 -0000 Date: Thu, 3 Sep 2020 12:38:07 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200903163807.GF3265@brightrain.aerifal.cx> References: <20200903112309.102601-1-sorear@fastmail.com> <20200903112309.102601-8-sorear@fastmail.com> <20200903154935.GA3265@brightrain.aerifal.cx> <4c14e84f-d139-42d1-a23e-74be05de26ec@www.fastmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4c14e84f-d139-42d1-a23e-74be05de26ec@www.fastmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 07/14] Emulate wait4 using waitid On Thu, Sep 03, 2020 at 12:25:19PM -0400, Stefan O'Rear wrote: > > > On Thu, Sep 3, 2020, at 11:49 AM, Rich Felker wrote: > > On Thu, Sep 03, 2020 at 07:23:02AM -0400, Stefan O'Rear wrote: > > > riscv32 and future architectures lack wait4. > > > --- > > > src/internal/wait4_waitid.h | 1 + > > > src/linux/__wait4_waitid.c | 52 +++++++++++++++++++++++++++++++++++++ > > > src/linux/wait4.c | 5 ++++ > > > src/process/waitpid.c | 6 +++++ > > > src/stdio/pclose.c | 6 +++++ > > > src/unistd/faccessat.c | 6 ++++- > > > 6 files changed, 75 insertions(+), 1 deletion(-) > > > create mode 100644 src/internal/wait4_waitid.h > > > create mode 100644 src/linux/__wait4_waitid.c > > > > I really don't like introducing a new src/internal file for this. If > > I agree. > > > the code needs to be shared it should probably just be in wait4.c, > > renamed to __wait4, declared in src/include/sys/wait.h according to > > the usual pattern for exposing namespace-safe versions of > > namespace-violating functions. > > Currently waitpid and waitid are cancellation points (required by POSIX) > but wait4 is not, which is why I did not have anything else call a function > with exactly wait4 semantics. > > > Ideally I'd like to just not need it, but I'm not sure that's > > possible: > > > > - faccessat throws away the status and doesn't have any need for the > > status or idtype emulation code. It could easily be just different > > syscalls in #ifdef/#else paths. > > I think that is exactly what my patch does? Yes, sorry I missed that. I think I saw the other three and just assumed this one was the same since it was part of the same patch. > > - pclose does need status but not the idtype part. but it's only > > listening for process termination not all the other weird stuff. I'm > > not sure if it would make sense to construct status inline here. It > > might if we had a macro like the glibc "inverse-W" macro (I forget > > its name) to construct a wait status from parts; if so this could > > later be considered for a public API (previously requested). > > Earlier I had the wait status conversion code broken out separately > but it seemed not worth it for just pclose. > > > - waitpid and wait4 at least need the idtype and status construction. > > > > Unfortunately, wait4 also needs the rusage conversion, which waitpid > > doesn't want. This kinda defeats my idea of just exposing __wait4 from > > wait4.c. > > > > A side observation to keep in mind is that passing the argument cp > > doesn't really help optimize anything; it only worked well in > > c2feda4e2e because the function there is inline. Of the 4 callers you > > have, faccessat and pclose have hard requirements not to be a > > cancellation point and waitpid has a hard requirement to be one. But > > pclose has a hard requirement to not be a cancellation point but it also > has "If a thread is canceled during execution of pclose(), the behavior > is undefined." The implications of the combination are confusing. It's an optional cancellation point, but if cancelled the side effects must be correct. Since the FILE and underlying fd have already been closed at this point, it's too late for cancellation to be acted upon; any thing you do would leave the process in an inconsistent state, where the caller has to assume the FILE is still valid and thus performs UAF/DF if it does anything with it. Note that you can't swap the order of the fd close and wait because the child very well may not exit until the pipe is closed. So, the only valid way pclose could act on cancellation is before taking any action. This is almost surely not what was intended in the standard, since it's useles, but the conclusion here is just that nobody thought about this stuff enough to realize that allowing pclose to be a cancellation point was fundamentally wrong. > > if the function weren't used for the former, it could just always be > > cancellable -- wait4 probably should have been cancellable all along, > > and almost surely is on other implementations, so to use it safely > > you'd have to not use, block, or handle cancellation. > > I have no immediate objection to making wait4 a cancellation point but > I would prefer to make that change consistently on all architectures. > I don't have a good sense of who uses cancellation and wait4 and what > compatibility constraints are imposed by code (as opposed to standards). Yes, it should be done consistently and as a separate patch if we go this way. > > So I'm leaning towards sticking with something like what you've done, > > but with declaration moved to src/include/sys/wait.h, or possibly > > src/internal/syscall.h (since it's essentially emulation of a > > I'd be very happy to not have a one-line header file. I looked at syscall.h > earlier but wasn't sure if it fit; I did not consider reserved namespace in > a public header (would this preclude use of hidden?). src/include/* are not public headers, but wrappers around the public headers to add namespace-protected versions of functions etc. However since this is really not a "version of wait4" but an "emulation of SYS_wait4", it probably makes more sense to put it in internal/syscall.h, just like __sys_open[23]. That doesn't mean it has to be inline, just that the declaration could go there, and a macro to just use __syscall(SYS_wait4...) directly if it exists which would keep some small amount of #ifdef out of source files. > Is src/linux/__exact_name_of_function.c the appropriate name for the file? If there's an external source file still, I'd put it in src/process/__function_name.c or src/internal/... like you did. The existing naming here is not entirely consistent, but generally, src/subsystem/__internal_function.c is an internal function for implementing that subsystem/family, not an internal functon used by other subsystems that just happens to mostly-match a public function in that subsystem. src/internal is a kinda sloppy mix of things, but is mostly internal functions that are shared between two or more subsystems. Rich