* Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] @ 2016-03-30 13:19 Florian Weimer 2016-03-30 13:38 ` Andreas Schwab 2016-03-30 16:47 ` Richard Henderson 0 siblings, 2 replies; 8+ messages in thread From: Florian Weimer @ 2016-03-30 13:19 UTC (permalink / raw) To: GNU C Library; +Cc: Richard Henderson 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-03-30 13:19 Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] Florian Weimer @ 2016-03-30 13:38 ` Andreas Schwab 2016-03-30 16:47 ` Richard Henderson 1 sibling, 0 replies; 8+ messages in thread From: Andreas Schwab @ 2016-03-30 13:38 UTC (permalink / raw) To: Florian Weimer; +Cc: GNU C Library, Richard Henderson 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." ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-03-30 13:19 Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] Florian Weimer 2016-03-30 13:38 ` Andreas Schwab @ 2016-03-30 16:47 ` Richard Henderson 2016-03-30 19:10 ` Szabolcs Nagy 1 sibling, 1 reply; 8+ messages in thread From: Richard Henderson @ 2016-03-30 16:47 UTC (permalink / raw) To: Florian Weimer, GNU C Library 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~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-03-30 16:47 ` Richard Henderson @ 2016-03-30 19:10 ` Szabolcs Nagy 2016-03-30 19:51 ` Richard Henderson 0 siblings, 1 reply; 8+ messages in thread From: Szabolcs Nagy @ 2016-03-30 19:10 UTC (permalink / raw) To: Richard Henderson; +Cc: Florian Weimer, GNU C Library * 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.) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-03-30 19:10 ` Szabolcs Nagy @ 2016-03-30 19:51 ` Richard Henderson 2016-04-11 9:34 ` Florian Weimer 0 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2016-03-30 19:51 UTC (permalink / raw) To: Szabolcs Nagy; +Cc: Florian Weimer, GNU C Library, Roland McGrath 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~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-03-30 19:51 ` Richard Henderson @ 2016-04-11 9:34 ` Florian Weimer 2016-04-11 9:58 ` Andreas Schwab 0 siblings, 1 reply; 8+ messages in thread From: Florian Weimer @ 2016-04-11 9:34 UTC (permalink / raw) To: Richard Henderson, Szabolcs Nagy; +Cc: GNU C Library, Roland McGrath 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-04-11 9:34 ` Florian Weimer @ 2016-04-11 9:58 ` Andreas Schwab 2016-04-11 10:02 ` Florian Weimer 0 siblings, 1 reply; 8+ messages in thread From: Andreas Schwab @ 2016-04-11 9:58 UTC (permalink / raw) To: Florian Weimer Cc: Richard Henderson, Szabolcs Nagy, GNU C Library, Roland McGrath 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." ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] 2016-04-11 9:58 ` Andreas Schwab @ 2016-04-11 10:02 ` Florian Weimer 0 siblings, 0 replies; 8+ messages in thread From: Florian Weimer @ 2016-04-11 10:02 UTC (permalink / raw) To: Andreas Schwab Cc: Richard Henderson, Szabolcs Nagy, GNU C Library, Roland McGrath 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-04-11 10:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-30 13:19 Incorrect IFUNC use in libpthread for fork, vfork wrapper [BZ #19861] Florian Weimer 2016-03-30 13:38 ` Andreas Schwab 2016-03-30 16:47 ` Richard Henderson 2016-03-30 19:10 ` Szabolcs Nagy 2016-03-30 19:51 ` Richard Henderson 2016-04-11 9:34 ` Florian Weimer 2016-04-11 9:58 ` Andreas Schwab 2016-04-11 10:02 ` Florian Weimer
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 read-only IMAP folder(s) and NNTP newsgroup(s).