PATCH: handle and mem leak on the win32 side

[admin]

Hi Doug,

I had some fights with the win32 code today.

Several HANDLEs didn't got closed and the way and one Heap wasn't
free()d in a error-case.

The normal way to write generic error-code is having a good old friend
joining the game again: goto 🙂

proc = OpenProcess();
if (!proc) goto error;

/* do something with proc that fails */
if (...) goto error;

...
return SIGAR_OK;
error:
if (...) CloseHandle(...);

return SIGAR_*;
}

This is basicly the same as Exceptions in Java and C++ and gives you a
central place of cleanup. Otherwise goto should be banned to death 🙂
I'm a friend of return-early too. But sometimes the duplication of the
cleanup code causes more problems, like here.

I added one FIXME to the code that I left to you:

/* FIXME: close the kdll handles */

I don't know if the lib-handles can be closed after the address is taken
for getenv() and lstrlenA(). sigar_remote_proc_env_get() leaks two
handles per call now in the normal use-case.

regards,
Jan
--
Jan Kneschke, MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Hans von Bell, Kaj Arnö - HRB München 162140

[dougm_hyperic]

Hi Jan,

Wow, I'm surprised this hasn't caused us problems. Many thanks for
the patch, I've applied it to both trunk and the SIGAR_1_3 branch.
I'm familar with the goto/error pattern, but tend not to practice
it. I agree the duplication of cleanup isn't much better, but I've
at least tried to avoid that where possible. For example, I made 2
adjustments to your patch so the cleanup is in one place. I'll re-
consider use of goto/error in the future, perhaps starting with
cleanup of of the kdll handles 🙂

[dougm_hyperic]

On Mar 7, 2007, at 3:43 PM, Doug MacEachern wrote:

...

I'll re-consider use of goto/error in the future, perhaps starting with 
cleanup of of the kdll handles 🙂

I had forgotten about this, but here's the reason there's no FreeLibrary(kdll):

"The GetModuleHandle function returns a handle to a mapped module without incrementing its reference count. Therefore, use care when passing the handle to the FreeLibrary function, because doing so can cause a DLL module to be unmapped prematurely."

See: http://msdn2.microsoft.com/en-us/library/ms683199.aspx

[admin]

Doug MacEachern wrote:
> On Mar 7, 2007, at 3:43 PM, Doug MacEachern wrote:
> ...
>>
>> I'll re-consider use of goto/error in the future, perhaps starting with
>> cleanup of of the kdll handles 🙂
>>
> I had forgotten about this, but here's the reason there's no
> FreeLibrary(kdll):
>
> "The GetModuleHandle function returns a handle to a mapped module
> without incrementing its reference count. Therefore, use care when
> passing the handle to the FreeLibrary function, because doing so can
> cause a DLL module to be unmapped prematurely."
>
> See: http://msdn2.microsoft.com/en-us/library/ms683199.aspx

I see 3 ways to handle this:
1. open a extra handle in the init-function to make sure the ref-count
never becomes zero
2. get the handle once in the init function and close in shutdown
3. get the handle once in the get_remote_proc_env function and close on
sigar-shutdown.

if (!sigar->kdll_...) {
sigar->kdll_... = GetModuleHandle(...);
}

sigar_shutdown() {
if (sigar->kdll_...) CloseHandle(sigar->kdll...);
...
}

That way they are only open once and the sigar_shutdown() can cleanup at
the end.

Jan
--
Jan Kneschke, MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Hans von Bell, Kaj Arnö - HRB München 162140

[dougm_hyperic]

I was pointing out "without incrementing its reference count".  I don't think CloseHandle() has any effect on the handle returned by GetModuleHandle(), does it?

On Mar 8, 2007, at 2:13 AM, Jan Kneschke wrote:

Doug MacEachern wrote:
> On Mar 7, 2007, at 3:43 PM, Doug MacEachern wrote:
> ...
>>
>> I'll re-consider use of goto/error in the future, perhaps starting with
>> cleanup of of the kdll handles 🙂
>>
> I had forgotten about this, but here's the reason there's no
> FreeLibrary(kdll):
>
> "The GetModuleHandle function returns a handle to a mapped module
> without incrementing its reference count. Therefore, use care when
> passing the handle to the FreeLibrary function, because doing so can
> cause a DLL module to be unmapped prematurely."
>
> See: http://msdn2.microsoft.com/en-us/library/ms683199.aspx

I see 3 ways to handle this:
1. open a extra handle in the init-function to make sure the ref-count
never becomes zero
2. get the handle once in the init function and close in shutdown
3. get the handle once in the get_remote_proc_env function and close on
sigar-shutdown.

  if (!sigar->kdll_...) {
    sigar->kdll_... = GetModuleHandle(...);
  }

sigar_shutdown() {
  if (sigar->kdll_...) CloseHandle(sigar->kdll...);
  ...
}

That way they are only open once and the sigar_shutdown() can cleanup at
the end.

Jan
--
Jan Kneschke, MySQL GmbH, Radlkoferstr. 2, D-81373 München
Geschäftsführer: Hans von Bell, Kaj Arnö - HRB München 162140