Re: ntdll: Fixed LdrUnloadDll may recursion unload same dll

[Sebastian Lackner]

On 09.02.2017 10:39, 谢威 wrote:
> From 3b5efb522f3ef46b009e6a159a605e087e8841a9 Mon Sep 17 00:00:00 2001
> From: Wei xie <xiewei(a)linuxdeepin.com>
> Date: Thu, 19 Jan 2017 14:55:44 +0800
> Subject: [PATCH] ntdll: Fixed LdrUnloadDll may recursion unload same dll
>
> Signed-off-by: Wei xie <xiewei(a)linuxdeepin.com>
> ---
>  dlls/ntdll/loader.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
> index 74feb97..8762917 100644
> --- a/dlls/ntdll/loader.c
> +++ b/dlls/ntdll/loader.c
> @@ -64,7 +64,7 @@ WINE_DECLARE_DEBUG_CHANNEL(pid);
>  typedef DWORD (CALLBACK *DLLENTRYPROC)(HMODULE,DWORD,LPVOID);
>
> static BOOL process_detaching = FALSE;  /* set on process detach to avoid deadlocks with thread detach */
> -static int free_lib_count;   /* recursion depth of LdrUnloadDll calls */
> +static int free_lib_count = 0;   /* recursion depth of LdrUnloadDll calls */

This isn't really necessary, all static variables are initialized to zero.
> static const char * const reason_names[] =
>  {
> @@ -2859,7 +2859,7 @@ static void MODULE_FlushModrefs(void)
>   *
>   * The loader_section must be locked while calling this function.
>   */
> -static void MODULE_DecRefCount( WINE_MODREF *wm )
> +static void MODULE_DecRefCount( WINE_MODREF *wm , BOOL bDeps)
>  {
>      int i;
>
> @@ -2872,13 +2872,13 @@ static void MODULE_DecRefCount( WINE_MODREF *wm )
>      --wm->ldr.LoadCount;
>      TRACE("(%s) ldr.LoadCount: %d\n", debugstr_w(wm->ldr.BaseDllName.Buffer), wm->ldr.LoadCount );
>
> -    if ( wm->ldr.LoadCount == 0 )
> +    if ( wm->ldr.LoadCount == 0 && bDeps)
>      {
>          wm->ldr.Flags |= LDR_UNLOAD_IN_PROGRESS;
>
> for ( i = 0; i < wm->nDeps; i++ )
>              if ( wm->deps[i] )
> -                MODULE_DecRefCount( wm->deps[i] );
> +                MODULE_DecRefCount( wm->deps[i] , TRUE );
>
> wm->ldr.Flags &= ~LDR_UNLOAD_IN_PROGRESS;
>      }
> @@ -2893,6 +2893,7 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule )
>  {
>      WINE_MODREF *wm;
>      NTSTATUS retv = STATUS_SUCCESS;
> +    static HMODULE first_recursion_module = 0;
>
> if (process_detaching) return retv;
>
> @@ -2900,13 +2901,19 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule )
>
> RtlEnterCriticalSection( &loader_section );
>
> +    if (0 == free_lib_count)
> +        first_recursion_module = hModule;
> +
>      free_lib_count++;
>      if ((wm = get_modref( hModule )) != NULL)
>      {
>          TRACE("(%s) - START\n", debugstr_w(wm->ldr.BaseDllName.Buffer));
>
> /* Recursively decrement reference counts */
> -        MODULE_DecRefCount( wm );
> +        if (0 != free_lib_count && first_recursion_module == hModule)
> +            MODULE_DecRefCount( wm , FALSE );
> +        else
> +            MODULE_DecRefCount( wm , TRUE );

At the location above free_lib_count will always be != 0 - this means that
your patch basically disables unloading of dependencies completely. Do you
have a small testcase for the problem you are trying to fix?
> /* Call process detach notifications */
>          if ( free_lib_count <= 1 )
> @@ -2922,6 +2929,9 @@ NTSTATUS WINAPI LdrUnloadDll( HMODULE hModule )
>
> free_lib_count--;
>
> +    if (0 == free_lib_count)
> +        first_recursion_module = 0;
> +
>      RtlLeaveCriticalSection( &loader_section );
>
> return retv;
> -- 2.10.2