Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Florian Weimer]

The IFUNC use is incorrect because you cannot assume that some other
symbol has been relocated, and the current implementation sometimes
returns an unrelocated address.  (The bug is about vfork, but I'm sure
fork has the same issue.)

Is there a reliable test case which exposes this problem?  I made the
vfork wrapper in libpthread a non-tail-call, fixed up the nptl vfork
tests to actually call the wrapper (which I assume they currently don't
due to the compat symbol), and still didn't get any crash.  I even tried
to add a bit of extra work in the child, to make sure that the stack is
overwritten (but downwards only, obviously).

Any ideas?

Florian

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Andreas Schwab]

The usual problem with a non-tail-call vfork is that subsequent calls in
the child overwrite the return address on the stack so that when vfork
returns the second time it doesn't return to the original caller in the
parent.  That doesn't necessarily lead to a crash, but you get a wrong
control flow.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Richard Henderson]

On 03/30/2016 06:18 AM, Florian Weimer wrote:
> The IFUNC use is incorrect because you cannot assume that some other
> symbol has been relocated, and the current implementation sometimes
> returns an unrelocated address.  (The bug is about vfork, but I'm sure
> fork has the same issue.)

You should be able to rely on relocations within library A having been run
before ifunc use within library A.  Anything else is probably a linker/loader bug.

There are complex situations in which an IFUNC in library A references a symbol
X within library B, and the data structures behind X have not been relocated.

But that is not the case here: The reference to __libc_fork within libpthread
should be resolved by ld.so to the location within libc during primary
(non-lazy) relocation processing, before entry to main.

> Is there a reliable test case which exposes this problem?  I made the
> vfork wrapper in libpthread a non-tail-call, fixed up the nptl vfork
> tests to actually call the wrapper (which I assume they currently don't
> due to the compat symbol), and still didn't get any crash.

If you're talking vfork, then you *must* use a tail-call, or other means (like
ifunc) to arrive at the syscall with no stack used.  Otherwise you will have
situations where the child corrupts the saved return address of the parent.

The inability to reliably generate a tail-call between libraries for all ports
is exactly why we're defaulting to use an IFUNC in the first place.

But certainly individual ports are free to override pt-vfork.c.  The simplest
way is simply not to bounce to libc at all, but continue to provide a copy of
vfork within libpthread.  E.g. sysdeps/unix/sysv/linux/alpha/pt-vfork.S.


r~

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Szabolcs Nagy]

* Richard Henderson <rth@redhat.com> [2016-03-30 09:47:39 -0700]:
> On 03/30/2016 06:18 AM, Florian Weimer wrote:
> > The IFUNC use is incorrect because you cannot assume that some other
> > symbol has been relocated, and the current implementation sometimes
> > returns an unrelocated address.  (The bug is about vfork, but I'm sure
> > fork has the same issue.)
> 
> You should be able to rely on relocations within library A having been run
> before ifunc use within library A.  Anything else is probably a linker/loader bug.
> 
> There are complex situations in which an IFUNC in library A references a symbol
> X within library B, and the data structures behind X have not been relocated.
> 
> But that is not the case here: The reference to __libc_fork within libpthread
> should be resolved by ld.so to the location within libc during primary
> (non-lazy) relocation processing, before entry to main.
> 

the crash happens before entry to main.

libdofork.so has a GLOB_DAT relocation against fork,
so when it is loaded it runs the ifunc resolver
defined in libpthread.so

(the easy way to get that is to use
void *volatile p = (void*)fork;
but -Wl,-z,now works too.)

if the relocations of libpthread.so are processed later
than the relocations of libdofork.so then this can crash.

i'm not sure about the ordering requirements of
relocations of modules.. i think this is only observable
through ifunc resolution.
(e.g. elf defines symbol lookup and ctor ordering but
not relocation ordering across modules.)

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Richard Henderson]

On 03/30/2016 12:10 PM, Szabolcs Nagy wrote:
> if the relocations of libpthread.so are processed later
> than the relocations of libdofork.so then this can crash.
> 
> i'm not sure about the ordering requirements of
> relocations of modules..

Ah, this is a very interesting case.

I went back and looked at the PR more closely and my first reaction was that
libdofork.so was being built incorrectly:

  gcc -shared -Wl,-z,now -o libdofork.so dofork.o

Note that there's no dependency of libdofork.so on libpthread.so.  The proper
existence of such dependencies is the only way that ld.so has in order to
process the libraries in the proper order.  You'll find that the crash goes
away if we do provide this dependency:

  gcc -shared -Wl,-z,now -o libdofork.so dofork.o -lpthread

Now, ordinarily I would have continued to advise that you link with

  -Wl,--error-unresolved-symbols

so that these cases are caught at link time.

HOWEVER!  That will not help here.

In fact, libdofork.so *is* being built correctly, and properly binds to
fork@GLIBC_2.2.5, as present in the libc.so library.

Except that at runtime, we bind to the symbol of the same version as present in
libpthread.so.

We made a mistake when removing the fork and vfork symbols from libpthread.
While we did demote the symbol version in libpthread from default to
compatibility, we should have bumped the symbol version in libc at the same
time.  This would have prevented the symbol in libpthread from being considered
for new binaries, forcing the symbol to be resolved from libc.

Come to think of it, we had the same situation of the same symbol version being
provided by two libraries prior to removing fork from libpthread too, so when
providing compatibility symbols we must be mindful of that -- proper
dependencies on libpthread may well not exist.

Which means that using IFUNC to implement the compatibility symbols isn't an
option, as demonstrated by this PR.

So, while we can provide a generic fallback via a simple wrapper for fork, this
situation must be resolved on a per-port basis for vfork.  Either provide a
stub that performs a tail-call, or simply include the full code for vfork
within libpthread.


r~

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Florian Weimer]

On 03/30/2016 09:51 PM, Richard Henderson wrote:

> Come to think of it, we had the same situation of the same symbol version being
> provided by two libraries prior to removing fork from libpthread too, so when
> providing compatibility symbols we must be mindful of that -- proper
> dependencies on libpthread may well not exist.

Considering that libpthread has a DT_NEEDED entry for libc.so.6, doe we 
actually need the compatibility wrapper?

I removed it, built a special libdofork.so which has a DT_NEEDED entry 
for libpthread.so.0 only, and got this with a glibc built that doesn't 
have the compatibility wrapper:

      12074:     symbol=fork;  lookup in file=/tmp/boom [0]
      12074:     symbol=fork;  lookup in file=./nptl/libpthread.so.0 [0]
      12074:     symbol=fork;  lookup in file=./libdofork.so [0]
      12074:     symbol=fork;  lookup in file=./libc.so.6 [0]
      12074:     binding file ./libdofork.so [0] to ./libc.so.6 [0]: 
normal symbol `fork' [GLIBC_2.2.5]

So it seems to work just as before.

The difference will be visible with dlopen, but will that matter?

Florian

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> I removed it, built a special libdofork.so which has a DT_NEEDED entry for
> libpthread.so.0 only,

Did you build libdofork before removing the wrapper?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861]

[Florian Weimer]

On 04/11/2016 11:57 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> I removed it, built a special libdofork.so which has a DT_NEEDED entry for
>> libpthread.so.0 only,
>
> Did you build libdofork before removing the wrapper?

Yes, I did, and I linked explicitly with -lpthread.  After that, I 
patched out the DT_NEEDED entry:

Dynamic segment contains 31 entries:
  Addr: 0x0000000000200db8  Offset: 0x000db8  Link to section: [ 4] 
'.dynstr'
   Type              Value
   NEEDED            Shared library: [libpthread.so.0]
   <unknown>: 0xff   0x000000000000008c

However, the symbol version is stilled mapped to libc.so.6:

Version symbols section [ 5] '.gnu.version' contains 14 entries:
  Addr: 0x000000000000043a  Offset: 0x00043a  Link to section: [ 3] 
'.dynsym'
     0:   0 *local*                        0 *local*
     2:   0 *local*                        0 *local*
     4:   0 *local*                        0 *local*
     6:   2 GLIBC_2.2.5(libc.so.6)         2 GLIBC_2.2.5(libc.so.6)
     8:   1 *global*                       1 *global*
    10:   1 *global*                       1 *global*
    12:   1 *global*                       1 *global*

Version needs section [ 6] '.gnu.version_r' contains 1 entry:
  Addr: 0x0000000000000458  Offset: 0x000458  Link to section: [ 4] 
'.dynstr'
   000000: Version: 1  File: libc.so.6  Cnt: 1
   0x0010: Name: GLIBC_2.2.5  Flags: none  Version: 2

I don't know how t get rid of that.

Florian