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=Gd0AH4Bninuwmvf57lehICTQpKd27nOiq+M21xsKGS4=;
        b=YYcx/B2HCHqQp9BPJ3rQX8UO7VaDbn4/WjPjcTid121qXnIDkkeeNRo2Z/XBuR/WXv
         u//LlM0ePscHLM/5Ncwc4oiCWF6+xLGraXEpsTLUCh4hU9m6nYp+pfnhQ4/MW0vWvaH/
         xgqlxiL8Nt1i5LKSj+wHuqweIqPDUgFLP+x6CnC+A5p5N98m+eA6lzhRbrOSlpXx8tt5
         Z0UMaCMiUU8Z/sZOsAuAELHHpS44AurgR7A1N4gRLl6rUmfPiIsGdX9umfoq8uOreD3c
         15zPu/WuzJt0MBt2fORs8zN09LCv/KiOEnJXmD/egK8H2Qv7EwSeAJNdCiavtzX9uFsM
         NZ9g==
MIME-Version: 1.0
X-Received: by 10.50.114.1 with SMTP id jc1mr7668612igb.56.1444037559712; Mon,
 05 Oct 2015 02:32:39 -0700 (PDT)
In-Reply-To: <CAA2C=vAs5kHn8H7LBCq3Tek2MXLpOFOEMxOUZ7eWTP4K=-Z2bA@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>
	<CAA2C=vD0h_NLv0Vc951j4JmBXhM2HZR-cRMyscFHHLGbs6CgZA AT mail DOT gmail DOT com>
	<CAA2C=vCqpCDom_-WwSx7hVq0rNvS4gNo5znTzzDST8NWnZOgLg AT mail DOT gmail DOT com>
	<CAA2C=vAs5kHn8H7LBCq3Tek2MXLpOFOEMxOUZ7eWTP4K=-Z2bA AT mail DOT gmail DOT com>
Date: Mon, 5 Oct 2015 12:32:39 +0300
Message-ID: <CAA2C=vCdOLNknMoeeDWM74_JxQJr1kWhfuDbWTVnjgK_fOVyeA@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, 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.