From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Thu, 04 Nov 2010 00:53:57 GMT
Subject: Re: [PATCH v6 00/16] daemon-win32
Message-ID: <AANLkTimcYdQD-En+SfkH5dkaxaWXveuA5Oz-Hn0Cf1v+@mail.gmail.com>
In-Reply-To: <8739rind8u.fsf@fox.patthoyts.tk>

On Thu, Nov 4, 2010 at 1:28 AM, Pat Thoyts
<patthoyts@users.sourceforge.net> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>> diff --git a/compat/mingw.c b/compat/mingw.c
>>> index b780200..47e7d26 100644
>>> --- a/compat/mingw.c
>>> +++ b/compat/mingw.c
>>> @@ -1519,8 +1519,10 @@ pid_t waitpid(pid_t pid, int *status, unsigned options)
>>>        }
>>>
>>>        if (pid > 0 && options & WNOHANG) {
>>> -               if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
>>> +               if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0)) {
>>
>>AAAND the last one is right here as well:
>>-              if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
>>+              if (WAIT_OBJECT_0 != WaitForSingleObject(h, 0)) {
>>
>
> Applying both of these doesn't fix the handle leak when I test
> this. Looking further I believe it is due to the use of a reallocated
> array. When you remove a pinfo structure you realloc to the one you
> found, potentially freeing items you still require.
>

If this is the reason, then that's a bug. The code TRIES to do the
right thing by memmove'ing the the entro to be deleted to the end.
Looking over the code, I still don't see anyhing obviously wrong.
But...

> Attached is a patch that switches to a linked list for this
> instead. Using this I no longer accumulate leaked handles.
>
> ----
> From e0c05f8f9ed8729648eea92cf654f357fa884e40 Mon Sep 17 00:00:00 2001
> From: Pat Thoyts <patthoyts@users.sourceforge.net>
> Date: Thu, 4 Nov 2010 00:23:08 +0000
> Subject: [PATCH] win32-daemon: fix handle leaks
>
> The use of an array of pinfo structures and the realloc used when cleaning
> up a closed child can free structures that are still in use. Use a linked
> list instead.
> This stops the leaking of handles in the win32-daemon.
>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
>  compat/mingw.c |   43 ++++++++++++++++++++++++-------------------
>  1 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index b780200..29f4036 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -637,11 +637,12 @@ static int env_compare(const void *a, const void *b)
>        return strcasecmp(*ea, *eb);
>  }
>
> -struct {
> +struct pinfo_t {
> +       struct pinfo_t *next;
>        pid_t pid;
>        HANDLE proc;
> -} *pinfo;
> -static int num_pinfo;
> +} pinfo_t;
> +struct pinfo_t *pinfo = NULL;
>  CRITICAL_SECTION pinfo_cs;
>
>  static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
> @@ -746,10 +747,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **env,
>         * Keep the handle in a list for waitpid.
>         */
>        EnterCriticalSection(&pinfo_cs);
> -       num_pinfo++;
> -       pinfo = xrealloc(pinfo, sizeof(*pinfo) * num_pinfo);
> -       pinfo[num_pinfo - 1].pid = pi.dwProcessId;
> -       pinfo[num_pinfo - 1].proc = pi.hProcess;
> +       {
> +               struct pinfo_t *info = xmalloc(sizeof(struct pinfo_t));
> +               info->pid = pi.dwProcessId;
> +               info->proc = pi.hProcess;
> +               info->next = pinfo;
> +               pinfo = info;
> +       }
>        LeaveCriticalSection(&pinfo_cs);
>
>        return (pid_t)pi.dwProcessId;
> @@ -1519,13 +1523,15 @@ pid_t waitpid(pid_t pid, int *status, unsigned options)
>        }
>
>        if (pid > 0 && options & WNOHANG) {
> -               if (WAIT_OBJECT_0 != WaitForSingleObject((HANDLE)pid, 0))
> +               if (WAIT_OBJECT_0 != WaitForSingleObject(h, 0)) {
> +                       CloseHandle(h);
>                        return 0;
> +               }
>                options &= ~WNOHANG;
>        }
>
>        if (options == 0) {
> -               int i;
> +               struct pinfo_t **ppinfo;
>                if (WaitForSingleObject(h, INFINITE) != WAIT_OBJECT_0) {
>                        CloseHandle(h);
>                        return 0;
> @@ -1536,17 +1542,16 @@ pid_t waitpid(pid_t pid, int *status, unsigned options)
>
>                EnterCriticalSection(&pinfo_cs);
>
> -               for (i = 0; i < num_pinfo; ++i)
> -                       if (pinfo[i].pid == pid)
> +               ppinfo = &pinfo;
> +               while (*ppinfo) {
> +                       struct pinfo_t *info = *ppinfo;
> +                       if (info->pid == pid) {
> +                               CloseHandle(info->proc);
> +                               *ppinfo = info->next;
> +                               free(info);
>                                break;
> -
> -               if (i < num_pinfo) {
> -                       CloseHandle(pinfo[i].proc);
> -                       memmove(pinfo + i, pinfo + i + 1,
> -                           sizeof(*pinfo) * (num_pinfo - i - 1));
> -                       num_pinfo--;
> -                       pinfo = xrealloc(pinfo,
> -                           sizeof(*pinfo) * num_pinfo);
> +                       }
> +                       ppinfo = &info->next;
>                }
>
>                LeaveCriticalSection(&pinfo_cs);

...yeah, using a linked list is more elegant. Do you mind if I snatch
your code and leave a comment in the commit message?