Cygwin: dlfcn: Fix reference counting

The original dll_init code was living under the wrong assumption that
dll_dllcrt0_1 and in turn dll_list::alloc will be called for each
LoadLibrary call.  The same wrong assumption was made for
cygwin_detach_dll/dll_list::detach called via FreeLibrary.

In reality, dll_dllcrt0_1 gets only called once at first LoadLibrary
and cygwin_detach_dll once at last FreeLibrary.

In effect, reference counting for DLLs was completely broken after fork:

  parent:
    l1 = dlopen ("lib1");  // LoadLibrary, LoadCount = 1
    l2 = dlopen ("lib1");  // LoadLibrary, LoadCount = 2

    fork ();               // LoadLibrary in the child, LoadCount = 1!
      child:
        dlclose (l1);      // FreeLibrary actually frees the lib
        x = dlsym (l2);    // SEGV

* Move reference counting to dlopen/dlclose since only those functions
  have to keep track of loading/unloading DLLs in the application context.

* Remove broken accounting code from dll_list::alloc and dll_list::detach.

* Fix error handling in dlclose.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

3 files changed, 57 insertions, 34 deletions

diff --git a/winsup/cygwin/dlfcn.cc b/winsup/cygwin/dlfcn.cc
index 9959ff752..06e685447 100644
--- a/winsup/cygwin/dlfcn.cc
+++ b/winsup/cygwin/dlfcn.cc
@@ -9,14 +9,15 @@ details. */
 #include "winsup.h"
 #include <psapi.h>
 #include <stdlib.h>
+#include <dlfcn.h>
 #include <ctype.h>
 #include <wctype.h>
 #include "path.h"
 #include "fhandler.h"
 #include "dtable.h"
+#include "dll_init.h"
 #include "cygheap.h"
 #include "perprocess.h"
-#include "dlfcn.h"
 #include "cygtls.h"
 #include "tls_pbuf.h"
 #include "ntdll.h"
@@ -291,6 +292,13 @@ dlopen (const char *name, int flags)
 #endif
 
       ret = (void *) LoadLibraryW (wpath);
+      /* reference counting */
+      if (ret)
+	{
+	  dll *d = dlls.find (ret);
+	  if (d)
+	    ++d->count;
+	}
 
 #ifndef __x86_64__
       /* Restore original cxx_malloc pointer. */
@@ -361,13 +369,27 @@ dlsym (void *handle, const char *name)
 extern "C" int
 dlclose (void *handle)
 {
-  int ret;
-  if (handle == GetModuleHandle (NULL))
-    ret = 0;
-  else if (FreeLibrary ((HMODULE) handle))
-    ret = 0;
-  else
-    ret = -1;
+  int ret = 0;
+  if (handle != GetModuleHandle (NULL))
+    {
+      /* reference counting */
+      dll *d = dlls.find (handle);
+      if (d) system_printf ("%W count %d", d->name, d->count);
+      if (!d || d->count <= 0)
+	{
+	  errno = ENOENT;
+	  ret = -1;
+	}
+      else
+	{
+	  --d->count;
+	  if (!FreeLibrary ((HMODULE) handle))
+	    {
+	      __seterrno ();
+	      ret = -1;
+	    }
+	}
+    }
   if (ret)
     set_dl_error ("dlclose");
   return ret;
diff --git a/winsup/cygwin/dll_init.cc b/winsup/cygwin/dll_init.cc
index 86776f2ce..490f1d688 100644
--- a/winsup/cygwin/dll_init.cc
+++ b/winsup/cygwin/dll_init.cc
@@ -200,9 +200,8 @@ dll_list::alloc (HINSTANCE h, per_process *p, dll_type type)
   dll *d = (type == DLL_LINK) ? dlls.find_by_modname (modname) : dlls[name];
   if (d)
     {
-      if (!in_forkee)
-	d->count++;	/* Yes.  Bump the usage count. */
-      else if (d->handle != h)
+      /* We only get here in the forkee. */
+      if (d->handle != h)
 	fabort ("%W: Loaded to different address: parent(%p) != child(%p)",
 		name, d->handle, h);
       /* If this DLL has been linked against, and the full path differs, try
@@ -224,15 +223,14 @@ dll_list::alloc (HINSTANCE h, per_process *p, dll_type type)
     }
   else
     {
-      /* FIXME: Change this to new at some point. */
-      d = (dll *) cmalloc (HEAP_2_DLL, sizeof (*d) + (namelen * sizeof (*name)));
-
+      d = (dll *) cmalloc (HEAP_2_DLL,
+			   sizeof (*d) + (namelen * sizeof (*name)));
       /* Now we've allocated a block of information.  Fill it in with the
 	 supplied info about this DLL. */
-      d->count = 1;
       wcscpy (d->name, name);
       d->modname = d->name + (modname - name);
       d->handle = h;
+      d->count = 0;	/* Reference counting performed in dlopen/dlclose. */
       d->has_dtors = true;
       d->p = p;
       d->ndeps = 0;
@@ -438,25 +436,21 @@ dll_list::detach (void *retaddr)
   guard (true);
   if ((d = find (retaddr)))
     {
-      if (d->count <= 0)
-	system_printf ("WARNING: trying to detach an already detached dll ...");
-      if (--d->count == 0)
-	{
-	  /* Ensure our exception handler is enabled for destructors */
-	  exception protect;
-	  /* Call finalize function if we are not already exiting */
-	  if (!exit_state)
-	    __cxa_finalize (d->handle);
-	  d->run_dtors ();
-	  d->prev->next = d->next;
-	  if (d->next)
-	    d->next->prev = d->prev;
-	  if (d->type == DLL_LOAD)
-	    loaded_dlls--;
-	  if (end == d)
-	    end = d->prev;
-	  cfree (d);
-	}
+      system_printf ("HERE %W", d->name);
+      /* Ensure our exception handler is enabled for destructors */
+      exception protect;
+      /* Call finalize function if we are not already exiting */
+      if (!exit_state)
+	__cxa_finalize (d->handle);
+      d->run_dtors ();
+      d->prev->next = d->next;
+      if (d->next)
+	d->next->prev = d->prev;
+      if (d->type == DLL_LOAD)
+	loaded_dlls--;
+      if (end == d)
+	end = d->prev;
+      cfree (d);
     }
   guard (false);
 }
@@ -641,6 +635,9 @@ void dll_list::load_after_fork_impl (HANDLE parent, dll* d, int retries)
       if (h != d->handle)
 	fabort ("unable to map %W to same address as parent: %p != %p",
 		d->modname, d->handle, h);
+      /* Fix OS reference count. */
+      for (int cnt = 1; cnt < d->count; ++cnt)
+	LoadLibraryW (d->name);
     }
 }
 
diff --git a/winsup/cygwin/release/2.8.0 b/winsup/cygwin/release/2.8.0
index 2ccbcf10e..92dff7421 100644
--- a/winsup/cygwin/release/2.8.0
+++ b/winsup/cygwin/release/2.8.0
@@ -33,3 +33,7 @@ Bug Fixes
 
 - Fix a potential crash in duplocale.
   Addresses: https://sourceware.org/ml/newlib/2017/msg00166.html
+
+- Fix dlopen/dlclose reference counting to make sure FreeLibrary isn't
+  called too early in forked process.
+  Addresses: https://cygwin.com/ml/cygwin/2017-03/msg00220.html