i#4501: In module_lookup_symbol skip other client modules. (#6976)

In module_lookup_symbol, which is invoked from DynamoRIO's private
loader, skip other top-level client modules (but not
extensions/libraries) because some will not be initialised and top-level
clients should be leaves of the dependency tree and not provide symbols
for other modules. Top-level client modules are recognised using a new
module flag, is_top_level_client.

Also add a test that two clients can be loaded without a crash.

Fixes #4501

core/loader_shared.c

@@ -178,6 +178,7 @@ loader_init_prologue(void)
         privmod_t *mod =
             privload_insert(NULL, privmod_static[i].base, privmod_static[i].size,
                             privmod_static[i].name, privmod_static[i].path);
+        mod->is_top_level_client = true;
         mod->is_client = true;
     }
 
@@ -520,6 +521,7 @@ privload_insert(privmod_t *after, app_pc base, size_t size, const char *name,
     }
     mod->ref_count = 1;
     mod->externally_loaded = false;
+    mod->is_top_level_client = false; /* up to caller to set later */
     mod->is_client = false; /* up to caller to set later */
     mod->called_proc_entry = false;
     mod->called_proc_exit = false;

core/module_shared.h

@@ -413,7 +413,9 @@ typedef struct _privmod_t {
     char path[MAXIMUM_PATH];
     uint ref_count;
     bool externally_loaded;
-    bool is_client; /* or Extension */
+    /* XXX i#6982: Perhaps replace is_client with is_top_level_client. */
+    bool is_top_level_client; /* set for command-line clients */
+    bool is_client;           /* set for command-line client or extension */
     bool called_proc_entry;
     bool called_proc_exit;
     struct _privmod_t *next;

core/unix/loader.c

@@ -581,9 +581,9 @@ privload_process_imports(privmod_t *mod)
                     SYSLOG_INTERNAL_WARNING(
                         "private libpthread.so loaded but not fully supported (i#956)");
                 }
-                /* i#852: identify all libs that import from DR as client libs.
-                 * XXX: this code seems stale as libdynamorio.so is already loaded
-                 * (xref #3850).
+                /* i#852: Identify all libs that import from DR as client libs.
+                 * XXX i#6982: The following condition is never true as
+                 * libdynamorio.so has already been loaded (xref #3850).
                  */
                 if (impmod->base == get_dynamorio_dll_start())
                     mod->is_client = true;

core/unix/module_elf.c

@@ -1104,7 +1104,6 @@ module_lookup_symbol(ELF_SYM_TYPE *sym, os_privmod_data_t *pd)
 {
     app_pc res;
     const char *name;
-    privmod_t *mod;
     bool is_ifunc;
     dcontext_t *dcontext = get_thread_private_dcontext();
 
@@ -1133,11 +1132,21 @@ module_lookup_symbol(ELF_SYM_TYPE *sym, os_privmod_data_t *pd)
      * FIXME: i#461 We do not tell weak/global, but return on the first we see.
      */
     ASSERT_OWN_RECURSIVE_LOCK(true, &privload_lock);
-    mod = privload_first_module();
     /* FIXME i#3850: Symbols are currently looked up following the dependency chain
      * depth-first instead of breadth-first.
      */
-    while (mod != NULL) {
+    for (privmod_t *mod = privload_first_module(); mod != NULL;
+         mod = privload_next_module(mod)) {
+        /* Skip other client modules at this point because some will not be
+         * initialised and clients should be leaves of the dependency tree and
+         * not provide symbols for other modules. Skipping just the uninitialised
+         * client modules should also work but might introduce an element of
+         * unpredictability if we are unsure in what order modules will be
+         * initialised. Skipping all uninitialised modules should also work but
+         * might hide a more serious problem. See i#4501.
+         */
+        if (mod->is_top_level_client)
+            continue;
         pd = mod->os_privmod_data;
         ASSERT(pd != NULL && name != NULL);
 
@@ -1172,7 +1181,6 @@ module_lookup_symbol(ELF_SYM_TYPE *sym, os_privmod_data_t *pd)
             }
             return res;
         }
-        mod = privload_next_module(mod);
     }
     return NULL;
 }

suite/tests/CMakeLists.txt

@@ -3647,6 +3647,19 @@ if (BUILD_SAMPLES)
         "CFLAGS=-m32;CXXFLAGS=-m32")
     endif ()
   endif ()
+  if (UNIX)
+    # XXX: Change this to go through torun() like the other tests to share
+    # output, multiplexing, _timeout, etc. features: not entirely
+    # straightforward because this test uses two clients.
+    # For now, "-s 90 -quiet -killpg" is added explicitly.
+    get_target_path_for_execution(drrun_path drrun "${location_suffix}")
+    get_client_path(client1 bbcount bbcount)
+    get_client_path(client2 opcodes opcodes)
+    get_target_path_for_execution(app_path "${ci_shared_app}" "${location_suffix}")
+    add_test(two_clients ${drrun_path} -s 90 -quiet -killpg
+      -client ${client1} 0 ''
+      -client ${client2} 1 '' ${dr_test_ops} -- ${app_path})
+  endif (UNIX)
 endif (BUILD_SAMPLES)
 
 if (BUILD_CLIENTS)