From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12081 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] save/restore errno around pthread_atfork handlers Date: Fri, 10 Nov 2017 19:16:27 -0500 Message-ID: <20171111001627.GS1627@brightrain.aerifal.cx> References: <20171110205829.18319-1-koorogi@koorogi.info> <20171110233134.GR1627@brightrain.aerifal.cx> <20171111000340.GA22903@dora.lan> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1510359401 23755 195.159.176.226 (11 Nov 2017 00:16:41 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 11 Nov 2017 00:16:41 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12097-gllmg-musl=m.gmane.org@lists.openwall.com Sat Nov 11 01:16:38 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1eDJTO-0005qt-Dq for gllmg-musl@m.gmane.org; Sat, 11 Nov 2017 01:16:34 +0100 Original-Received: (qmail 21567 invoked by uid 550); 11 Nov 2017 00:16:40 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 21546 invoked from network); 11 Nov 2017 00:16:39 -0000 Content-Disposition: inline In-Reply-To: <20171111000340.GA22903@dora.lan> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12081 Archived-At: On Fri, Nov 10, 2017 at 06:03:40PM -0600, Bobby Bingham wrote: > On Fri, Nov 10, 2017 at 06:31:34PM -0500, Rich Felker wrote: > > On Fri, Nov 10, 2017 at 02:58:29PM -0600, Bobby Bingham wrote: > > > If the syscall fails, errno must be preserved for the caller. There's no > > > guarantee that the handlers registered with pthread_atfork won't clobber > > > errno. > > > --- > > > src/process/fork.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/src/process/fork.c b/src/process/fork.c > > > index b96f0024..6602eafc 100644 > > > --- a/src/process/fork.c > > > +++ b/src/process/fork.c > > > @@ -15,6 +15,7 @@ pid_t fork(void) > > > { > > > pid_t ret; > > > sigset_t set; > > > + int olderr; > > > __fork_handler(-1); > > > __block_all_sigs(&set); > > > #ifdef SYS_fork > > > @@ -30,6 +31,10 @@ pid_t fork(void) > > > libc.threads_minus_1 = 0; > > > } > > > __restore_sigs(&set); > > > + > > > + olderr = errno; > > > __fork_handler(!ret); > > > + errno = olderr; > > > + > > > return ret; > > > } > > > -- > > > 2.15.0 > > > > I think the patch as written is incorrect, because it can set errno to > > 0 after application code in the atfork handler set it to something > > nonzero; doing so is non-conforming. > > Good point. It does make me wonder though: when libc invokes a callback > and that callback sets errno to zero, is that a violation of the > prohibition on library functions setting errno to zero? No, that's application code setting it to 0. The case I'm talking about is when errno is 0 before fork is called, 0 gets stored in olderr, the atfork handler sets errno to some nonzero value, and then the implementation wrongly sets it back to 0. That's observable by the application. > > It would be possible to special-case to avoid this, but it probably > > makes more sense to just call SYS_fork/SYS_clone with __syscall rather > > than syscall, then return __syscall_ret(ret) instead of return ret. > > Does that sound correct? > > Yes, and it's also probably simpler. I'll send a new patch. OK. Rich