mailing list of musl libc
 help / color / mirror / code / Atom feed
* [PATCH] save/restore errno around pthread_atfork handlers
@ 2017-11-10 20:58 Bobby Bingham
  2017-11-10 23:31 ` Rich Felker
  0 siblings, 1 reply; 5+ messages in thread
From: Bobby Bingham @ 2017-11-10 20:58 UTC (permalink / raw)
  To: musl

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] save/restore errno around pthread_atfork handlers
  2017-11-10 20:58 [PATCH] save/restore errno around pthread_atfork handlers Bobby Bingham
@ 2017-11-10 23:31 ` Rich Felker
  2017-11-11  0:03   ` Bobby Bingham
  0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2017-11-10 23:31 UTC (permalink / raw)
  To: musl

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.

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?

Rich


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] save/restore errno around pthread_atfork handlers
  2017-11-10 23:31 ` Rich Felker
@ 2017-11-11  0:03   ` Bobby Bingham
  2017-11-11  0:16     ` Rich Felker
  0 siblings, 1 reply; 5+ messages in thread
From: Bobby Bingham @ 2017-11-11  0:03 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] save/restore errno around pthread_atfork handlers
  2017-11-11  0:03   ` Bobby Bingham
@ 2017-11-11  0:16     ` Rich Felker
  2017-11-11  0:37       ` Bobby Bingham
  0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2017-11-11  0:16 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] save/restore errno around pthread_atfork handlers
  2017-11-11  0:16     ` Rich Felker
@ 2017-11-11  0:37       ` Bobby Bingham
  0 siblings, 0 replies; 5+ messages in thread
From: Bobby Bingham @ 2017-11-11  0:37 UTC (permalink / raw)
  To: musl

On Fri, Nov 10, 2017 at 07:16:27PM -0500, Rich Felker wrote:
> On Fri, Nov 10, 2017 at 06:03:40PM -0600, Bobby Bingham wrote:
> > > 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.

I understood the case you were talking about.  It just made me wonder
about this other case as well.

While callbacks are application code, they may not always be known to
the code calling the library function.  pthread_atfork is a perfect
example -- libraries like libressl may use it behind the back of the
main application.  If they set errno to zero in their atfork handlers,
it may _look_ to the main application like fork set errno to zero.

I'm also trying to understand the rationale for library functions being
barred from setting errno to zero.  Right where C11 section 7.5 says
errno is never set to zero by any library function, it has a footnote:

	Thus, a program that uses errno for error checking should set it to
	zero before a library function call, then inspect it before a
	subsequent library function call.

This seems to imply that the purpose of the prohibition is so that
application code can set errno to zero, call a function, and then look
at errno to determine if an error occurred.  Possibly call multiple
functions and do error checking all at the end, like the ferror status
in stdio.

But of course, the standard also says "the value of errno may be set to
nonzero by a library function call whether or not there is an error".
So this doesn't work, and I'm left not understanding why the prohibition
exists at all.

There are a few specific functions where it's important for application
code to set errno to zero first, like strtol and family, but otherwise
I'm not sure why this footnote encourages applications to set errno to
zero.

Bobby


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-11-11  0:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 20:58 [PATCH] save/restore errno around pthread_atfork handlers Bobby Bingham
2017-11-10 23:31 ` Rich Felker
2017-11-11  0:03   ` Bobby Bingham
2017-11-11  0:16     ` Rich Felker
2017-11-11  0:37       ` Bobby Bingham

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).