On 03.03.2017 19:13, Sebastian Lackner wrote:
On 03.03.2017 18:48, Jacek Caban wrote:
Signed-off-by: Jacek Caban jacek@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.
Hmm, sure, I may change it. But what's the convention here? Given that basic functions such as NtReadFile are loaded dynamically, I was under an impression that we want to load dynamically all functions from ntdll. Now I see it's not the case for other functions. I recall considering to change that, but I think Alexandre preferred it that way (I'm not sure about that, I may remember it wrong).
} 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).
Yeah, it seems to be post-XP behaviour. I will fix that.
- 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" ?
Sure.
* 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.
OK.
Thanks for the review, Jacek