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