Re: Removing longjmp error handling from the dynamic loader

[Rich Felker <dalias at libc dot org>]

On Tue, Mar 05, 2019 at 05:37:09PM +0100, Florian Weimer wrote:
> Currently, dynamic loader operations triggered by explicit function
> calls (dlopen, dlsym, dlcose) wrapped in exception handlers, installed
> using _dl_catch_error.  This function calls setjmp, and when an error is
> raised the dynamic linker calls longjmp, resetting the call stack.
> 
> This leads to strange bugs, such as undefined symbols in init/fini calls
> being treated as non-fatal soft errors reported through dlerror
> (bug 24304).  It also leads to a design where most dlopen failures are
> not handled locally.  Instead they bubble up to the top-level dlopen
> error handler installed using _dl_catch_error, which then attempts to
> undo the changes of the  partially completed state to the global dynamic
> linker state (see bug 20839 for a couple of problems with that).
> 
> 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.
> 
> Does anyone think we should retain the current error handling scheme?
> 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).
> 
> 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).
> 
> 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.
> 
> We cannot convert the generic code first because that would mean calling
> setjmp each time before calling into architecture-specific code, which I
> expect will be too problematic from a performance point of view.
> 
> 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.)
> 
> From a source code and binary size point of view, there is not much
> difference between using longjmp and the explicit function argument
> approach on x86-64.
> 
> Does anyone want to keep the longjmp approach?  Should I polish my patch
> and extend it to cover all architectures?

I don't have a strong opinion (and maybe not enough information to
have any opinion) on whether you keep longjmp or go with something
else, but I don't think there's any fundamental reason you need to
change this to fix current bugs. In musl, longjmp is used partly by
historical accident (I don't recall fully, but I think it was a matter
of adding dlopen to code that was originally written just for initial
dynamic linking at program entry), and doesn't have problems like the
ones you describe.

For the init/fini "soft errors" problem, it sounds to me like the code
that runs the ctors should just be outside of the scope of the
_dl_catch_error. If you've started running ctors, you're past the
point where the operation can be backed out in any meaningful sense.
I wonder if it's worse with ifunc, but I think not -- without
bind_now, the ifunc resolvers don't even need to run before you pass
the point of no return, and with bind_now, you'd be executing them in
a context where resolver errors are still "hard" and can/should cause
dlopen to fail.

Are there things I'm missing?

Rich