Patchwork elf: Never use the file ID of the main executable [BZ #24900]

login
register
mail settings
Submitter Florian Weimer
Date Aug. 22, 2019, 4:54 p.m.
Message ID <87pnkxw36k.fsf@oldenburg2.str.redhat.com>
Download mbox | patch
Permalink /patch/34246/
State New
Headers show

Comments

Florian Weimer - Aug. 22, 2019, 4:54 p.m.
If the loader is invoked explicitly and loads the main executable,
it stores the file ID of the main executable in l_file_id.  This
information is not available if the main excutable is loaded by the
kernel, so this is another case where the two cases differ.

This enhances commit 23d2e5faf0bca6d9b31bef4aa162b95ee64cbfc6
("elf: Self-dlopen failure with explict loader invocation
[BZ #24900]").

2019-08-22  Florian Weimer  <fweimer@redhat.com>

	[BZ #24900]
	* elf/dl-load.c (_dl_map_object_from_fd): Do not use the file ID
	when loading the executable as part of an explicit loader
	invocation.
Gabriel F. T. Gomes - Sept. 23, 2019, 2:21 p.m.
Hi, Florian,

On Thu, 22 Aug 2019, Florian Weimer wrote:

>If the loader is invoked explicitly and loads the main executable,
>it stores the file ID of the main executable in l_file_id.  This
>information is not available if the main excutable is loaded by the
>kernel, so this is another case where the two cases differ.
>
>This enhances commit 23d2e5faf0bca6d9b31bef4aa162b95ee64cbfc6
>("elf: Self-dlopen failure with explict loader invocation
>[BZ #24900]").

As we discussed at Cauldron, I had reviewed this patch and noticed that it
changes the place where elf/tst-dlopen-aout fails.

Before this patch, it fails in the test case itself , at:

error: tst-dlopen-aout.c:48: dlopen succeeded unexpectedly: elf/tst-dlopen-aout

After this patch, it fails at:

elf/dl-tls.c: 517: _dl_allocate_tls_init: Assertion `listp != NULL' failed!

which, as you explained to me at Cauldron, is expected and will only be
solved with a sebsequent patch [1].  As you also mentioned to me at
Cauldron, reviewing the second patch is a little tricky, but I'll try to
do it.

[1] https://sourceware.org/ml/libc-alpha/2019-08/msg00625.html

>-  /* Get file information.  */
>+  /* Get file information.  To match the kernel behavior, do not fill
>+     in this information for the executable in case of an explicit
>+     loader invocation.  */
>   struct r_file_id id;
>+  if (mode & __RTLD_OPENEXEC)
>+    {
>+      assert (nsid == LM_ID_BASE);
>+      memset (&id, 0, sizeof (id));
>+    }
>+  else
>+    {

OK.

I think that this patch does the right thing and could be committed.

Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>

Thanks!

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 5abeb867f1..7bf0ca563e 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -876,33 +876,43 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
   struct r_debug *r = _dl_debug_initialize (0, nsid);
   bool make_consistent = false;
 
-  /* Get file information.  */
+  /* Get file information.  To match the kernel behavior, do not fill
+     in this information for the executable in case of an explicit
+     loader invocation.  */
   struct r_file_id id;
-  if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
+  if (mode & __RTLD_OPENEXEC)
     {
-      errstring = N_("cannot stat shared object");
-    call_lose_errno:
-      errval = errno;
-    call_lose:
-      lose (errval, fd, name, realname, l, errstring,
-	    make_consistent ? r : NULL, nsid);
+      assert (nsid == LM_ID_BASE);
+      memset (&id, 0, sizeof (id));
     }
+  else
+    {
+      if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
+	{
+	  errstring = N_("cannot stat shared object");
+	call_lose_errno:
+	  errval = errno;
+	call_lose:
+	  lose (errval, fd, name, realname, l, errstring,
+		make_consistent ? r : NULL, nsid);
+	}
 
-  /* Look again to see if the real name matched another already loaded.  */
-  for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
-    if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
-      {
-	/* The object is already loaded.
-	   Just bump its reference count and return it.  */
-	__close_nocancel (fd);
+      /* Look again to see if the real name matched another already loaded.  */
+      for (l = GL(dl_ns)[nsid]._ns_loaded; l != NULL; l = l->l_next)
+	if (!l->l_removed && _dl_file_id_match_p (&l->l_file_id, &id))
+	  {
+	    /* The object is already loaded.
+	       Just bump its reference count and return it.  */
+	    __close_nocancel (fd);
 
-	/* If the name is not in the list of names for this object add
-	   it.  */
-	free (realname);
-	add_name_to_object (l, name);
+	    /* If the name is not in the list of names for this object add
+	       it.  */
+	    free (realname);
+	    add_name_to_object (l, name);
 
-	return l;
-      }
+	    return l;
+	  }
+    }
 
 #ifdef SHARED
   /* When loading into a namespace other than the base one we must