This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] elf: Ignore LD_AUDIT interfaces if la_version returns 0 [BZ #24122]



On 04/02/2019 11:51, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> The actual bug fix is the separate error message for the case when
>>> la_version is zero.  The dynamic linker error message (which is NULL
>>> in this case) is no longer used.  Based on the intended use of version
>>> zero (ignore this module due to explicit request), the message is only
>>> printed if debugging is enabled.
>>>
>>> 2019-02-02  Florian Weimer  <fweimer@redhat.com>
>>>
>>> 	[BZ #24122]
>>> 	* elf/rtld.c (unload_audit_module): New function.
>>> 	(report_audit_module_load_error): Likewise.
>>> 	(load_audit_module): Likewise.  Extracted from dl_main.  Call
>>> 	_dl_close if the laversion symbol cannot be found.  Use early
>>> 	returns for error handling.  Check for a zero return value from
>>> 	la_version.  Remove spurious comment about static TLS
>>> 	initialization.  Remove (void) casts of function results.
>>> 	(load_audit_modules): New function.  Extracted from dl_main.
>>> 	(early_audit_modules_notification): Likewise.
>>> 	(dl_main): Call load_audit_modules and
>>> 	early_audit_modules_notification.
>>
>> I think we should reinstate the testcase from original commit 
>> 8e889c5da3c5981c5a46a93fec02de40131ac5a6 along with this change. It aligns
>> with our policy of fixing bug reports with a regression test.
> 
> Absolutely.  I already posted that as a separate patch.  I want to
> retain your authorship, and since it is a completely separate change, a
> separate commit seems reasonable to me.

I don't mind, just add me on CL and put reviewed-by: on commit message.

> 
>>> +/* Load one audit module.  */
>>> +static void
>>> +load_audit_module (const char *name, struct audit_ifaces **last_audit)
>>> +{
>>> +  int original_tls_idx = GL(dl_tls_max_dtv_idx);
>>> +
>>> +  struct dlmopen_args dlmargs;
>>> +  dlmargs.fname = name;
>>> +  dlmargs.map = NULL;
>>
>> Maybe a simple initialization as 'struct dlmopen_args dlmargs = { name, NULL }'?
> 
> It's what was there before. 8-/
> 
>>> +  const char *objname;
>>> +  const char *err_str = NULL;
>>> +  bool malloced;
>>> +  _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit, &dlmargs);
>>> +  if (__glibc_unlikely (err_str != NULL))
>>> +    {
>>> +      report_audit_module_load_error (name, err_str, malloced);
>>> +      return;
>>> +    }
>>> +
>>> +  struct lookup_args largs;
>>> +  largs.name = "la_version";
>>> +  largs.map = dlmargs.map;
>>> +  _dl_catch_error (&objname, &err_str, &malloced, lookup_doit, &largs);
>>> +  if (__glibc_likely (err_str != NULL))
>>> +    {
>>> +      unload_audit_module (dlmargs.map, original_tls_idx);
>>> +      report_audit_module_load_error (name, err_str, malloced);
>>> +      return;
>>> +    }
>>> +
>>> +  unsigned int (*laversion) (unsigned int) = largs.result;
>>> +  assert (laversion != NULL);
>>
>> I think we need to add a comment why we assert on a NULL 'laversion'
>> symbol, maybe by adding this means a corrupted audit object.
> 
> I can add:
> 
> /* A null symbol indicates that something is very wrong with the loaded
>    object because defined symbols are supposed to have a valid, non-null
>    address.  */

Ok.

> 
>>> +  unsigned int lav = laversion (LAV_CURRENT);
>>> +  if (lav == 0)
>>
>> Maybe __glibc_unlikely here?
> 
> We don't really know what's unlikely or not.  Since audit modules are
> used only rarely, I don't want to worry about it.

Ok.

> 
> 
>>> +  /* Allocate structure for the callback function pointers.  This call
>>> +     can never fail.  */
>>> +  enum { naudit_ifaces = 8 };
>>> +  union
>>> +  {
>>> +    struct audit_ifaces ifaces;
>>> +    void (*fptr[naudit_ifaces]) (void);
>>> +  } *newp = malloc (sizeof (*newp));
>>
>> If this call should never fail, should we also assert for the malloc ?
> 
> I think we use _dl_fatal_printf elsewhere.  Again, this is a
> pre-existing issue. 8-/

Ok, we can live with it.

> 
>>
>>> +
>>> +  /* Names of the auditing interfaces.  All in one
>>> +     long string.  */
>>> +  static const char audit_iface_names[] =
>>> +    "la_activity\0"
>>> +    "la_objsearch\0"
>>> +    "la_objopen\0"
>>> +    "la_preinit\0"
>>> +#if __ELF_NATIVE_CLASS == 32
>>> +    "la_symbind32\0"
>>> +#elif __ELF_NATIVE_CLASS == 64
>>> +    "la_symbind64\0"
>>> +#else
>>> +# error "__ELF_NATIVE_CLASS must be defined"
>>> +#endif
>>> +#define STRING(s) __STRING (s)
>>> +    "la_" STRING (ARCH_LA_PLTENTER) "\0"
>>> +    "la_" STRING (ARCH_LA_PLTEXIT) "\0"
>>> +    "la_objclose\0";
>>> +  unsigned int cnt = 0;
>>> +  const char *cp = audit_iface_names;
>>> +  do
>>> +    {
>>> +      largs.name = cp;
>>> +      (void) _dl_catch_error (&objname, &err_str, &malloced,
>>> +			      lookup_doit, &largs);
>>
>> Do we need the cast here?
> 
> I think we should switch to the new error handling framework
> (_dl_catch_exception) and not worry about it until then.

Alright, I think since you refactoring the code it would be good to
not propagate unnecessary code conventions like that. 

> 
>>> +
>>> +      /* Store the pointer.  */
>>> +      if (err_str == NULL && largs.result != NULL)
>>> +	{
>>> +	  newp->fptr[cnt] = largs.result;
>>> +
>>> +	  /* The dynamic linker link map is statically allocated,
>>> +	     initialize the data now.  */
>>> +	  GL(dl_rtld_map).l_audit[cnt].cookie
>>> +	    = (intptr_t) &GL(dl_rtld_map);
>>> +	}
>>> +      else
>>> +	newp->fptr[cnt] = NULL;
>>> +      ++cnt;
>>> +
>>> +      cp = (char *) rawmemchr (cp, '\0') + 1;
>>
>> Same as before.
> 
> Yeah, an unrelated, pre-existing issue.

Ok.

> 
>>> +/* If we have any auditing modules, announce that we already have two
>>> +   objects loaded: the main program and the dynamic linker itself.  */
>>> +static void
>>> +early_audit_modules_notification (struct link_map *main_map)
>>> +{
>>> +  if (__glibc_likely (GLRO(dl_naudit) == 0))
>>> +    return;
>>> +
>>> +  struct link_map *ls[2] = { main_map, &GL(dl_rtld_map) };
>>> +
>>> +  for (unsigned int outer = 0; outer < 2; ++outer)
>>
>> Maybe size_t here?
> 
> Despite outer < 2?
> 
> I can unroll the loop in the caller, like this:
> 
>   if (__glibc_likely (GLRO(dl_naudit) > 0)
>     {
>       early_audit_modules_notification (main_map);
>       early_audit_modules_notification (&GL(dl_rtld_map));
>     }
> 
> That actually looks quite reasonable.  What do you think?

Look better to me, thanks.

> 
> Thanks,
> Florian
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]