| Message ID | CALoOobOt4q05ht82tWw3P=+TgR5sp3HW=DFpJC57O9knXzO6iQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri 21 Mar 2014 15:39:13 Paul Pluzhnikov wrote: > On Sun, Mar 16, 2014 at 3:51 AM, Siddhesh Poyarekar wrote: > > On Fri, Mar 14, 2014 at 02:54:05PM -0700, Paul Pluzhnikov wrote: > >> Attached patch fixes BZ #16634 by moving sanity check for dlopen()ing > >> a.out before we call _dl_next_tls_modid() for it. > >> > >> Tested on Linux/x86_64; no new failures. > > > > A more detailed description please, assuming that it's going to be > > used for the commit log. > > I usually just use ChangeLog as commit log. > Should I be using more verbose patch description instead? yes, your commit log needs to be useful by itself. see b36208627c3cd3b5f031a4e42d0f21211f7ccba0 as an example. whether the ChangeLog is posted inline is up to the respective committer at this time. -mike
On 22 March 2014 04:09, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > I usually just use ChangeLog as commit log. > Should I be using more verbose patch description instead? What Mike said - it makes things much easier for us distro folks to do backports and definitely infinitely easier for someone looking for the rationale of a specific change. > An application that erroneously tries to repeatedly dlopen("a.out", ...) > hits assertion failure: > > Inconsistency detected by ld.so: dl-tls.c: 474: _dl_allocate_tls_init: > Assertion `listp != ((void *)0)' failed! > > dlopen() actually fails with "./a.out: cannot dynamically load executable", > but it does so after incrementing dl_tls_max_dtv_idx. > > Once we run out of TLS_SLOTINFO_SURPLUS (62), we crash. This is good, thanks. >> Use __glibc_unlikely. > > There are a lot of __builtin_expect()s in this file. > > I would prefer to convert them to __glibc_{un}likely in a separate pass. OK, but please don't forget ;) > 2014-03-21 Paul Pluzhnikov <ppluzhnikov@google.com> > > [BZ #16634] > * elf/dl-load.c (open_verify): Add mode parameter. > Error early when ET_EXEC and mode does not have __RTLD_OPENEXEC. > (open_path): Change from boolean 'secure' to complete flag 'mode' > (_dl_map_object): Adjust. The patch looks OK to me, but (I'm sorry it didn't occur to me in my initial review) shouldn't there be a test case for this? I think you could adapt the reproducer in the bz into a test case. Also, please try to always post the patch inline like you did originally - an attached patch does not come up in the reply template and due to that it is a bit cumbersome to comment on snippets of patches. Thanks, Siddhesh
On Fri, Mar 21, 2014 at 10:47 PM, Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> wrote: >> 2014-03-21 Paul Pluzhnikov <ppluzhnikov@google.com> >> >> [BZ #16634] >> * elf/dl-load.c (open_verify): Add mode parameter. >> Error early when ET_EXEC and mode does not have __RTLD_OPENEXEC. >> (open_path): Change from boolean 'secure' to complete flag 'mode' >> (_dl_map_object): Adjust. > > The patch looks OK to me, but (I'm sorry it didn't occur to me in my > initial review) shouldn't there be a test case for this? I think you > could adapt the reproducer in the bz into a test case. As mentioned in bugzilla, the bug doesn't show up when invoked via ./elf/ld.so --library-path ... pr16634 and only shows up if the binary is linked with gcc -pthread -ldl -Wl,-rpath=.:nptl/l:dlfcn,--dynamic-linker=elf/ld.so I've spent some time tracing through ld.so to understand why that is. In the former case, _dl_map_object_from_fd (which is where the assert fires) does not get invoked. It looks like there is some way to link the test with --dynamic-linker; I'll look some more into creating a reproducer. > Also, please try to always post the patch inline like you did > originally That is unfortunately impossible with GMail :-(
On 23 March 2014 22:25, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > > gcc -pthread -ldl -Wl,-rpath=.:nptl/l:dlfcn,--dynamic-linker=elf/ld.so IIRC we do -rpath-link=... --dynamic-linker=elf/ld.so ... which unfortunately is useless during execution and the binary still needs to be invoked through the linker. I wonder why we don't just use -rpath. > That is unfortunately impossible with GMail :-( Maybe use another email service? ;) Siddhesh
Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> writes: > On 23 March 2014 22:25, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >> >> gcc -pthread -ldl -Wl,-rpath=.:nptl/l:dlfcn,--dynamic-linker=elf/ld.so > > IIRC we do -rpath-link=... --dynamic-linker=elf/ld.so ... The tests are linked like any other binary unless you use --enable-hardcoded-path-in-tests. Andreas.
On Sun, Mar 23, 2014 at 10:23 AM, Andreas Schwab <schwab@linux-m68k.org> wrote: > Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com> writes: > >> On 23 March 2014 22:25, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: >>> >>> gcc -pthread -ldl -Wl,-rpath=.:nptl/l:dlfcn,--dynamic-linker=elf/ld.so >> >> IIRC we do -rpath-link=... --dynamic-linker=elf/ld.so ... > > The tests are linked like any other binary unless you use > --enable-hardcoded-path-in-tests. I have verified that --enable-hardcoded-path-in-tests allows me to have a reproducer for BZ#16634. Should '--enable-hardcoded-path-in-tests' be made the default when not cross-compiling?
On Sun, Mar 23, 2014 at 02:36:15PM -0700, Paul Pluzhnikov wrote: > Should '--enable-hardcoded-path-in-tests' be made the default when not > cross-compiling? I would be in favour of this, but this needs wider consensus. For now, since the test is not completely useless (i.e. it does its job in at least one configuration), it should be OK to add it with a comment explicitly mentioning that the test is only useful with --enable-hardcoded-path-in-tests. Once we have consensus either way on the default, it would only potentially improve the situation. Siddhesh
On 03/23/2014 04:55 PM, Paul Pluzhnikov wrote: > >> > Also, please try to always post the patch inline like you did >> > originally > That is unfortunately impossible with GMail :-( FWIW, Maybe with gmail-the-web-client, but quite possible if one sends patches through the gmail smtp server. The easiest way being to send patches with 'git send-mail', which is a good idea anyway, but any mail client other than the web client should do.
On Mon, Mar 24, 2014 at 2:54 AM, Pedro Alves <palves@redhat.com> wrote: > On 03/23/2014 04:55 PM, Paul Pluzhnikov wrote: > >> That is unfortunately impossible with GMail :-( > Maybe with gmail-the-web-client, but quite possible if one sends > patches through the gmail smtp server. Correct. > The easiest way being > to send patches with 'git send-mail', which is a good idea anyway, Right. I'll try that workflow, thanks for reminding me of it.
diff --git a/elf/dl-load.c b/elf/dl-load.c index 8ebc128..9455df2 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1667,7 +1667,7 @@ print_search_path (struct r_search_path_elem **list, user might want to know about this. */ static int open_verify (const char *name, struct filebuf *fbp, struct link_map *loader, - int whatcode, bool *found_other_class, bool free_name) + int whatcode, int mode, bool *found_other_class, bool free_name) { /* This is the expected ELF header. */ #define ELF32_CLASS ELFCLASS32 @@ -1843,6 +1843,17 @@ open_verify (const char *name, struct filebuf *fbp, struct link_map *loader, errstring = N_("only ET_DYN and ET_EXEC can be loaded"); goto call_lose; } + else if (__glibc_unlikely (ehdr->e_type == ET_EXEC + && (mode & __RTLD_OPENEXEC) == 0)) + { + /* BZ #16634. It is an error to dlopen ET_EXEC (unless + __RTLD_OPENEXEC is explicitly set). We return error here + so that code in _dl_map_object_from_fd does not try to set + l_tls_modid for this module. */ + + errstring = N_("cannot dynamically load executable"); + goto call_lose; + } else if (__builtin_expect (ehdr->e_phentsize, sizeof (ElfW(Phdr))) != sizeof (ElfW(Phdr))) { @@ -1928,7 +1939,7 @@ open_verify (const char *name, struct filebuf *fbp, struct link_map *loader, if MAY_FREE_DIRS is true. */ static int -open_path (const char *name, size_t namelen, int secure, +open_path (const char *name, size_t namelen, int mode, struct r_search_path_struct *sps, char **realname, struct filebuf *fbp, struct link_map *loader, int whatcode, bool *found_other_class) @@ -1980,8 +1991,8 @@ open_path (const char *name, size_t namelen, int secure, if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS)) _dl_debug_printf (" trying file=%s\n", buf); - fd = open_verify (buf, fbp, loader, whatcode, found_other_class, - false); + fd = open_verify (buf, fbp, loader, whatcode, mode, + found_other_class, false); if (this_dir->status[cnt] == unknown) { if (fd != -1) @@ -2010,7 +2021,7 @@ open_path (const char *name, size_t namelen, int secure, /* Remember whether we found any existing directory. */ here_any |= this_dir->status[cnt] != nonexisting; - if (fd != -1 && __builtin_expect (secure, 0) + if (fd != -1 && __builtin_expect (mode & __RTLD_SECURE, 0) && INTUSE(__libc_enable_secure)) { /* This is an extra security effort to make sure nobody can @@ -2184,7 +2195,7 @@ _dl_map_object (struct link_map *loader, const char *name, for (l = loader; l; l = l->l_loader) if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH")) { - fd = open_path (name, namelen, mode & __RTLD_SECURE, + fd = open_path (name, namelen, mode, &l->l_rpath_dirs, &realname, &fb, loader, LA_SER_RUNPATH, &found_other_class); @@ -2200,7 +2211,7 @@ _dl_map_object (struct link_map *loader, const char *name, && main_map != NULL && main_map->l_type != lt_loaded && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH, "RPATH")) - fd = open_path (name, namelen, mode & __RTLD_SECURE, + fd = open_path (name, namelen, mode, &main_map->l_rpath_dirs, &realname, &fb, loader ?: main_map, LA_SER_RUNPATH, &found_other_class); @@ -2208,7 +2219,7 @@ _dl_map_object (struct link_map *loader, const char *name, /* Try the LD_LIBRARY_PATH environment variable. */ if (fd == -1 && env_path_list.dirs != (void *) -1) - fd = open_path (name, namelen, mode & __RTLD_SECURE, &env_path_list, + fd = open_path (name, namelen, mode, &env_path_list, &realname, &fb, loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded, LA_SER_LIBPATH, &found_other_class); @@ -2217,7 +2228,7 @@ _dl_map_object (struct link_map *loader, const char *name, if (fd == -1 && loader != NULL && cache_rpath (loader, &loader->l_runpath_dirs, DT_RUNPATH, "RUNPATH")) - fd = open_path (name, namelen, mode & __RTLD_SECURE, + fd = open_path (name, namelen, mode, &loader->l_runpath_dirs, &realname, &fb, loader, LA_SER_RUNPATH, &found_other_class); @@ -2267,7 +2278,8 @@ _dl_map_object (struct link_map *loader, const char *name, { fd = open_verify (cached, &fb, loader ?: GL(dl_ns)[nsid]._ns_loaded, - LA_SER_CONFIG, &found_other_class, false); + LA_SER_CONFIG, mode, &found_other_class, + false); if (__glibc_likely (fd != -1)) { realname = local_strdup (cached); @@ -2287,7 +2299,7 @@ _dl_map_object (struct link_map *loader, const char *name, && ((l = loader ?: GL(dl_ns)[nsid]._ns_loaded) == NULL || __builtin_expect (!(l->l_flags_1 & DF_1_NODEFLIB), 1)) && rtld_search_dirs.dirs != (void *) -1) - fd = open_path (name, namelen, mode & __RTLD_SECURE, &rtld_search_dirs, + fd = open_path (name, namelen, mode, &rtld_search_dirs, &realname, &fb, l, LA_SER_DEFAULT, &found_other_class); /* Add another newline when we are tracing the library loading. */ @@ -2305,7 +2317,7 @@ _dl_map_object (struct link_map *loader, const char *name, else { fd = open_verify (realname, &fb, - loader ?: GL(dl_ns)[nsid]._ns_loaded, 0, + loader ?: GL(dl_ns)[nsid]._ns_loaded, 0, mode, &found_other_class, true); if (__builtin_expect (fd, 0) == -1) free (realname);