Mail Archives: djgpp/2015/10/05/05:32:54
On 10/4/15, Ozkan Sezer <sezeroz AT gmail DOT com> wrote:
> On 10/4/15, Ozkan Sezer <sezeroz AT gmail DOT com> wrote:
>> On 10/4/15, Ozkan Sezer <sezeroz AT gmail DOT com> wrote:
>>> On 10/4/15, Ozkan Sezer <sezeroz AT gmail DOT com> wrote:
>>>> On 10/4/15, Juan Manuel Guerrero (juan DOT guerrero AT gmx DOT de) [via
>>>> djgpp AT delorie DOT com] <djgpp AT delorie DOT com> wrote:
>>>>> Am 04.10.2015 12:39, schrieb Juan Manuel Guerrero
>>>>> (juan DOT guerrero AT gmx DOT de)
>>>>> [via djgpp AT delorie DOT com]:
>>>>>
>>>>> [snip]
>>>>>> Below is a patch that will implement ref-counting for implicitly
>>>>>> loaded
>>>>>> modules
>>>>>> by other ones. Please not that this ref-counting only works for
>>>>>> dlopen/dlclose.
>>>>>> There may be other loading mechanisms that suffer from the same issue
>>>>>> and
>>>>>> that
>>>>>> will not be fixed by this modification.
>>>>>> The patch is intended for the main branch but should work on v2_05_1
>>>>>> as
>>>>>> well.
>> [snip]
>>>
>>> Here are my concerns about the patch:
>>>
>>>
>>> 1. It is possible that we mis-record a dep handle: dlopen() always
>>> returns dxe_chain, i.e. the top of the open dxes list, unless it is
>>> an already open module. Consider this deps scenario:
>>>
>>> a.dxe -> b.dxe -> c.dxe -> d.dxe, and additionally a.dxe -> d.dxe
>>>
>>> This implementation will mis-record the d.dxe as b.dxe, because we are
>>> recording dxe_chain and not the handle itself returned by dlopen().
>>> Upon a dlclose() of a.dxe, d.dxe will remain open:
>>>
>>> dxe3gen -o a.dxe -P b.dxe -P d.dxe -U a.o
>>> dxe3gen -o b.dxe -P c.dxe -U b.o
>>> dxe3gen -o c.dxe -P d.dxe -U c.o
>>> dxe3gen -o d.dxe -U d.o
>>>
>>> $ cat 0.c
>>> #include <dlfcn.h>
>>> #include <stdio.h>
>>>
>>> void *my_dxe1;
>>> void *p;
>>>
>>> void test_dxes(void) {
>>> if ((p = dlsym(RTLD_DEFAULT,"_func_a"))!=NULL)
>>> printf("a.dxe still active\n");
>>> if ((p = dlsym(RTLD_DEFAULT,"_func_b"))!=NULL)
>>> printf("b.dxe still active\n");
>>> if ((p = dlsym(RTLD_DEFAULT,"_func_c"))!=NULL)
>>> printf("c.dxe still active\n");
>>> if ((p = dlsym(RTLD_DEFAULT,"_func_d"))!=NULL)
>>> printf("d.dxe still active\n");
>>> }
>>>
>>> int main (void) {
>>> my_dxe1 = dlopen("a.dxe",RTLD_GLOBAL);
>>> if (!my_dxe1) {
>>> printf("dlopen() failed\n");
>>> test_dxes();
>>> return 1;
>>> }
>>> dlclose(my_dxe1);
>>> test_dxes();
>>>
>>> return 0;
>>> }
>>>
>>> Running a.exe prints "d.dxe still active"
>>> The a.dxe portion of dxe_chain.txt looks like this:
>>> dxe_chain : 0x9fa70
>>> next : 0x9f908
>>> fname : C:\A.DXE
>>> mode : RTLD_GLOBAL
>>> inuse : 1
>>> n_exp_syms : 1
>>> exp_table : 0x9ef00
>>> exp_table[0]->offset : 0
>>> exp_table[0]->name : _func_a
>>> header : 0x9ee30
>>> section : 0x9ee40
>>> _fini : 48
>>> deps : 0x9fa40
>>> handle : 0x9f908
>>> handle->fname : C:\B.DXE
>>>
>>> deps : 0x9fa58
>>> handle : 0x9f908
>>> handle->fname : C:\B.DXE
>>>
>>>
>>> 2. Consider the dependency chain above again, this time assume that
>>> c.dxe or one or more of the deeper dependencies is missing: dlopen()
>>> does not do a cleanup for implicitly opened modules and will leak
>>> memory.
>>>
>>>
>>> 3. I am not sure about this part: in dlclose(), we are first closing
>>> the deps, and then ejecting our dxe from dxe_chain. However, recursive
>>> dlclose() calls will modify dxe_chain, so how safe is this? I would
>>> suggest that we first eject the handle from dxe_chain, and then do the
>>> cleanup for its dependencies.
>>>
>>
>> And another:
>>
>> 4. If malloc() fails for deps list, it goes to midwayerror label
>> instead of unrecoverable and leaves discardable not freed.
>>
>> Here is a quick update of the patch which seems to address all four
>> of the concerns I noted. (also attached as a file: dlopen3.patch)
>>
>> Certainly need more of careful eyes to detect more possible issues.
>>
>
> Here is a better version: fixes a thinko in the previous one.
> (also attached: dlopen3a.patch)
>
> Index: src/libc/dxe/dlopen.c
> ===================================================================
> RCS file: /cvs/djgpp/djgpp/src/libc/dxe/dlopen.c,v
> retrieving revision 1.11
> diff -U 5 -p -r1.11 dlopen.c
> --- src/libc/dxe/dlopen.c 4 Oct 2015 10:27:28 -0000 1.11
> +++ src/libc/dxe/dlopen.c 4 Oct 2015 17:55:05 -0000
> @@ -36,14 +36,16 @@
>
> #ifndef ELOOP
> #define ELOOP EMLINK
> #endif
>
> +#define DEBUG_DXE3 0 /* Prints struct __dxe_handle members. Always
> commited as 0. */
> +
> /* private stack */
> -typedef struct stk_node {
> +typedef struct __stk_node {
> const char *name;
> - struct stk_node *next;
> + struct __stk_node *next;
> } stk_node;
>
> /* Exported symbols table entry */
> typedef struct
> {
> @@ -57,10 +59,16 @@ typedef struct
> unsigned short n_rel_relocs;
> unsigned short n_abs_relocs;
> char name [1]; /* expanded as needed */
> } __attribute__((packed)) unres_table_entry;
>
> +/* This is a linked list of implicitly loaded dxe modules. */
> +typedef struct __llist {
> + struct __dxe_handle *handle; /* last implicitly opened module */
> + struct __llist *next;
> +} dxe_list;
> +
> /* This is the private dxe_h structure */
> typedef struct __dxe_handle
> {
> struct __dxe_handle *next; /* Pointer to next module in chain */
> char fname[FILENAME_MAX]; /* Full module pathname */
> @@ -69,10 +77,11 @@ typedef struct __dxe_handle
> int n_exp_syms; /* Number of entries in export table */
> exp_table_entry **exp_table; /* Exported symbols table */
> char *header; /* The resident portion of header */
> char *section; /* code+data+bss section */
> long _fini; /* finalization */
> + dxe_list *deps; /* Linked list of implicitly open module by this
> module or NULL */
> } dxe_handle, *dxe_h;
>
> /* Last-resort symbol resolver */
> void *(*_dlsymresolver) (const char *symname) = NULL;
> /* Last-error unresolved symbol count */
> @@ -110,10 +119,12 @@ void *dlopen(const char *filename, int m
> char *discardable;
>
> stk_node *node;
> static stk_node *stk_top = NULL;
>
> + dxe_list *deps;
> +
> #ifndef __GNUC__
> static int cleanup = 0;
> #endif
>
> _dl_unresolved_count = 0;
> @@ -205,10 +216,11 @@ found:
> /* O.k, fill the module handle structure */
> strcpy(dxe.fname, realfn);
> dxe.inuse = 1;
> dxe.mode = mode;
> dxe.n_exp_syms = dxehdr.n_exp_syms;
> + dxe.deps = NULL;
>
> /* Read DXE tables and the data section */
> hdrsize = dxehdr.symbol_offset - sizeof(dxehdr);
> discardsize = dxehdr.dep_size + dxehdr.unres_size + dxehdr.nrelocs
> * sizeof(long);
> if ((dxe.header = malloc(hdrsize + dxehdr.sec_size)) == NULL)
> @@ -240,26 +252,50 @@ found:
> /* Fill the unfilled portion of code+data+bss segment with zeros */
> memset(dxe.section + dxehdr.sec_f_size, 0, dxehdr.sec_size -
> dxehdr.sec_f_size);
>
> /* Load the dependencies */
> scan = discardable;
> - for (i = 0; i < dxehdr.n_deps; i++)
> + for (deps = NULL, i = 0; i < dxehdr.n_deps; i++)
> {
> stk_node tmp;
> + dxe_h dep_h;
> +
> tmp.name = realfn;
> tmp.next = stk_top;
> stk_top = &tmp;
>
> - if (dlopen(scan, RTLD_GLOBAL) == NULL)
> + if ((dep_h = dlopen(scan, RTLD_GLOBAL)) == NULL)
> {
> stk_top = tmp.next;
> goto unrecoverable;
> }
> + else
> + {
> + dxe_list *next;
> +
> + stk_top = tmp.next;
>
> - stk_top = tmp.next;
> + scan = strchr(scan, 0) + 1;
>
> - scan = strchr(scan, 0) + 1;
> + /* Register all implicitly open modules by this one. */
> + if ((next = malloc(sizeof(dxe_list))) == NULL)
> + {
> + dlclose(dep_h);
> + errno = ENOMEM;
> + goto unrecoverable;
> + }
> + next->handle = dep_h;
> + next->next = NULL;
> +
> + if (deps)
> + {
> + deps->next = next;
> + deps = deps->next;
> + }
> + else
> + dxe.deps = deps = next;
> + }
> }
>
> /* Allright, now we're ready to resolve all unresolved symbols */
> _dl_unresolved_count = dxehdr.n_unres_syms;
> _dl_unresolved_symbol[0] = 0;
> @@ -325,10 +361,50 @@ found:
> errno = ENOMEM;
> goto midwayerror;
> }
> memcpy(dxe_chain, &dxe, sizeof(dxe_handle));
>
> +#if (DEBUG_DXE3 -0) == 1
> + {
> + FILE *f = fopen("c:/tmp/dxe_chain.txt", "a");
> +
> + if (f)
> + {
> + fprintf(f, "dxe_chain : 0x%p\n"
> + " next : 0x%p\n"
> + " fname : %s\n"
> + " mode : %s\n"
> + " inuse : %d\n"
> + " n_exp_syms : %d\n"
> + " exp_table : 0x%p\n",
> + dxe_chain, dxe_chain->next, dxe_chain->fname,
> + dxe_chain->mode == RTLD_LAZY ? "RTLD_LAZY" :
> + dxe_chain->mode == RTLD_NOW ? "RTLD_NOW" :
> + dxe_chain->mode == RTLD_LOCAL ? "RTLD_LOCAL" :
> + dxe_chain->mode == RTLD_GLOBAL ? "RTLD_GLOBAL" :
> "unknown",
> + dxe_chain->inuse, dxe_chain->n_exp_syms,
> dxe_chain->exp_table);
> + for (i = 0; i < dxe_chain->n_exp_syms; i++)
> + fprintf(f, " exp_table[%d]->offset : %ld\n"
> + " exp_table[%d]->name : %s\n",
> + i, dxe_chain->exp_table[i]->offset, i,
> dxe_chain->exp_table[i]->name);
> + fprintf(f, " header : 0x%p\n"
> + " section : 0x%p\n"
> + " _fini : %ld\n",
> + dxe_chain->header, dxe_chain->section, dxe_chain->_fini);
> + if ((deps = dxe_chain->deps))
> + for (; deps; deps = deps->next)
> + fprintf(f, " deps : 0x%p\n"
> + " handle : 0x%p\n"
> + " handle->fname : %s\n\n",
> + deps, deps->handle, deps->handle->fname);
> + else
> + fprintf(f, " deps : 0x00000000\n\n");
> + fclose(f);
> + }
> + }
> +#endif
> +
> #ifndef __GNUC__
> if (!cleanup)
> {
> cleanup = !0;
> atexit(_closeall);
> @@ -340,10 +416,20 @@ found:
> unrecoverable:
> free(discardable);
> midwayerror:
> free(dxe.header);
>
> + deps = dxe.deps;
> + while (deps)
> + {
> + dxe_list *next = deps->next;
> +
> + dlclose(deps->handle);
> + free(deps);
> + deps = next;
> + }
> +
> return NULL;
> }
>
> int dlclose(void *dxe)
> {
> @@ -366,10 +452,23 @@ int dlclose(void *dxe)
> *cur = ((dxe_h)dxe)->next;
> break;
> }
> }
>
> + /* remove all implicitly loaded modules by this module. */
> + {
> + dxe_list *deps = ((dxe_h)dxe)->deps;
> + while (deps)
> + {
> + dxe_list *next = deps->next;
> +
> + dlclose(deps->handle);
> + free(deps);
> + deps = next;
> + }
> + }
> +
> free(((dxe_h)dxe)->header);
> free(((dxe_h)dxe)->exp_table);
> free(dxe);
>
> return 0;
>
We run-tested the above patch with our q2dos project without any
issues.
--
O.S.
- Raw text -