Re: [PATCH v2 06/14] mingw: use real pid

On Sat, Jan 16, 2010 at 10:12 AM, Erik Faye-Lund
[off-list ref] wrote:

On Sat, Jan 16, 2010 at 9:03 AM, Johannes Sixt [off-list ref] wrote:

quoted

On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:

quoted

On Fri, Jan 15, 2010 at 11:30 PM, Johannes Sixt [off-list ref] wrote:

quoted

On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:

quoted

@@ -729,7 +729,7 @@ static pid_t mingw_spawnve(const char *cmd, const

char **argv, char **env, return -1;
      }
      CloseHandle(pi.hThread);
-     return (pid_t)pi.hProcess;
+     return (pid_t)pi.dwProcessId;
 }

You are not using the pi.hProcess anymore, so you must close it.

No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).

Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.

Oh, I see. My planned route with this (before I looked for where the
handle was closed), was to maintain some sort of list of each started
PID and their handle, and lookup in that list instead of using
OpenProcess. I guess that would solve the problem here, but it feels a
bit nasty. Not as nasty as introducing a leak, though.

What I had in mind was something along these lines:

diff --git a/compat/mingw.c b/compat/mingw.c
index e2821b3..71201d0 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c

@@ -638,6 +638,13 @@ static int env_compare(const void *a, const void *b)
 	return strcasecmp(*ea, *eb);
 }

+struct pid_info {
+	pid_t pid;
+	HANDLE proc;
+};
+static struct pid_info *pinfo;
+static int num_pinfo;
+
 static pid_t mingw_spawnve(const char *cmd, const char **argv, char **env,
 			   int prepend_cmd)
 {

@@ -729,6 +736,13 @@ static pid_t mingw_spawnve(const char *cmd, const

char **argv, char **env,
 		return -1;
 	}
 	CloseHandle(pi.hThread);
+
+	/* store process handle */
+	num_pinfo++;
+	pinfo = xrealloc(pinfo, sizeof(struct pid_info) * num_pinfo);
+	pinfo[num_pinfo - 1].pid = pi.dwProcessId;
+	pinfo[num_pinfo - 1].proc = pi.hProcess;
+
 	return (pid_t)pi.dwProcessId;
 }

@@ -1536,6 +1550,7 @@ int waitpid(pid_t pid, int *status, unsigned options)
 	}

 	if (options == 0) {
+		int i;
 		if (WaitForSingleObject(h, INFINITE) != WAIT_OBJECT_0) {
 			CloseHandle(h);
 			return 0;

@@ -1544,6 +1559,19 @@ int waitpid(pid_t pid, int *status, unsigned options)
 		if (status)
 			GetExitCodeProcess(h, (LPDWORD)status);

+		for (i = 0; i < num_pinfo; ++i)
+			if (pinfo[i].pid == pid)
+				break;
+
+		if (i < num_pinfo) {
+			CloseHandle(pinfo[i].proc);
+			memmove(pinfo + i, pinfo + i + 1,
+			    sizeof(struct pid_info) * (num_pinfo - i - 1));
+			num_pinfo--;
+			pinfo = xrealloc(pinfo,
+			    sizeof(struct pid_info) * num_pinfo);
+		}
+
 		CloseHandle(h);
 		return pid;
 	}

-- 
Erik "kusma" Faye-Lund