From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12079 Path: news.gmane.org!.POSTED!not-for-mail From: Bobby Bingham Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] save/restore errno around pthread_atfork handlers Date: Fri, 10 Nov 2017 18:03:40 -0600 Message-ID: <20171111000340.GA22903@dora.lan> References: <20171110205829.18319-1-koorogi@koorogi.info> <20171110233134.GR1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 X-Trace: blaine.gmane.org 1510358678 944 195.159.176.226 (11 Nov 2017 00:04:38 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 11 Nov 2017 00:04:38 +0000 (UTC) User-Agent: Mutt/1.9.1 (2017-09-22) To: musl@lists.openwall.com Original-X-From: musl-return-12095-gllmg-musl=m.gmane.org@lists.openwall.com Sat Nov 11 01:04:27 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 1eDJHe-00088f-B8 for gllmg-musl@m.gmane.org; Sat, 11 Nov 2017 01:04:26 +0100 Original-Received: (qmail 9267 invoked by uid 550); 11 Nov 2017 00:04:31 -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 9244 invoked from network); 11 Nov 2017 00:04:30 -0000 Content-Disposition: inline In-Reply-To: <20171110233134.GR1627@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:12079 Archived-At: 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? > > 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. > > Rich