X-Authentication-Warning: delorie.com: mail set sender to djgpp-bounces using -f
X-Recipient: djgpp AT delorie DOT com
X-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20120113;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :content-type;
        bh=jS/0OLeLtc2OSqWJeQoSN0Uq7PrXKZwvc46YhDVxgHs=;
        b=eMZTyDi9thvA+juv2AVeJwU5ykZbuVWNBL3fQUtqUmERuPHsH6CPBzdbJz6kzEaXZo
         1gFxnNtFalk9s8vMV7Pr4wJCYP/4b19PtuPEPdRstOjADjE5wWraGPzRND608OnwroYW
         D8569vw9bwVpT3PT6QGn3Zt8347da1H7HuXrd2/7X/7kpYLGVLG/rxhObEoE72pU63Yu
         WAaTyI9xOtPRc52ZlAGhmhvevyJdZPOUyN22VJeZPsVYr2w3qbr4v/oV9VHfLkUDTgPn
         es/kTIRifpHAPyETGcVa3IfVdWauRu/B/h1Wr9l6dMS0TOfLaI8Ovv5z2Lad0Obvs1VK
         VxuA==
MIME-Version: 1.0
X-Received: by 10.107.17.206 with SMTP id 75mr29282843ior.140.1443976374961;
 Sun, 04 Oct 2015 09:32:54 -0700 (PDT)
In-Reply-To: <CAA2C=vAaGfzEU1cYVYda+KuV29haYtDCSV9dzxnb4UMjOGgYhA@mail.gmail.com>
References: <CAA2C=vAwcH9pHN63=Mskr9L016yAAJ6KkMPeuO9o_2cV7Pd0Kw AT mail DOT gmail DOT com>
	<CAA2C=vDDN8UqGpbAzkba19Syq-1mLsBPAuSzSPWue_S2TYf_XQ AT mail DOT gmail DOT com>
	<b8759e89-e375-4647-a9eb-64543cc49748 AT googlegroups DOT com>
	<CAA2C=vDSknbCKHzUA954weavLj16nXuDyP_Ggi+SmLJ_e-U_KA AT mail DOT gmail DOT com>
	<CAA2C=vAdzu+ebq1mEsoqwkxJcYR+2EZdKJ3d5XxOZRCytL6z8w AT mail DOT gmail DOT com>
	<952a68b4-223b-4222-b456-35514bb8b7eb AT googlegroups DOT com>
	<CAA2C=vA0sYw19muvCeX-mY3hegXnpt2P76wn73+fbdJfdj+DwQ AT mail DOT gmail DOT com>
	<561101C4 DOT 7040701 AT gmx DOT de>
	<56111BF6 DOT 8040408 AT gmx DOT de>
	<CAA2C=vAaGfzEU1cYVYda+KuV29haYtDCSV9dzxnb4UMjOGgYhA AT mail DOT gmail DOT com>
Date: Sun, 4 Oct 2015 19:32:54 +0300
Message-ID: <CAA2C=vD0h_NLv0Vc951j4JmBXhM2HZR-cRMyscFHHLGbs6CgZA@mail.gmail.com>
Subject: Re: dlclose not removing dependency dxes
From: "Ozkan Sezer (sezeroz AT gmail DOT com) [via djgpp AT delorie DOT com]" <djgpp AT delorie DOT com>
To: djgpp AT delorie DOT com
Content-Type: text/plain; charset=UTF-8
Reply-To: djgpp AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

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.