Re: A collection of LD_AUDIT bugs that are important for tools (with better formatting for this list)

[John Mellor-Crummey <johnmc@rice.edu>]

> On Jun 22, 2021, at 3:15 AM, Florian Weimer <fweimer@redhat.com> wrote:
> 
> * John Mellor-Crummey:
> 
>> On Jun 17, 2021, at 3:09 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> 
>>> The issue is that the la_symbind interface is not very good at
>>> communicating that PLT enter/exit hooks aren't available under these
>>> circumstances.  
>> 
>> This is a separate issue from the one we reported. The issue we reported
>> was that la_symbind wasn’t called and LD_BIND_NOW was not used.
> 
> It's kind of related.  Our own example implementation looks like this:
> 
> uintptr_t
> la_symbind (Elf_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> 	    uintptr_t *defcook, unsigned int *flags, const char *symname)
> {
>  if (!do_exit)
>    *flags = LA_SYMB_NOPLTEXIT;
> 
>  return sym->st_value;
> }
> 
> Let's assume that we start calling la_symbind in places where there is
> no support for enter/exit hooks.  We could initialize *flags with
> LA_SYMB_NOPLTENTER | LA_SYMB_NOPLTEXIT, but the code above would clear
> the LA_SYMB_NOPLTENTER flag in !do_exit mode.
> 
> I want to increase LAV_CURRENT to 2 and call la_symbind in the BIND_NOW
> cases only if la_version returned a value greater than 1.  This way, old
> audit modules (which are supposed to return LAV_CURRENT from <link.h> in
> la_version) will continue to work because they do not see any unexpected
> la_symbind calls.
> 
> Once we call la_symbind in contexts where no enter/exit hooks are
> available, we should initialize the flags to LA_SYMB_NOPLTENTER |
> LA_SYMB_NOPLTEXIT (so that la_symbind can detect the situation), and
> report a dlopen/loader error if those flags are cleared by la_symbind.
> (With our example code, this would call pretty much all binding to fail,
> which is why I think we need the LAV_CURRENT change.)

Your suggested change would work for us. I would be happy with receiving the 
la_symbind calls even in the absence of an ability to get PLT enter/exit calls as
we don’t want them anyway for our tool.

To be explicit, since we intend to use la_symbind to provide a wrapped functions rather
than the original in some cases, we want la_symbind callbacks even if BIND_NOW
eager bindings are performed.

> 
>>> pthread_create interception becomes more difficult in glibc 2.34 because
>>> the pthread_create symbol is no longer interposable.
>> 
>> I don’t understand why pthread_create will no longer be interposable in 2.34.
>> We have a set of other functions that we also need to intercept, shown below:
>> 
>> _Exit
>> _exit
>> execl
>> execle
>> execlp
>> execv
>> execve
>> execvp
>> exit
>> fork
>> pthread_create
>> pthread_exit
>> pthread_sigmask
>> sigaction
>> signal
>> sigprocmask
>> sigtimedwait
>> sigwait
>> sigwaitinfo
>> system
>> vfork
>> 
>> If by "pthread_create symbol is no longer interposable", that means we
>> can’t insert a wrapper, then that is very bad for performance tools.
> 
> Once we merge librt and libanl into libc (patches for that have been
> posted), mq_notify, the timer functions, and getaddrinfo_a will call
> pthread_create using a direct call that cannot be intercepted in this
> way.  There is precedent for making things interposable/interceptable
> in the form of malloc
> 
>  <https://www.gnu.org/software/libc/manual/html_node/Replacing-malloc.html>
> 
> but we are currently do not plan to do this for pthread_create.  It
> would not be an ABI change as such, so we could introduce the indirect
> call as a later change based on user feedback.
> 
> You can already see this non-interceptable thread creation behavior
> today (in glibc 2.33 and earlier) with thrd_create, which does not
> result in a pthread_create call, either, despite creating a new thread
> as if by pthread_create.

Having a non-interposable thrd_create is a problem for us too, though 
we haven’t yet seen it in practice in HPC applications (or maybe it happened
and we were just unaware!).

> It's also the reason why your list contains the exec* functions and
> system in addition to fork, vfork, and execve, even though system is
> implemented on top of those functions: the internal direct calls are
> invisible to auditors.  But posix_spawn, posix_spawnp, popen are
> missing, too, so you will not trace all created processes.

Thanks for the advice. We will need to look at posix_spawn, 
posix_spawn_p, and popen.

> Starting with glibc 2.32, thread signal masks can also be manipulated
> using pthread_attr_setsigmask_np, and that might go unnoticed with your
> present sets of intercepts (although the mask change would be visible
> from a thread start routine wrapper injected via pthread_create).

We inspect and manipulate other thread attributes prior to thread creation.
We’ll add signal masks to the list. Thanks for the warning. 

> 
> Going back to trheading, I find it a bit curious that you intercept
> pthread_create, but not pthread_join.  How do you detect thread exit?  I
> assume you are interested in that event, too.  Merely wrapping the
> thread start routine is insufficient because there are other ways for a
> thread to exit besides returning from the start routine and calling
> pthread_exit (e.g., thread cancellation and unwinding).

We use pthread_cleanup_push to add a routine that will be called when a thread
exits.

> 
>> Should we expect any problems for the other functions listed above in
>> addition to pthread_create?
> 
> I don't think glibc 2.34 will bring any new problems in this area, but
> there are some pre-existing issues around posix_spawn, popen,
> pthread_attr_setsigmask_np.
> 
> Thanks,
> Florian
>