On Thu, Nov 4, 2010 at 1:28 AM, Pat Thoyts
<patthoyts@xxxxxxxxxxxxxxxxxxxxx> wrote:
> Erik Faye-Lund <kusmabite@xxxxxxxxx> writes:
>>On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html