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

From: Pat Thoyts <patthoyts@users.sourceforge.net>
Date: Nov 4, 2010, 00:28 UTC
Message-ID: <8739rind8u.fsf@fox.patthoyts.tk>
In-Reply-To: <AANLkTin6120QbxyxHt=UM9Dpk_3C-vaD2z8RaRuut76r@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

Show 20 quoted lines

>On Wed, Nov 3, 2010 at 11:18 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> Ah, thanks. For me it's leaking a variable amount of handles per
>> ls-remote, but if I apply the following patch it's down to one. Need
>> to find that one as well...
>>
>> 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.

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);

-- 
1.7.3.1.210.g3526b.dirty