From 33297d810d9033ffca661b8f9158116602615dd7 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Tue, 21 Mar 2017 14:30:24 +0100 Subject: [PATCH] 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 --- winsup/cygwin/dlfcn.cc | 38 ++++++++++++++++++++++------ winsup/cygwin/dll_init.cc | 49 +++++++++++++++++-------------------- winsup/cygwin/release/2.8.0 | 4 +++ 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 #include +#include #include #include #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 -- 2.43.5