[review] dlsym: Do not determine caller link map if not needed

["Carlos O'Donell (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/528
......................................................................


Patch Set 1: Code-Review+2

(6 comments)

Optimization looks correct and covers RTLD_GLOBAL, RTLD_NEXT and audit cases.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +4,18 @@ AuthorDate: 2019-11-08 15:48:51 +0100
| +Commit:     Florian Weimer <fweimer@redhat.com>
| +CommitDate: 2019-11-08 15:48:51 +0100
| +
| +dlsym: Do not determine caller link map if not needed
| +
| +Obtaining the link map is potentially very slow because it requires
| +iterating over all loaded objects in the current implementation.  If
| +the caller supplied an explicit handle (i.e., not one of the RTLD_*
| +constants), the dlsym implementation does not need the identity of the
| +caller (except in the special cause of auditing), so this change

PS1, Line 13:

s/cause/case/g

| +avoids computing it in that case.
| +
| +Even in the minimal case (dlsym called from a main program linked with
| +-dl), this shows a small speedup, perhaps around five percent.  The
| +performance improvement can be arbitrarily large in principle (if
| +_dl_find_dso_for_object has to iterate over many link maps).

PS1, Line 19:

OK, great reason to fix this. I agree completely.

| +
| +Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
| --- elf/dl-sym.c
| +++ elf/dl-sym.c
| @@ -74,33 +74,45 @@ };
|  static void
|  call_dl_lookup (void *ptr)
|  {
|    struct call_dl_lookup_args *args = (struct call_dl_lookup_args *) ptr;
|    args->map = GLRO(dl_lookup_symbol_x) (args->name, args->map, args->refp,
|  					args->map->l_scope, args->vers, 0,
|  					args->flags, NULL);
|  }
|  
| +/* Return the link map containing the caller address.  */
| +static inline struct link_map *
| +find_caller_link_map (ElfW(Addr) caller)
| +{
| +  struct link_map *l = _dl_find_dso_for_object (caller);
| +  if (l != NULL)
| +    return l;
| +  else
| +    /* If the address is not recognized the call comes from the main
| +       program (we hope).  */
| +    return GL(dl_ns)[LM_ID_BASE]._ns_loaded;
| +}

PS1, Line 94:

OK. Refactored.

|  
|  static void *
|  do_sym (void *handle, const char *name, void *who,
|  	struct r_found_version *vers, int flags)
|  {
|    const ElfW(Sym) *ref = NULL;
|    lookup_t result;
|    ElfW(Addr) caller = (ElfW(Addr)) who;
|  
| -  struct link_map *l = _dl_find_dso_for_object (caller);
| -  /* If the address is not recognized the call comes from the main
| -     program (we hope).  */
| -  struct link_map *match = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
| +  /* Link map of the caller if needed.  */
| +  struct link_map *match = NULL;
|  
|    if (handle == RTLD_DEFAULT)
|      {
| +      match = find_caller_link_map (caller);

PS1, Line 109:

OK. Use the new refactored function.

| +
|        /* Search the global scope.  We have the simple case where
|  	 we look up in the scope of an object which was part of
|  	 the initial binary.  And then the more complex part
|  	 where the object is dynamically loaded and the scope
|  	 array can change.  */
|        if (RTLD_SINGLE_THREAD_P)
|  	result = GLRO(dl_lookup_symbol_x) (name, match, &ref,
|  					   match->l_scope, vers, 0,

 ...

| @@ -122,17 +134,19 @@ do_sym (void *handle, const char *name, void *who,
|  	  THREAD_GSCOPE_RESET_FLAG ();
|  	  if (__glibc_unlikely (exception.errstring != NULL))
|  	    _dl_signal_exception (err, &exception, NULL);
|  
|  	  result = args.map;
|  	}
|      }
|    else if (handle == RTLD_NEXT)
|      {
| +      match = find_caller_link_map (caller);

PS1, Line 143:

OK. Likewise.

| +
|        if (__glibc_unlikely (match == GL(dl_ns)[LM_ID_BASE]._ns_loaded))
|  	{
|  	  if (match == NULL
|  	      || caller < match->l_map_start
|  	      || caller >= match->l_map_end)
|  	    _dl_signal_error (0, NULL, NULL, N_("\
|  RTLD_NEXT used in code not dynamically loaded"));
|  	}

 ...

| @@ -182,16 +196,19 @@ #ifdef SHARED
|  	{
|  	  const char *strtab = (const char *) D_PTR (result,
|  						     l_info[DT_STRTAB]);
|  	  /* Compute index of the symbol entry in the symbol table of
|  	     the DSO with the definition.  */
|  	  unsigned int ndx = (ref - (ElfW(Sym) *) D_PTR (result,
|  							 l_info[DT_SYMTAB]));
|  
| +	  if (match == NULL)
| +	    match = find_caller_link_map (caller);

PS1, Line 205:

OK, and again for auditing.

| +
|  	  if ((match->l_audit_any_plt | result->l_audit_any_plt) != 0)
|  	    {
|  	      unsigned int altvalue = 0;
|  	      struct audit_ifaces *afct = GLRO(dl_audit);
|  	      /* Synthesize a symbol record where the st_value field is
|  		 the result.  */
|  	      ElfW(Sym) sym = *ref;
|  	      sym.st_value = (ElfW(Addr)) value;

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ide5d9e2cc7ac25a0ffae8fb4c26def0c898efa29
Gerrit-Change-Number: 528
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Szabolcs Nagy <szabolcs.nagy@arm.com>
Gerrit-Comment-Date: Wed, 27 Nov 2019 18:47:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment