Re: [PATCH] relocatable: free memory in DLL_PROCESS_DETACH

[Bruno Haible]

Hi,

Jonathan Boeing wrote:
> Claws Mail uses the Enchant spell checking library, which we build with
> relocatable support for Windows.
> 
> Running with app verifier enabled caught a memory leak in
> lib/relocatable.c.  The memory allocated in DllMain() by:
> 
> shared_library_fullname = strdup (location);
> 
> is never freed.  _DLL_InitTerm() looks like it has the same issue, and
> find_shared_library_fullname() seems to have a similar issue with the
> call to getline().
> 
> 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.

> 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.

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

> 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?


2021-08-08  Bruno Haible  <bruno@clisp.org>

        relocatable-lib-lgpl: Fix a memory leak related to a Windows DLL.
        Reported by Jonathan Boeing <jonathan@claws-mail.org> in
        <https://lists.gnu.org/archive/html/bug-gnulib/2021-08/msg00048.html>.
        * lib/relocatable.c (DllMain): Avoid memory leak in a special case
        of repeated attach/detach.

diff --git a/lib/relocatable.c b/lib/relocatable.c
index ababc35..a682912 100644
--- a/lib/relocatable.c
+++ b/lib/relocatable.c
@@ -323,7 +323,10 @@ static char *shared_library_fullname;
    supports longer file names
    (see <https://cygwin.com/ml/cygwin/2011-01/msg00410.html>).  */
 
-/* Determine the full pathname of the shared library when it is loaded.  */
+/* Determine the full pathname of the shared library when it is loaded.
+
+   Documentation:
+   <https://docs.microsoft.com/en-us/windows/win32/dlls/dllmain>  */
 
 BOOL WINAPI
 DllMain (HINSTANCE module_handle, DWORD event, LPVOID reserved)
@@ -343,7 +346,13 @@ DllMain (HINSTANCE module_handle, DWORD event, LPVOID 
reserved)
         /* Shouldn't happen.  */
         return FALSE;
 
-      shared_library_fullname = strdup (location);
+      /* Avoid a memory leak when the same DLL get attached, detached,
+         attached, detached, and so on.  This happens e.g. when a spell
+         checker DLL is used repeatedly by a mail program.  */
+      if (!(shared_library_fullname != NULL
+            && strcmp (shared_library_fullname, location) == 0))
+        /* Remember the full pathname of the shared library.  */
+        shared_library_fullname = strdup (location);
     }
 
   return TRUE;