This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[review v3] Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]
- From: "Carlos O'Donell (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Mon, 18 Nov 2019 11:23:57 -0500
- Subject: [review v3] Lazy binding failures during dlopen/dlclose must be fatal [BZ #24304]
- Auto-submitted: auto-generated
- References: <gerrit.1572549639000.I6b1addfe2e30f50a1781595f046f44173db9491a@gnutoolchain-gerrit.osci.io>
- Reply-to: gnutoolchain-gerrit at osci dot io
Carlos O'Donell has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/467
......................................................................
Patch Set 3: Code-Review+2
(16 comments)
This patch moves us in the right direction with respect to failure semantics. In the past, and I tested this with various small semantic tests, we used to attempt to back out of the failure in the dlopen case. The result of trying to back out of the dlopen failure has lead to several bugs where we have incomplete state rollback, and some of this is really really hard to get right once you start going past a certain point. What I like about this change is that we draw a line in the sand at the point just before we run foreign functions from the loaded object, so once the foreign functions (constructors) are running, we immediately decide the failures are going to be fatal. This is good because it actually matches what normally happens when you don't use dlopen, so the semantics are better aligned IMO. Terminating the process on lazy symbol resolution during the loading of the object is a change in semantics, but I think this is the correct way forward to some of the complex state roll
back problems we've seen. Once the foreign functions start running there is no way to rollback state correctly on failure. The rollback there is going to be at the process level e.g. restart the process and try again.
Please update the packaging changes here:
https://www.sourceware.org/glibc/wiki/Release/2.31
to indicate that the semantic change has happened and to watch out for it.
I think we should have a NEWS item for this, but you can commit that in a followup if you want.
OK for master.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
| --- elf/Makefile
| +++ elf/Makefile
| @@ -188,18 +188,18 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
| tst-nodelete) \
| tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
| tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
| tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
| tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
| tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
| tst-unwind-ctor tst-unwind-main tst-audit13 \
| tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \
| - tst-dlopen-self tst-auditmany
| + tst-dlopen-self tst-auditmany tst-initfinilazyfail
PS3, Line 196:
OK. New test tst-initfinilazyfail. Good.
| # reldep9
| tests-internal += loadtest unload unload2 circleload1 \
| neededtest neededtest2 neededtest3 neededtest4 \
| tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \
| tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \
| tst-create_format1
| tests-container += tst-pldd tst-dlopen-tlsmodid-container \
| tst-dlopen-self-container
| test-srcs = tst-pathopt
...
| @@ -280,17 +280,18 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
| tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
| tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
| tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
| tst-audit13mod1 tst-sonamemove-linkmod1 \
| tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
| tst-auditmanymod1 tst-auditmanymod2 tst-auditmanymod3 \
| tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
| - tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9
| + tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
| + tst-initlazyfailmod tst-finilazyfailmod
PS3, Line 288:
OK. Two new dsos for the test.
| # Most modules build with _ISOMAC defined, but those filtered out
| # depend on internal headers.
| modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
| $(modules-names))
|
| ifeq (yes,$(have-mtls-dialect-gnu2))
| tests += tst-gnu2-tls1
| modules-names += tst-gnu2-tls1mod
| $(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so
...
| @@ -1575,8 +1576,18 @@ # Avoid creating an ABI tag note, which may come before the
| # artificial, large note in tst-big-note-lib.o and invalidate the
| # test.
| $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
| $(LINK.o) -shared -o $@ $(LDFLAGS.so) $<
|
| $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
|
| CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
| +
| +$(objpfx)tst-initfinilazyfail: $(libdl)
| +$(objpfx)tst-initfinilazyfail.out: \
| + $(objpfx)tst-initlazyfailmod.so $(objpfx)tst-finilazyfailmod.so
| +# Override -z defs, so that we can reference an undefined symbol.
| +# Force lazy binding for the same reason.
| +LDFLAGS-tst-initlazyfailmod.so = \
| + -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
| +LDFLAGS-tst-finilazyfailmod.so = \
| + -Wl,-z,lazy -Wl,--unresolved-symbols=ignore-all
PS3, Line 1593:
OK. Add dso deps and force lazy.
| --- elf/dl-close.c
| +++ elf/dl-close.c
| @@ -100,17 +100,41 @@ remove_slotinfo (size_t idx, struct dtv_slotinfo_list *listp, size_t disp,
| GL(dl_tls_max_dtv_idx) = idx;
| return true;
| }
| }
|
| /* No non-entry in this list element. */
| return false;
| }
|
| +/* Invoke dstructors for CLOSURE (a struct link_map *). Called with
| + exception handling temporarily disabled, to make errors fatal. */
PS3, Line 110:
OK. Good comment.
| +static void
| +call_destructors (void *closure)
| +{
| + struct link_map *map = closure;
| +
| + if (map->l_info[DT_FINI_ARRAY] != NULL)
| + {
| + ElfW(Addr) *array =
| + (ElfW(Addr) *) (map->l_addr
| + + map->l_info[DT_FINI_ARRAY]->d_un.d_ptr);
| + unsigned int sz = (map->l_info[DT_FINI_ARRAYSZ]->d_un.d_val
| + / sizeof (ElfW(Addr)));
| +
| + while (sz-- > 0)
| + ((fini_t) array[sz]) ();
PS3, Line 125:
OK. Run the destructors from DT_FINI_ARRAY.
| + }
| +
| + /* Next try the old-style destructor. */
| + if (map->l_info[DT_FINI] != NULL)
| + DL_CALL_DT_FINI (map, ((void *) map->l_addr
| + + map->l_info[DT_FINI]->d_un.d_ptr));
PS3, Line 131:
OK. Run the other set of desctrutors, old-style, from DT_FINI.
| +}
|
| void
| _dl_close_worker (struct link_map *map, bool force)
| {
| /* One less direct use. */
| --map->l_direct_opencount;
|
| /* If _dl_close is called recursively (some destructor call dlclose),
...
| @@ -260,20 +284,20 @@ #endif
|
| /* All elements must be in the same namespace. */
| assert (imap->l_ns == nsid);
|
| if (!used[i])
| {
| assert (imap->l_type == lt_loaded
| && (imap->l_flags_1 & DF_1_NODELETE) == 0);
|
| /* Call its termination function. Do not do it for
| - half-cooked objects. */
| + half-cooked objects. Temporarily disable exception
| + handling, so that errors are fatal. */
PS3, Line 295:
OK. The point here is that a failure in a destructor is past the point
of no return for unloading the objects. If we fail to unload the
objects then the entire state of the dynamic loader is in question.
There is a balance here, either this was a programming error on the
part of the user, or it was a real dynamic loader bug. The problem is
that we want the loader to be as robust as possible in these cases,
and handle all forms of partial state is actually really hard and can
lead to subtle bugs, some of which may be security bugs. I'm for the
case where we have better consistency in the loader for handling these
issues. That is to say that a failure in running destructors is fatal,
and while this is a subtle change in the failure semantics, I think
this is a good thing. We immediately fail at the first point of
failure and make this easier to diagnose. This is in line with our
usual principle of fail early and catastrophically.
| if (imap->l_init_called)
| {
| /* When debugging print a message first. */
| if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,
| 0))
| _dl_debug_printf ("\ncalling fini: %s [%lu]\n\n",
| imap->l_name, nsid);
|
| - if (imap->l_info[DT_FINI_ARRAY] != NULL)
...
| @@ -286,18 +304,12 @@ #endif
| -
| - while (sz-- > 0)
| - ((fini_t) array[sz]) ();
| - }
| -
| - /* Next try the old-style destructor. */
| - if (imap->l_info[DT_FINI] != NULL)
| - DL_CALL_DT_FINI (imap, ((void *) imap->l_addr
| - + imap->l_info[DT_FINI]->d_un.d_ptr));
| + if (imap->l_info[DT_FINI_ARRAY] != NULL
| + || imap->l_info[DT_FINI] != NULL)
| + _dl_catch_exception (NULL, call_destructors, imap);
PS3, Line 306:
OK. Nice refactoring into _dl_catch_exception with a NULL so all
failures are fatal.
| }
|
| #ifdef SHARED
| /* Auditing checkpoint: we remove an object. */
| if (__glibc_unlikely (do_audit))
| {
| struct audit_ifaces *afct = GLRO(dl_audit);
| for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
| {
| --- elf/dl-open.c
| +++ elf/dl-open.c
| @@ -217,8 +218,24 @@ /* struct dl_init_args and call_dl_init are used to call _dl_init with
| + exception handling disabled. */
| +struct dl_init_args
| +{
| + struct link_map *new;
| + int argc;
| + char **argv;
| + char **env;
| +};
| +
| +static void
| +call_dl_init (void *closure)
| +{
| + struct dl_init_args *args = closure;
| + _dl_init (args->new, args->argc, args->argv, args->env);
| +}
PS3, Line 232:
OK. This is the same noexcept setup but for initializers.`
| +
| static void
| dl_open_worker (void *a)
| {
| struct dl_open_args *args = a;
| const char *file = args->file;
| int mode = args->mode;
| struct link_map *call_map = NULL;
|
...
| @@ -563,9 +580,20 @@ #endif
| + fatal. */
| + {
| + struct dl_init_args init_args =
| + {
| + .new = new,
| + .argc = args->argc,
| + .argv = args->argv,
| + .env = args->env
| + };
| + _dl_catch_exception (NULL, call_dl_init, &init_args);
| + }
PS3, Line 590:
OK. Use NULL to run with noexcept so exceptions thrown are fatal.
|
| /* Now we can make the new map available in the global scope. */
| if (mode & RTLD_GLOBAL)
| add_to_global_update (new);
|
| #ifndef SHARED
| /* We must be the static _dl_open in libc.a. A static program that
| has loaded a dynamic object now has competition. */
| __libc_multiple_libcs = 1;
| --- /dev/null
| +++ elf/tst-finilazyfailmod.c
| @@ -1,0 +17,11 @@ /* Helper module for tst-initfinilazyfail: lazy binding failure in destructor.
| + <https://www.gnu.org/licenses/>. */
| +
| +/* An undefined function. Calling it will cause a lazy binding
| + failure. */
| +void undefined_function (void);
| +
| +static void __attribute__ ((destructor))
| +fini (void)
| +{
| + undefined_function ();
| +}
PS3, Line 27:
OK. Clear comment. Calling an undefined function lazily which will
result in an error.
| --- /dev/null
| +++ elf/tst-initfinilazyfail.c
| @@ -1,0 +19,60 @@ /* Test that lazy binding failures in constructors and destructors are fatal.
| +#include <dlfcn.h>
| +#include <string.h>
| +#include <support/capture_subprocess.h>
| +#include <support/check.h>
| +#include <support/xdlfcn.h>
| +
| +static void
| +test_constructor (void *closure)
| +{
| + void *handle = dlopen ("tst-initlazyfailmod.so", RTLD_LAZY);
PS3, Line 28:
OK. Previously this would cause a fatal error to be thrown in the
constructor because it had a lazy binding failure, and that would in
turn cause the dlopen to fail and return NULL, but leave the half-
constructed link map lying around. Now with your semantics changes
this is fully fatal at it really should be, similar to the situation
you get if you do not use dlopen. I think this is the right behaviour
because we're past the point of no return for cleaning up the mess
that is left because of the failure. Worst case we will need a new
flag for this if it turns out someone really really needs the ability
to load a failing DSO and leave it in a failed state (though that's
fairly dangerous).
| + if (handle == NULL)
| + FAIL_EXIT (2, "dlopen did not terminate the process: %s", dlerror ());
| + else
| + FAIL_EXIT (2, "dlopen did not terminate the process (%p)", handle);
| +}
| +
| +static void
| +test_destructor (void *closure)
| +{
| + void *handle = xdlopen ("tst-finilazyfailmod.so", RTLD_LAZY);
PS3, Line 38:
OK. Same test but for the desctructors.
| + int ret = dlclose (handle);
| + const char *message = dlerror ();
| + if (message != NULL)
| + FAIL_EXIT (2, "dlclose did not terminate the process: %d, %s",
| + ret, message);
| + else
| + FAIL_EXIT (2, "dlopen did not terminate the process: %d", ret);
| +}
| +
| +static int
| +do_test (void)
| +{
| + {
| + struct support_capture_subprocess proc
PS3, Line 52:
OK. Run in a subprocess for both constructor and destructor tests
because they are going to die.
| + = support_capture_subprocess (test_constructor, NULL);
| + support_capture_subprocess_check (&proc, "constructor", 127,
| + sc_allow_stderr);
| + printf ("info: constructor failure output: [[%s]]\n", proc.err.buffer);
| + TEST_VERIFY (strstr (proc.err.buffer,
| + "tst-initfinilazyfail: symbol lookup error: ")
| + != NULL);
| + TEST_VERIFY (strstr (proc.err.buffer,
| + "tst-initlazyfailmod.so: undefined symbol:"
| + " undefined_function\n") != NULL);
| + support_capture_subprocess_free (&proc);
| + }
| +
| + {
| + struct support_capture_subprocess proc
| + = support_capture_subprocess (test_destructor, NULL);
| + support_capture_subprocess_check (&proc, "destructor", 127,
PS3, Line 69:
OK. Same thing but with destructor test.
| + sc_allow_stderr);
| + printf ("info: destructor failure output: [[%s]]\n", proc.err.buffer);
| + TEST_VERIFY (strstr (proc.err.buffer,
| + "tst-initfinilazyfail: symbol lookup error: ")
| + != NULL);
| + TEST_VERIFY (strstr (proc.err.buffer,
| + "tst-finilazyfailmod.so: undefined symbol:"
| + " undefined_function\n") != NULL);
| + support_capture_subprocess_free (&proc);
| --- /dev/null
| +++ elf/tst-initlazyfailmod.c
| @@ -1,0 +17,11 @@ /* Helper module for tst-initfinilazyfail: lazy binding failure in constructor.
| + <https://www.gnu.org/licenses/>. */
| +
| +/* An undefined function. Calling it will cause a lazy binding
| + failure. */
| +void undefined_function (void);
| +
| +static void __attribute__ ((constructor))
| +init (void)
| +{
| + undefined_function ();
PS3, Line 26:
OK. Same DSO but for constructor failure.
| +}
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I6b1addfe2e30f50a1781595f046f44173db9491a
Gerrit-Change-Number: 467
Gerrit-PatchSet: 3
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Mon, 18 Nov 2019 16:23:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment