From: Pat Thoyts Date: Thu, 04 Nov 2010 01:04:52 GMT Subject: Re: [PATCH v6 00/16] daemon-win32 Message-ID: <87r5f1x5jf.fsf@fox.patthoyts.tk> In-Reply-To: Erik Faye-Lund writes: >On Thu, Nov 4, 2010 at 1:28 AM, Pat Thoyts > wrote: >> Erik Faye-Lund writes: >>>On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund 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 >> 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 >> --- >>  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? > Sure. When I wrote the comment I'd forgotten about the memmove. So hmmm. However, I just pulled you win32-daemon branch and applied this patch on top. Now there are no more warnings so your union fix is good. As I understand it thats the way to handle this particular warning. ipv6 and ipv4 both still work fine as well. With my patch a while true; do git ls-remote; done shows no more leaking handles or memory. So the use of a list is fixing something. Looking great now. -- Pat Thoyts http://www.patthoyts.tk/ PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD