Patchwork [roland/getpid] Simplify getpid handling of the race case.

login
register
mail settings
Submitter Roland McGrath
Date May 9, 2014, 10:23 p.m.
Message ID <20140509222324.795042C3A01@topped-with-meat.com>
Download mbox | patch
Permalink /patch/867/
State New
Headers show

Comments

Roland McGrath - May 9, 2014, 10:23 p.m.
In looking at consolidating the NPTL vs not versions of vfork, I came
across the single, bizarre difference between the libpthread and libc
versions of vfork.  This is implemented by a few lines of assembly for each
machine, and nowhere is there a comment that explains the rationale.  

The code in question is fiddling with the pid field of 'struct pthread'.
It negates the value before doing vfork and then restores it afterwards in
the parent.  The rationale for this is that there could be a race where a
signal handler runs in the child immediately after the vfork call, and in
that case the getpid optimization of just returning THREAD_SELF->pid would
return the parent's PID rather than the child's.  That's all fair enough.
Nothing ever sets ->pid in the child; it wouldn't be safe to do so, since
the child and parent share memory, so if ->pid were ever >0 in the child,
there would be a race after the child exits/execs and the parent resumes
where a signal handler could run and see the child-set value of ->pid.
(This just means that getpid's short-circuit logic will never win in a
vfork child, so it makes actual getpid system calls.)

The difference is that in the libc version, it checks for ->pid being zero
and if it's zero instead sets it to 0x80000000 (i.e. an arbitrary negative
number).  The libpthread version doesn't do that, so if it's set to zero
then it stays that way.  AFAICT the reason for this is so that getpid, in
its handling of the race case (i.e. THREAD_SELF->pid <= 0) can have another
special case to avoid making the getpid system call: If ->pid was exactly
zero but ->tid is nonzero, it just returns ->tid as the PID.  But I don't
actually see how ->pid could ever be set to zero so as to trigger this
case.  Then there is further logic whereby if ->pid and ->tid were both set
to zero, then getpid will store the PID retrieved by the system call in
->tid (but not in ->pid).  Again I don't see how that case can arise: ->tid
can never be zero in a live thread after __pthread_initialize_minimal runs.
Perhaps there can be a window where user constructors run before that?

It all seems rather muddled, and I feel like I must be missing some of
what's going on.  If anything, the libc/libpthread difference seems
backwards: in libc without libpthread, using ->tid should be OK because
there's only one thread and its TID==PID; in libpthread, using ->tid can
only be right if you're sure you're in the child and ->tid has been set to
the child's new TID (which matches its PID).  Maybe someone else can help
explain it all.

But all this complexity seems like a lot of hair to micro-optimize a case
that can only arise during a race condition.  If we simplify getpid so that
it always just falls back to the system call when any fork/vfork is in
progress, then we don't need the complexity and we don't need the libc and
libpthread versions of vfork to differ.  This change does that, leaving the
way open to consolidate the vfork code during my NPTL consolidation.

I'm not very clear on the utility of ever setting ->pid to a negative
value.  The fork and vfork code never look at the negated value again, they
just save the original value locally and restore it later.  So why not just
zero ->pid at the beginning of fork and vfork instead?

Does this seem sensible?  Can anyone give a full explanation of the picture
here that gives the rationale for the confusing things in the status quo?


Thanks,
Roland


	* nptl/sysdeps/unix/sysv/linux/getpid.c
	(really_getpid): Function removed.
	(__getpid): Rewritten.  Under [!NOT_IN_libc], use THREAD_SELF->pid if
	it's > 0.  Otherwise always just make the system call.
Rich Felker - May 10, 2014, 12:12 a.m.
On Fri, May 09, 2014 at 03:23:24PM -0700, Roland McGrath wrote:
> In looking at consolidating the NPTL vs not versions of vfork, I came
> across the single, bizarre difference between the libpthread and libc
> versions of vfork.  This is implemented by a few lines of assembly for each
> machine, and nowhere is there a comment that explains the rationale.  
> 
> The code in question is fiddling with the pid field of 'struct pthread'.

I think there are several open race-condition bugs that amount to the
pid caching glibc does being invalid in certain corner cases. Would
you review them as part of looking at this issue?

Rich
Roland McGrath - May 13, 2014, 5:39 p.m.
> I think there are several open race-condition bugs that amount to the
> pid caching glibc does being invalid in certain corner cases. Would
> you review them as part of looking at this issue?

I'm only talking about what I'm talking about, which does not include any
known bugs.  If you know of any bugs, then file bugzilla reports for them
and elaborate on the precise details.


Thanks,
Roland
Rich Felker - May 14, 2014, 1:52 p.m.
On Tue, May 13, 2014 at 10:39:14AM -0700, Roland McGrath wrote:
> > I think there are several open race-condition bugs that amount to the
> > pid caching glibc does being invalid in certain corner cases. Would
> > you review them as part of looking at this issue?
> 
> I'm only talking about what I'm talking about, which does not include any
> known bugs.  If you know of any bugs, then file bugzilla reports for them
> and elaborate on the precise details.

There are at least a couple with open bug reports that seem to be
related to this issue: 12889 and 15368.

Rich

Patch

--- a/nptl/sysdeps/unix/sysv/linux/getpid.c
+++ b/nptl/sysdeps/unix/sysv/linux/getpid.c
@@ -21,42 +21,17 @@ 
 #include <sysdep.h>
 
 
-#ifndef NOT_IN_libc
-static inline __attribute__((always_inline)) pid_t really_getpid (pid_t oldval);
-
-static inline __attribute__((always_inline)) pid_t
-really_getpid (pid_t oldval)
-{
-  if (__glibc_likely (oldval == 0))
-    {
-      pid_t selftid = THREAD_GETMEM (THREAD_SELF, tid);
-      if (__glibc_likely (selftid != 0))
-	return selftid;
-    }
-
-  INTERNAL_SYSCALL_DECL (err);
-  pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
-
-  /* We do not set the PID field in the TID here since we might be
-     called from a signal handler while the thread executes fork.  */
-  if (oldval == 0)
-    THREAD_SETMEM (THREAD_SELF, tid, result);
-  return result;
-}
-#endif
-
 pid_t
 __getpid (void)
 {
-#ifdef NOT_IN_libc
-  INTERNAL_SYSCALL_DECL (err);
-  pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
-#else
+#ifndef NOT_IN_libc
   pid_t result = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__glibc_unlikely (result <= 0))
-    result = really_getpid (result);
+  if (__glibc_likely (result > 0))
+    return result;
 #endif
-  return result;
+
+  INTERNAL_SYSCALL_DECL (err);
+  return INTERNAL_SYSCALL (getpid, err, 0);
 }
 
 libc_hidden_def (__getpid)