Erik Faye-Lund <kusmabite@xxxxxxxxx> writes:
>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?
>
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
--
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