ntdll: Don't modify LockCount in RtlLeaveCriticalSection if section is not acquired.

Sebastian Lackner sebastian at fds-team.de
Fri Mar 3 12:13:19 CST 2017


On 03.03.2017 18:48, Jacek Caban wrote:
> Signed-off-by: Jacek Caban <jacek at codeweavers.com>
> ---
>  dlls/ntdll/critsection.c |  6 ++++-
>  dlls/ntdll/tests/rtl.c   | 59
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+), 1 deletion(-)
> 
> 
> 
> 0001-ntdll-Don-t-modify-LockCount-in-RtlLeaveCriticalSecti.diff
> 
> 
> diff --git a/dlls/ntdll/critsection.c b/dlls/ntdll/critsection.c
> index e2eb364..e405b08 100644
> --- a/dlls/ntdll/critsection.c
> +++ b/dlls/ntdll/critsection.c
> @@ -665,7 +665,11 @@ BOOL WINAPI RtlIsCriticalSectionLockedByThread( RTL_CRITICAL_SECTION *crit )
>   */
>  NTSTATUS WINAPI RtlLeaveCriticalSection( RTL_CRITICAL_SECTION *crit )
>  {
> -    if (--crit->RecursionCount) interlocked_dec( &crit->LockCount );
> +    if (--crit->RecursionCount)
> +    {
> +        if (crit->RecursionCount > 0) interlocked_dec( &crit->LockCount );
> +        else ERR( "section %p is not acquired\n", crit );
> +    }
>      else
>      {
>          crit->OwningThread = 0;
> diff --git a/dlls/ntdll/tests/rtl.c b/dlls/ntdll/tests/rtl.c
> index 43c26bd..4b6ee35 100644
> --- a/dlls/ntdll/tests/rtl.c
> +++ b/dlls/ntdll/tests/rtl.c
> @@ -98,6 +98,9 @@ static NTSTATUS  (WINAPI *pRtlCompressBuffer)(USHORT, const UCHAR*, ULONG, PUCHA
>  static BOOL      (WINAPI *pRtlIsCriticalSectionLocked)(CRITICAL_SECTION *);
>  static BOOL      (WINAPI *pRtlIsCriticalSectionLockedByThread)(CRITICAL_SECTION *);
>  static NTSTATUS  (WINAPI *pRtlInitializeCriticalSectionEx)(CRITICAL_SECTION *, ULONG, ULONG);
> +static NTSTATUS  (WINAPI *pRtlInitializeCriticalSection)(CRITICAL_SECTION *);
> +static NTSTATUS  (WINAPI *pRtlEnterCriticalSection)(RTL_CRITICAL_SECTION *);
> +static NTSTATUS  (WINAPI *pRtlLeaveCriticalSection)(RTL_CRITICAL_SECTION *);
>  
>  static HMODULE hkernel32 = 0;
>  static BOOL      (WINAPI *pIsWow64Process)(HANDLE, PBOOL);
> @@ -151,6 +154,9 @@ static void InitFunctionPtrs(void)
>          pRtlIsCriticalSectionLocked = (void *)GetProcAddress(hntdll, "RtlIsCriticalSectionLocked");
>          pRtlIsCriticalSectionLockedByThread = (void *)GetProcAddress(hntdll, "RtlIsCriticalSectionLockedByThread");
>          pRtlInitializeCriticalSectionEx = (void *)GetProcAddress(hntdll, "RtlInitializeCriticalSectionEx");
> +        pRtlInitializeCriticalSection = (void *)GetProcAddress(hntdll, "RtlInitializeCriticalSection");
> +        pRtlEnterCriticalSection = (void *)GetProcAddress(hntdll, "RtlEnterCriticalSection");
> +        pRtlLeaveCriticalSection = (void *)GetProcAddress(hntdll, "RtlLeaveCriticalSection");

Those functions should not require dynamic loading. The *Ex function is only
loaded dynamically because it was introduced in later Windows versions.

>      }
>      hkernel32 = LoadLibraryA("kernel32.dll");
>      ok(hkernel32 != 0, "LoadLibrary failed\n");
> @@ -2094,6 +2100,58 @@ static void test_RtlInitializeCriticalSectionEx(void)
>      RtlDeleteCriticalSection(&cs);
>  }
>  
> +static void test_RtlLeaveCriticalSection(void)
> +{
> +    RTL_CRITICAL_SECTION cs;
> +    NTSTATUS status;
> +
> +    pRtlInitializeCriticalSection(&cs);
> +
> +    status = pRtlEnterCriticalSection(&cs);
> +    ok(!status, "RtlEnterCriticalSection failed: %x\n", status);
> +    todo_wine
> +    ok(cs.LockCount == -2, "expected LockCount == -2, got %d\n", cs.LockCount);

Some of these tests seem to fail on XP (see testbot).

> +    ok(cs.RecursionCount == 1, "expected RecursionCount == 1, got %d\n", cs.RecursionCount);
> +    ok(cs.OwningThread == ULongToHandle(GetCurrentThreadId()), "unexpected OwningThread\n");
> +
> +    status = pRtlLeaveCriticalSection(&cs);
> +    ok(!status, "RtlLeaveCriticalSection failed: %x\n", status);
> +    ok(cs.LockCount == -1, "expected LockCount == -1, got %d\n", cs.LockCount);
> +    ok(cs.RecursionCount == 0, "expected RecursionCount == 0, got %d\n", cs.RecursionCount);
> +    ok(!cs.OwningThread, "unexpected OwningThread %p\n", cs.OwningThread);
> +
> +    /*
> +     * Trying to leave a section that wasn't acquired modifies RecusionCount to an invalid values,

Should this be "invalid value" ?

> +     * but doesn't modify LockCount so that an attempt to enter the section later will work.
> +     */
> +    status = pRtlLeaveCriticalSection(&cs);
> +    ok(!status, "RtlLeaveCriticalSection failed: %x\n", status);
> +    ok(cs.LockCount == -1, "expected LockCount == -1, got %d\n", cs.LockCount);
> +    ok(cs.RecursionCount == -1, "expected RecursionCount == -1, got %d\n", cs.RecursionCount);
> +    ok(!cs.OwningThread, "unexpected OwningThread %p\n", cs.OwningThread);
> +
> +    /* and again */
> +    status = pRtlLeaveCriticalSection(&cs);
> +    ok(!status, "RtlLeaveCriticalSection failed: %x\n", status);
> +    ok(cs.LockCount == -1, "expected LockCount == -1, got %d\n", cs.LockCount);
> +    ok(cs.RecursionCount == -2, "expected RecursionCount == -1, got %d\n", cs.RecursionCount);

Copy and paste error in the ok message.

> +    ok(!cs.OwningThread, "unexpected OwningThread %p\n", cs.OwningThread);
> +
> +    /* entering section fixes RecursionCount */
> +    status = pRtlEnterCriticalSection(&cs);
> +    ok(!status, "RtlEnterCriticalSection failed: %x\n", status);
> +    todo_wine
> +    ok(cs.LockCount == -2, "expected LockCount == -2, got %d\n", cs.LockCount);
> +    ok(cs.RecursionCount == 1, "expected RecursionCount == 1, got %d\n", cs.RecursionCount);
> +    ok(cs.OwningThread == ULongToHandle(GetCurrentThreadId()), "unexpected OwningThread\n");
> +
> +    status = pRtlLeaveCriticalSection(&cs);
> +    ok(!status, "RtlLeaveCriticalSection failed: %x\n", status);
> +    ok(cs.LockCount == -1, "expected LockCount == -1, got %d\n", cs.LockCount);
> +    ok(cs.RecursionCount == 0, "expected RecursionCount == 0, got %d\n", cs.RecursionCount);
> +    ok(!cs.OwningThread, "unexpected OwningThread %p\n", cs.OwningThread);

Please delete the critical section after the test to avoid leaks.

> +}
> +
>  START_TEST(rtl)
>  {
>      InitFunctionPtrs();
> @@ -2125,4 +2183,5 @@ START_TEST(rtl)
>      test_RtlDecompressBuffer();
>      test_RtlIsCriticalSectionLocked();
>      test_RtlInitializeCriticalSectionEx();
> +    test_RtlLeaveCriticalSection();
>  }
> 
> 
> 




More information about the wine-devel mailing list