Fix BZ #16634 -- assert in ld.so when dlopen("a.out"...) is called repeatedly.

Message ID CALoOobOt4q05ht82tWw3P=+TgR5sp3HW=DFpJC57O9knXzO6iQ@mail.gmail.com
State Committed
Delegated to: Siddhesh Poyarekar
Headers

Commit Message

Paul Pluzhnikov March 21, 2014, 10:39 p.m. UTC
  On Sun, Mar 16, 2014 at 3:51 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Fri, Mar 14, 2014 at 02:54:05PM -0700, Paul Pluzhnikov wrote:
>> Greetings,
>>
>> 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?

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.

>>       BZ #16634
>
> In square brackets.

Done.

>> +       /* For BZ #16634, return early.  */
>
> Likewise, please explain the condition instead of just quoting the bz
> number.

Done.

>> -       if (fd != -1 && __builtin_expect (secure, 0)
>> +       if (fd != -1 && __builtin_expect (mode & __RTLD_SECURE, 0)
>
> 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.

Thanks,
  

Comments

Mike Frysinger March 22, 2014, 4:35 a.m. UTC | #1
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
  
Patchwork Bot March 22, 2014, 5:47 a.m. UTC | #2
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
  
Paul Pluzhnikov March 23, 2014, 4:55 p.m. UTC | #3
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 :-(
  
Patchwork Bot March 23, 2014, 5:03 p.m. UTC | #4
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
  
Andreas Schwab March 23, 2014, 5:23 p.m. UTC | #5
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.
  
Paul Pluzhnikov March 23, 2014, 9:36 p.m. UTC | #6
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?
  
Siddhesh Poyarekar March 24, 2014, 8:31 a.m. UTC | #7
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
  
Pedro Alves March 24, 2014, 9:54 a.m. UTC | #8
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.
  
Paul Pluzhnikov March 24, 2014, 1:54 p.m. UTC | #9
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.
  

Patch

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);