Removing longjmp error handling from the dynamic loader

[Carlos O'Donell]

On 3/11/19 11:29 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> In the current scheme, more localized error handling is problematic
>>> because it has high syntactic overhead: You need to define a struct for
>>> argument data and a separate function that receives the data, and pass
>>> both to _dl_catch_error.  There also could be a performance overhead if
>>> individual malloc calls were protected in this way because each call to
>>> _dl_catch_error incurs a call to setjmp.
>>
>> Yes, Method 1 requires passing down all information encapsulated in a
>> structure.
>>
>> Why would malloc calls be protected in this way?
> 
> You need a local handler if the pointer is not rooted in something that
> is thrown away by the top-most handler, and you call a
> potentially-throwing function between the allocation and the
> deallocation.

Right, you might be able to get away with a free-list here, but I get your
point, in a fully generic system you must register a free handler which is
specific code that knows how to free the object you allocated. I hate to say
it but we're just designing C++ from the ground up again. Why not just get
the loader to the point it can run C++? This is a serious question.

>>> I personally do not have a problem with exceptions and stack unwinding,
>>> but if this is what we want, we should use a DWARF-based unwinder and
>>> GCC's exception handling features (the limited support in the C front
>>> end is probably sufficient).
>>
>> I agree.
>>
>> Let's call this "Method 2: C exceptions"
>>
>> For dlopen-et-al I think a DWARF-based unwinder would be great.
> 
> It requires moving the unwinder implementation from libgcc_s to libc,
> though.  The last time we discussed this (related to unwinder
> performance issues and the dl_iterate_phdr interface), this idea was not
> well-received.

It might still be the best technical choice.

I do not think we should discard this idea so easily.

Why wouldn't we pursue this option?

> If we fix the init/fini and IFUNC resolver bugs the way I intend, we
> will never have to unwind through user code before hitting the final
> handler, so there is no problem with the availability of unwinding data.
> So at least that part is entirely solvable.

I don't understand the exact bugs in question here, what are they?

What are your solutions?

>> I assume you envision that exception handling in C would help cleanup
>> the code and allow more locally visible cleanups to happen (unwinding
>> state).
> 
> It requires a vastly different coding style.  I have not thought much
> about it.  I don't think we should rely on a relatively obscure GNU
> extension so prominently.  Or put differently, if we want to use RAII as
> an implementation technique, I don't think we should use C.

This is a GNU project and we should do whatever we think is the best
solution for glibc.

If the obscure GNU extension has bugs, then that's a reason not to use
it, or if the debugger doesn't support it, then that's a reason not to
use it, but it can't just be about "obscure GNU extensions."

Alternatives are using Rust or C++.

I would prefer a limited form of C++ without libstdc++ support.

Get the loader to the point where it can run C++, with the unwinder
builtin, and then only use C++ classes the loader itself defines.

>>> The alternative to unwinding is an explicit struct dl_exception *
>>> argument for functions which can fail, and use the return value to
>>> indicate whether there was a fatal error.  This sometimes causes issues
>>> where the return value is already used to signal both success and
>>> non-fatal error (e.g., -1 for failure from open_verify, or NULL for
>>> RTLD_NOLOAD failure from _dl_map_object_from_fd).
>>
>> No, if we're going to change it should be *towards* something where the
>> compiler can help us get it right.
>>
>> I don't want to see an explicit "this" passed into every function by hand.
>>
>> Let's call this "Method 3: Explicit this"
> 
> The explicit argument really helps to spot places where unwinding
> happens.  For example, it makes it pretty clear that you have a memory
> leak if you call a function with such an argument and an allocation is
> not rooted in a global data structure.

I agree, but is it the best option?

Or are you arguing it's the best value for cost option?

>>> There is some impact on <dl-machine.h> because the relocation processing
>>> needs to change.  We can convert the relocation processing first to the
>>> new scheme and continue to signal any errors using longjmp in the
>>> generic code.  But supporting twice the number of relocation APIs for
>>> incremental conversion of targets will still be difficult.  I think we
>>> are still looking at one fairly large patch, given the number of
>>> architectures we support, although the changes should just be a few
>>> dozen lines per architecture.
>>
>> Please expand on this a bit more.
> 
> Let's look at sysdeps/powerpc/powerpc64/dl-machine.h.  elf_machine_rela
> calls _dl_reloc_overflow and _dl_reloc_bad_type, among other things, and
> those throw.  This means that elf_machine_rela needs to be adjusted with
> the explicit argument.  RESOLVE_MAP calls _dl_lookup_symbol_x behind the
> scenes as well, so that can throw, too.

I see where you're going with this, particularly the state rollback.
 
> Is there any state we need to roll back locally in elf_machine_rela?
> Probably not.  So this is one area where we do not benefit from the
> explicit argument.

Understood.

>>> A third option is not use an explicit struct dl_exception * argument,
>>> but a per-thread variable.  This will require changes to support TLS
>>> (presumably the initial-exec variant) in the dynamic linker itself,
>>> which is currently missing.  Since the exception pointer is only needed
>>> in case of an error, using a TLS variable for it will avoid the overhead
>>> of maintaining the explicit exception pointer argument and passing it
>>> around.  Adding TLS support to the dynamic linker could be implemented
>>> incrementally across architectures, but the conversion itself faces the
>>> same flag day challenge as the explicit argument solution.  The explict
>>> argument also ensures that places stick out where encoding fatal errors
>>> in the return argument is difficult.  (A fourth option would compile the
>>> dynamic linker twice and use the TLS-less version for the initial
>>> loading of the executables and its dependencies.)
>>
>> I don't like either of these options.
>>
>> Let's call this "Method 3: TP explicit this"
>>
>> I won't give the 4th option a name :-}
> 
> Note that the current implementation already stores the dl_exception *
> and the jump buffer for the error handler in thread-local storage.

Yes, I see that in elf/dl-error-skeleton.c (catch_hook).

> That's why we need to compile the exception handling machinery twice.

So to be clear, you're proposing:

"Method 3: Explicit this"

Which you argue has highest value, with lowest cost.

While I've been arguing for "Method 2: C exceptions" which you believe
has too much cost given the value, since we'd have to:
- move the unwinder to libc
- switch to some C++ subset

I can understand your position, and I'm not opposed to
"Method 3: Explicit this", it would call out the exact places where
we might have problems for a future conversion.

-- 
Cheers,
Carlos.