[PATCH] Fix relocation dependency handling

[Jakub Jelinek <jakub at redhat dot com>]

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