[PATCH] Fix relocation dependency handling

[PATCH] Fix relocation dependency handling

[Jakub Jelinek]

Hi!

The following is an attempt to fix
http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=63422
The problem is that at least if GL(dl_dynamic_weak) != 0 (but I think in
STV_PROTECTED handling this might happen as well), we don't call
add_dependency at all if we don't find a non-weak definition. We resolve
to the first weak dependency, and if this one is in a dlopened library
and that library is later on dlclosed, we end up with relocations pointing
to unmapped memory. Particularly:
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=./a.out
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libSDL-1.2.so.0
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/i686/libpthread.so.0
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/i686/libc.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/i686/libm.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/X11R6/lib/libX11.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/X11R6/lib/libXext.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/libdl.so.2
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/ld-linux.so.2
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libartscbackend.so.0
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libartsflow.so.1
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libsoundserver_idl.so.1
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libkmedia2_idl.so.1
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libaudiofile.so.0
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libartsflow_idl.so.1
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libmcop.so.1
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/libresolv.so.2
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/libdl.so.2
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/usr/lib/libstdc++-libc6.2-2.so.3
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/i686/libm.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/i686/libc.so.6
28774:  symbol=_t24__default_alloc_template2b1i0._S_node_allocator_lock;  lookup in file=/lib/ld-linux.so.2
28774:  binding file /usr/lib/libmcop.so.1 to /usr/lib/libartscbackend.so.0: normal symbol `_t24__default_alloc_template2b1i0._S_node_allocator_lock'

(note no
00746:  file=/usr/lib/libartscbackend.so.0;  needed by /usr/lib/libmcop.so.1 (relocation dependency)
line neither here nor anywhere before this place).
The patch below moves the add_dependency call and related code to the end of
the functions to the point where we unconditionally return current_value
as result. So far I've just lightly tested it on glibc tree from 4 days ago,
ie. predating your dl-lookup.c etc. changes, and it worked for this case as
expected.
While forward porting it to current dl-lookup.c, I've noticed you haven't changed
the last argument in the recursive calls to _dl_lookup_*symbol if add_dependency
fails. Previously it was passing 0 since code above it ensured ! explicit, so
it was just a more efficient way of passing explicit.
But now that explicit is (flags & DL_LOOKUP_ADD_DEPENDENCY) != 0, I think we need
to pass in flags, not 0 (both because of the additional bit fields and because
0 now means former !explicit.

2002-04-14  Jakub Jelinek  <jakub@redhat.com>

	* elf/dl-lookup.c (_dl_lookup_symbol): Move add_dependency call to
	the end of the function.  Pass original flags to recursive call if
	add_dependency failed.
	(_dl_lookup_versioned_symbol): Likewise.

--- libc/elf/dl-lookup.c.jj	Sun Apr 14 22:14:09 2002
+++ libc/elf/dl-lookup.c	Sun Apr 14 22:17:53 2002
@@ -228,24 +228,7 @@ _dl_lookup_symbol (const char *undef_nam
   for (scope = symbol_scope; *scope; ++scope)
     if (do_lookup (undef_name, hash, *ref, &current_value, *scope, 0, flags,
 		   NULL, type_class))
-      {
-	/* We have to check whether this would bind UNDEF_MAP to an object
-	   in the global scope which was dynamically loaded.  In this case
-	   we have to prevent the latter from being unloaded unless the
-	   UNDEF_MAP object is also unloaded.  */
-	if (__builtin_expect (current_value.m->l_type == lt_loaded, 0)
-	    /* Don't do this for explicit lookups as opposed to implicit
-	       runtime lookups.  */
-	    && (flags & DL_LOOKUP_ADD_DEPENDENCY) != 0
-	    /* Add UNDEF_MAP to the dependencies.  */
-	    && add_dependency (undef_map, current_value.m) < 0)
-	  /* Something went wrong.  Perhaps the object we tried to reference
-	     was just removed.  Try finding another definition.  */
-	  return INTUSE(_dl_lookup_symbol) (undef_name, undef_map, ref,
-					    symbol_scope, type_class, 0);
-
-	break;
-      }
+      break;
 
   if (__builtin_expect (current_value.s == NULL, 0))
     {
@@ -282,6 +265,21 @@ _dl_lookup_symbol (const char *undef_nam
 	}
     }
 
+  /* We have to check whether this would bind UNDEF_MAP to an object
+     in the global scope which was dynamically loaded.  In this case
+     we have to prevent the latter from being unloaded unless the
+     UNDEF_MAP object is also unloaded.  */
+  if (__builtin_expect (current_value.m->l_type == lt_loaded, 0)
+      /* Don't do this for explicit lookups as opposed to implicit
+	 runtime lookups.  */
+      && (flags & DL_LOOKUP_ADD_DEPENDENCY) != 0
+      /* Add UNDEF_MAP to the dependencies.  */
+      && add_dependency (undef_map, current_value.m) < 0)
+      /* Something went wrong.  Perhaps the object we tried to reference
+	 was just removed.  Try finding another definition.  */
+      return INTUSE(_dl_lookup_symbol) (undef_name, undef_map, ref,
+					symbol_scope, type_class, flags);
+
   if (__builtin_expect (GL(dl_debug_mask)
 			& (DL_DEBUG_BINDINGS|DL_DEBUG_PRELINK), 0))
     _dl_debug_bindings (undef_name, undef_map, ref, symbol_scope,
@@ -395,26 +393,7 @@ _dl_lookup_versioned_symbol (const char 
       int res = do_lookup_versioned (undef_name, hash, *ref, &current_value,
 				     *scope, 0, version, NULL, type_class);
       if (res > 0)
-	{
-	  /* We have to check whether this would bind UNDEF_MAP to an object
-	     in the global scope which was dynamically loaded.  In this case
-	     we have to prevent the latter from being unloaded unless the
-	     UNDEF_MAP object is also unloaded.  */
-	  if (__builtin_expect (current_value.m->l_type == lt_loaded, 0)
-	      /* Don't do this for explicit lookups as opposed to implicit
-		 runtime lookups.  */
-	      && flags != 0
-	      /* Add UNDEF_MAP to the dependencies.  */
-	      && add_dependency (undef_map, current_value.m) < 0)
-	    /* Something went wrong.  Perhaps the object we tried to reference
-	       was just removed.  Try finding another definition.  */
-	    return INTUSE(_dl_lookup_versioned_symbol) (undef_name, undef_map,
-							ref, symbol_scope,
-							version, type_class,
-							0);
-
-	  break;
-	}
+	break;
 
       if (__builtin_expect (res, 0) < 0)
 	{
@@ -479,6 +458,22 @@ _dl_lookup_versioned_symbol (const char 
 	}
     }
 
+  /* We have to check whether this would bind UNDEF_MAP to an object
+     in the global scope which was dynamically loaded.  In this case
+     we have to prevent the latter from being unloaded unless the
+     UNDEF_MAP object is also unloaded.  */
+  if (__builtin_expect (current_value.m->l_type == lt_loaded, 0)
+      /* Don't do this for explicit lookups as opposed to implicit
+	 runtime lookups.  */
+      && flags != 0
+      /* Add UNDEF_MAP to the dependencies.  */
+      && add_dependency (undef_map, current_value.m) < 0)
+      /* Something went wrong.  Perhaps the object we tried to reference
+	 was just removed.  Try finding another definition.  */
+      return INTUSE(_dl_lookup_versioned_symbol) (undef_name, undef_map,
+						  ref, symbol_scope,
+						  version, type_class, flags);
+
   if (__builtin_expect (GL(dl_debug_mask)
 			& (DL_DEBUG_BINDINGS|DL_DEBUG_PRELINK), 0))
     _dl_debug_bindings (undef_name, undef_map, ref, symbol_scope,

	Jakub

Re: [PATCH] Fix relocation dependency handling

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Sun, 2002-04-14 at 13:25, Jakub Jelinek wrote:

> 2002-04-14  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* elf/dl-lookup.c (_dl_lookup_symbol): Move add_dependency call to
> 	the end of the function.  Pass original flags to recursive call if
> 	add_dependency failed.
> 	(_dl_lookup_versioned_symbol): Likewise.

This patch looks OK, I've applied it.  Thanks,

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] Fix relocation dependency handling

[Andreas Schwab]

Ulrich Drepper <drepper@redhat.com> writes:

|> On Sun, 2002-04-14 at 13:25, Jakub Jelinek wrote:
|> 
|> > 2002-04-14  Jakub Jelinek  <jakub@redhat.com>
|> > 
|> > 	* elf/dl-lookup.c (_dl_lookup_symbol): Move add_dependency call to
|> > 	the end of the function.  Pass original flags to recursive call if
|> > 	add_dependency failed.
|> > 	(_dl_lookup_versioned_symbol): Likewise.
|> 
|> This patch looks OK, I've applied it.  Thanks,

See also http://sources.redhat.com/ml/libc-hacker/2002-02/msg00121.html.
Why didn't you just check in this patch?  It even contains a test!

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE GmbH, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix relocation dependency handling

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Mon, 2002-04-15 at 01:29, Andreas Schwab wrote:

> See also http://sources.redhat.com/ml/libc-hacker/2002-02/msg00121.html.
> Why didn't you just check in this patch?  It even contains a test!

I don't have this mail anywhere in my mail folders.  Fact is that I get
far too much mail and this unfortunately sometimes means that something
gets lost.  Especially if it's sent in a time when I'm away or working
on something without allowing distractions.

If you cannot understand this you might want collect the references to
your outstanding mails and resend this list regularly.

-- 
---------------.                          ,-.   1325 Chesapeake Terrace
Ulrich Drepper  \    ,-------------------'   \  Sunnyvale, CA 94089 USA
Red Hat          `--' drepper at redhat.com   `------------------------

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 232 bytes --]