| Message ID | 20180516171124.24962-6-vivek@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Proof-of-Concept implementation of RTLD_SHARED for dlmopen | expand |
On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote: > From: Vivek Das Mohapatra <vivek@collabora.co.uk> > > When cleaning up before exit we should not call destructors or > otherwise free [most of] the contents of a cloned link_map entry > since they share [most of] their contents with the LM_ID_BASE > object from which they were cloned. s/clones/proxies/g > Instead we do the minimal cleanup necessary and remove them from > the namespace linked list(s) before the main cleanup code path > is triggered: This prevemts double-frees and double-invocation > of any destructors (which might not be idempotent). > --- > elf/dl-fini.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/elf/dl-fini.c b/elf/dl-fini.c > index 3cfc262400..643a68504e 100644 > --- a/elf/dl-fini.c > +++ b/elf/dl-fini.c > @@ -24,6 +24,50 @@ > /* Type of the constructor functions. */ > typedef void (*fini_t) (void); > > +/* Remove (and free) cloned entries from the namespace specifid by `ns'. */ > +void > +_dl_forget_clones (Lmid_t ns) > +{ > +#ifdef SHARED /* Obviously none of this applies if */ Move the #ifdef up to the caller please. > + struct link_map *clone; > + struct link_map *next; > + > + if (ns < 0 || ns >= DL_NNS) > + return; Make this an assert. It is an internal error to have an invalid ns at this point. We should not return and ignore this. > + > + for (clone = GL(dl_ns)[ns]._ns_loaded; clone != NULL; clone = next) > + { > + next = clone->l_next; > + > + /* Not actually clone, don't care. */ > + if (!clone->l_clone) > + continue; > + > + if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS)) > + _dl_debug_printf ("\nclone removed %s [%lu]\n", clone->l_name, ns); > + > + /* If item is a clone, slice it out of the linked list. */ > + if (clone == GL(dl_ns)[ns]._ns_loaded) > + GL(dl_ns)[ns]._ns_loaded = clone->l_next; > + else > + clone->l_prev->l_next = clone->l_next; > + > + /* Remember to fix up the links in both directions. */ > + if (clone->l_next) > + clone->l_next->l_prev = clone->l_prev; > + > + /* Don't need to do most destructor handling for clones. */ > + if (clone->l_free_initfini) > + free (clone->l_initfini); > + > + /* Do need to fix the global load count which is updated in > + _dl_add_to_namespace_list. */ > + GL(dl_ns)[ns]._ns_nloaded--; > + > + free (clone); > + } > +#endif > +} OK. > void > _dl_fini (void) > @@ -52,6 +96,12 @@ _dl_fini (void) > /* Protect against concurrent loads and unloads. */ > __rtld_lock_lock_recursive (GL(dl_load_lock)); > > + /* We need to remove any pointers to cloned entries (link_map > + structs that are copied from one namespace to another to > + implement RTLD_SHARED semantics) before the regular cleanup > + code gets to them. */ > + _dl_forget_clones (ns); While this is easy to implement like this, I think we are going to run into use cases where this doesn't work. Consider this test case: - Only a fini in a library. - Lazy binding. - fini calls a libc.so.6 function for the first time. - When fini is called we jump into ld.so to find the symbol to call, but the libc.so.6 proxy has been removed, and so we can no longer resolve calls to it. You will, IMO, have to remove the proxies at the point the library would have been unloaded normally from the namespace, but avoid doing all the other work. > + > unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded; > /* No need to do anything for empty namespaces or those used for > auditing DSOs. */ > In summary I think we need a v2 of this patch which implements a more complex free-at-unload support.
>> --- a/elf/dl-fini.c >> +++ b/elf/dl-fini.c >> @@ -24,6 +24,50 @@ >> /* Type of the constructor functions. */ >> typedef void (*fini_t) (void); >> >> +/* Remove (and free) cloned entries from the namespace specifid by `ns'. */ >> +void >> +_dl_forget_clones (Lmid_t ns) >> +{ >> +#ifdef SHARED /* Obviously none of this applies if */ > > Move the #ifdef up to the caller please. I think it was there originally but when I was building the full suite (including the static variant) gcc got upset at me about the bounds checking of GL(dl_ns)[…]. I'll revisit it and see if I can figure out what the problem was. >> + /* We need to remove any pointers to cloned entries (link_map >> + structs that are copied from one namespace to another to >> + implement RTLD_SHARED semantics) before the regular cleanup >> + code gets to them. */ >> + _dl_forget_clones (ns); > > While this is easy to implement like this, I think we are going to run > into use cases where this doesn't work. > > Consider this test case: > > - Only a fini in a library. > - Lazy binding. > - fini calls a libc.so.6 function for the first time. > - When fini is called we jump into ld.so to find the symbol to call, > but the libc.so.6 proxy has been removed, and so we can no longer > resolve calls to it. > You will, IMO, have to remove the proxies at the point the library > would have been unloaded normally from the namespace, but avoid doing > all the other work. Hm. Ok, I'll get on that. >> + >> unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded; >> /* No need to do anything for empty namespaces or those used for >> auditing DSOs. */ >> > > In summary I think we need a v2 of this patch which implements a more complex > free-at-unload support. Right. I will work through this and the other changes, hopefully a new patchset should land next week.
diff --git a/elf/dl-fini.c b/elf/dl-fini.c index 3cfc262400..643a68504e 100644 --- a/elf/dl-fini.c +++ b/elf/dl-fini.c @@ -24,6 +24,50 @@ /* Type of the constructor functions. */ typedef void (*fini_t) (void); +/* Remove (and free) cloned entries from the namespace specifid by `ns'. */ +void +_dl_forget_clones (Lmid_t ns) +{ +#ifdef SHARED /* Obviously none of this applies if */ + struct link_map *clone; + struct link_map *next; + + if (ns < 0 || ns >= DL_NNS) + return; + + for (clone = GL(dl_ns)[ns]._ns_loaded; clone != NULL; clone = next) + { + next = clone->l_next; + + /* Not actually clone, don't care. */ + if (!clone->l_clone) + continue; + + if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS)) + _dl_debug_printf ("\nclone removed %s [%lu]\n", clone->l_name, ns); + + /* If item is a clone, slice it out of the linked list. */ + if (clone == GL(dl_ns)[ns]._ns_loaded) + GL(dl_ns)[ns]._ns_loaded = clone->l_next; + else + clone->l_prev->l_next = clone->l_next; + + /* Remember to fix up the links in both directions. */ + if (clone->l_next) + clone->l_next->l_prev = clone->l_prev; + + /* Don't need to do most destructor handling for clones. */ + if (clone->l_free_initfini) + free (clone->l_initfini); + + /* Do need to fix the global load count which is updated in + _dl_add_to_namespace_list. */ + GL(dl_ns)[ns]._ns_nloaded--; + + free (clone); + } +#endif +} void _dl_fini (void) @@ -52,6 +96,12 @@ _dl_fini (void) /* Protect against concurrent loads and unloads. */ __rtld_lock_lock_recursive (GL(dl_load_lock)); + /* We need to remove any pointers to cloned entries (link_map + structs that are copied from one namespace to another to + implement RTLD_SHARED semantics) before the regular cleanup + code gets to them. */ + _dl_forget_clones (ns); + unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded; /* No need to do anything for empty namespaces or those used for auditing DSOs. */
From: Vivek Das Mohapatra <vivek@collabora.co.uk> When cleaning up before exit we should not call destructors or otherwise free [most of] the contents of a cloned link_map entry since they share [most of] their contents with the LM_ID_BASE object from which they were cloned. Instead we do the minimal cleanup necessary and remove them from the namespace linked list(s) before the main cleanup code path is triggered: This prevemts double-frees and double-invocation of any destructors (which might not be idempotent). --- elf/dl-fini.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)