rickyz for historical purposes, though he's no longer on the team.

Also OS-Chrome since this will eventually impact Chrome.

Looks like glibc stopped populating the PID/TID cache in clone, which we rely on in our ForkWithFlags function: https://cs.chromium.org/chromium/src/base/process/launch.h?type=cs&q=ForkWithFlags&sq=package:chromium&l=313

Annoyingly, they stopped populating the TID cache in clone, but they didn't actually remove all uses of the cache:

https://sourceware.org/git/?p=glibc.git;a=blob;f=nptl/pthread_mutex_lock.c;h=dc9ca4c4764be2654141493330bd8a91a989f601;hb=HEAD#l148

At a glance, I unfortunately didn't see a nice way to trigger population of the tid cache short of doing a pointless additional fork or I guess doing horrible, potentially libc version dependent guessing to figure out where the tid cache lives in TLS and fix it up.

glibc has unfortunately done a very thorough job here of making it an utter pain to call clone() with the various namespacing flags.

I haven't looked at this code in a while, but as ugly as it is, I wonder if we could get away with using:

unshare(CLONE_NEWPID);
fork();

in NamespaceSandbox::ForkInNewPidNamespace.

I believe our other main use of clone, starting up the zygote, is fine because the cloned child with the invalid cached tid forks off the real zygote process (fork populates the tid cache), and the original process becomes a reaper process (CreateInitProcessReaper) which  should not do any pthread stuff that depends on a cached PID/TID.

That's a very interesting explanation, thanks for the write-up.

Is there anyone who I could assign this bug to, or is there anything we need to do in glibc?

Just to add another data point, I recently started running into this (trying to run a component debug chromium build) on Debian Testing (now buster) with user namespaces enabled (Debian defaults to disabled). It appears that the recent Debian Stable (stretch) also has this glibc change.

Note that (on Debian) if I disable user namespaces

sh -c 'echo 0 > /proc/sys/kernel/unprivileged_userns_clone'

and then build the 'old' chrome_sandbox and set it up then that works fine (as expected). Since Debian ships with user namespaces off by default, this means most Debian users probably aren't seeing this issue (since user namespaces are disabled). Ubuntu, however, turns user namespaces on by default, so the next Ubuntu release will surface this sort of issue far more often.

I'll take it for now to see if I can hand it off to someone else (maybe Robert?).

We should at least figure out an owner before M-61 branches.

Also, for those following at home, note that Ricky's suggestion works by putting the child created by fork() in a new namespace. unshare(CLONE_NEWPID) never changes the pid namespace of the current process, because a process should never see its own pid change during its lifetime.

Friendly ping here.

/proc/sys/kernel/unprivileged_userns_clone is specific to Debian/Ubuntu AFAIK; with Fedora 26 being based on glibc 2.25 I think it'll be harder to keep a local checkout with the pid caching change reverted, and like bungeman@ said this will probably cause a lot of pain for Ubuntu users in the future.

I'll take another look next week. We definitely need to fix this somehow.

Upstream glibc bug created: https://sourceware.org/bugzilla/show_bug.cgi?id=21793
Also, we have a workaround CL: https://chromium-review.googlesource.com/c/575769/

Maybe it would also be possible to statically link libc until the upstream fix lands and is backported?

Thanks for creating the upstream glibc, bug.

I want to respond directly here to reiterate a point I made there too.

Once you call clone() you own the created thread, and you cannot use any POSIX Thread functions provided by the libc you left behind when you cloned a new thread manually. If you are using clone() as a convenience function for unshare() then you should use unshare().

Take care that calling clone() yourself has implications on all the thread APIs, from thread local storage, to cancellation, to joining, to exiting threads, etc. You really really have to be ready to take over all aspects of the new thread using your own threading runtime. The point of clone() is to implement threading libraries, like those provided by glibc.

I'm all ears when it comes to hearing about what problems you use clone() to solve and finding a better solution for you.

Lastly, regarding comment 10, I strongly suggest against bundling/statically linking to glibc, such a solution will cause you all sorts of of potential pain, from security (need to recompile for latest CVE fixes), to deployment (all IdM [read NSS] functions go through the normal plugin infrastructure [read dlopen()] and require matching build/host runtimes).

thomasanderson@: Thanks for making the workaround - did you by any chance try the unshare(CLONE_NEWPID); fork() idea mentioned in comment 2 and run into issues? It would be nice not to pay the extra fork on Linux, and while it's not very likely to result in harmful side effects, hardcoding offsets into internal glibc structures is... unfortunate.

codonell@: Thanks for posting here. It's been a while since I've looked at Chrome's Linux sandbox, but here's what I recall about the functionality we wanted clone for:

Say you process that wants to repeatedly fork children, some which are placed in a separate user and PID namespace and some which are not. We cannot repeatedly call unshare(CLONE_NEWUSER | CLONE_NEWPID); fork(); because this places the parent process into the user namespace (and the parent process cannot rejoin its original user namespace/reset the PID namespace for future children without having CAP_SYS_ADMIN in the appropriate original namespaces). What we really want is a version of fork which supports these clone flags, but also updates THREAD_SELF->tid. Right now, if someone wants to do this, the best way I can think of is to pay an extra fork: fork(); unshare(...); fork().

All that said, while I think the above is a situation that I think is difficult to do with what glibc provides today, and I'd love for it to be conveniently doable, it does not perfectly match the situation that we're running into in this bug. Like I mentioned in comment #2, I think we might be able to get away with unshare(CLONE_NEWPID); fork() each time the zygote forks, since in our situation:

 - The the main zygote process should have a correct cached TID. The zygote is started using our problematic ForkWithFlags API, so immediately after calling ForkWithFlags, the resulting child process does have an incorrect cached TID. Luckily for us, we use this process as init for the zygote's PID namespace, so we never call any thread APIs in this thread. Instead, we fork off a process for the real zygote process, and doing so writes a correct cached TID.
 - The zygote never needs to fork a process that isn't in a new PID namespace.
 - The children that the zygote spawns remain in the same user namespace as the zygote (we just drop all capabilities in the child). The zygote process already has CAP_SYS_ADMIN in its user namespace, so unshare(CLONE_NEWPID) should be allowed.

I haven't tested this approach at all though, so folks can correct me if I'm mistaken.

Er correction: Everything I said about the zygote process becoming the init process is irrelevant because we start the zygote via LaunchProcess, which is a ForkWithFlags followed by an execve. After execve, the processes TID cache is setup properly, obviously :-) So whether we go with the double-forking approach or the unshare approach (assuming it works), we only need to worry about fixing NamespaceSandbox::ForkInNewPidNamespace.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24df419517bb8ca8ac2b75eb752bd9a133267108

commit 24df419517bb8ca8ac2b75eb752bd9a133267108
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Tue Jul 25 02:41:01 2017

Update glibc's thread descriptor after clone()'ing

Bug:  735048 
Change-Id: Ic60bc676c4d743e24168cf9f7cf3e4b6414ba3b4
R: jorgelo@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/575769
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Ricky Zhou <rickyz@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489195}
[modify] https://crrev.com/24df419517bb8ca8ac2b75eb752bd9a133267108/base/process/launch.h
[modify] https://crrev.com/24df419517bb8ca8ac2b75eb752bd9a133267108/sandbox/linux/services/namespace_sandbox.cc

Should be fixed now.  Thanks Ricky for all the help!

This is a warning that the fix you have committed in 24df419517bb8ca8ac2b75eb752bd9a133267108 is completely unsupported and will break when we change the TCB layout. You have no guarantees given by writing into implementation dependent structures. Your only safe implementation is to avoid using clone(), and use the double-fork() which was suggested in comment 13.

Thanks, we're aware that we're modifying internal glibc data structures.  However, the layout doesn't appear to have changed in almost a decade.  And if it does, uses will get a loud CHECK() failure and we'll see it in our crash reports and be able to respond quickly