Re: [PATCH] relocatable: free memory in DLL_PROCESS_DETACH

[Jonathan Boeing]

On Sun, 08 Aug 2021 19:18:51 +0200
Bruno Haible <bruno@clisp.org> wrote:

> > This is noticeable in Claws because we load/unload Enchant when we
> > open/close a message compose window.  
> 
> Thanks for explaining the use-case. I could not imagine why it would
> be useful to care about DLL_PROCESS_DETACH, i.e. why programs would
> repeatedly Load and Unload a library. Now I can imagine it.

(On a second look, Claws keeps Enchant loaded.  It's actually Enchant's
spelling backend that gets loaded/unloaded each time.)

> > This patch addresses the Windows case by freeing the memory in
> > DLL_PROCESS_DETACH.  
> 
> This is not correct, because the code that calls
> get_shared_library_fullname ()
> would then use a pointer to memory that has already been freed,
> and thus crash.

The library's code and data are all unmapped from memory after
DLL_PROCESS_DETACH, so wouldn't it just crash immediately if someone
called relocate() at that point?

> Instead, I'm applying a patch that avoid allocating more memory
> in case of a second DLL_PROCESS_ATTACH.

I tested that patch and it's still leaking memory.  The debugger shows
that shared_library_fullname is initialized to zero each time the
library loads.

> > It also initializes shared_library_fullname to NULL
> > so that it doesn't try to free an uninitialized pointer in the
> > unexpected situation of something failing in DLL_PROCESS_ATTACH.  
> 
> In C, 'static' storage is always zero-initialized. Do you have
> indications that this does not hold for storage that is allocated
> as part of a DLL? If so, please, can you provide a pointer to the
> documentation of it, on microsoft.com?

You're right, I was mixing up some of the tricks that app verifier can
pull with memory initialization. 

Regards,

Jonathan