Re: [PATCH v6 00/16] daemon-win32

Erik Faye-Lund [off-list ref] writes:

On Thu, Nov 4, 2010 at 1:28 AM, Pat Thoyts
[off-list ref] wrote:

quoted

Erik Faye-Lund [off-list ref] writes:

quoted

On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund [off-list ref] wrote:

quoted

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...

quoted

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 <redacted>
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 <redacted>
---
 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