Mail Archives: djgpp/2015/10/04/12:33:09
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]
>>
>>
>> There was a typo in the previously posted patch. Please use this one.
>>
>> Regards,
>> Juan M. Guerrero
>>
>
> The patch uses very long type and variable names: was hard to read
> for me. My version is as follows. (Also attached as a patch file, because
> gmail will mangle it.)
>
> I have some concerns with the patch itself, will outline them in the
> next mail.
>
> 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 14:55:50 -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,11 +252,11 @@ 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;
> tmp.name = realfn;
> tmp.next = stk_top;
> stk_top = &tmp;
> @@ -252,14 +264,35 @@ found:
> if (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)
> + {
> + errno = ENOMEM;
> + goto midwayerror;
> + }
> + next->handle = dxe_chain;
> + 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 +358,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);
> @@ -361,10 +434,21 @@ int dlclose(void *dxe)
> {
> dxe_h *cur;
> for (cur = &dxe_chain; *cur; cur = &(*cur)->next)
> if (*cur == dxe)
> {
> + /* Un-register all implicitly loaded modules by this module. */
> + dxe_list *deps = (*cur)->deps;
> + while (deps)
> + {
> + dxe_list *next = deps->next;
> +
> + dlclose(deps->handle);
> + free(deps);
> + deps = next;
> + }
> +
> *cur = ((dxe_h)dxe)->next;
> break;
> }
> }
>
>
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.
--
O.S.
- Raw text -