Re: ntdll: Don't modify LockCount in RtlLeaveCriticalSection if section is not acquired.
On 03.03.2017 18:48, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)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(); }
On 03.03.2017 19:13, Sebastian Lackner wrote:
On 03.03.2017 18:48, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)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
On 03.03.2017 19:30, Jacek Caban wrote:
On 03.03.2017 19:13, Sebastian Lackner wrote:
On 03.03.2017 18:48, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> @@ -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).
When we still had a Windows 2000 testbot VM it was required, but I think it is obsolete now and can be removed. Alexandre might know better if there is any other reason why we should keep it. Best regards, Sebastian
Sebastian Lackner <sebastian(a)fds-team.de> writes:
On 03.03.2017 19:30, Jacek Caban wrote:
On 03.03.2017 19:13, Sebastian Lackner wrote:
On 03.03.2017 18:48, Jacek Caban wrote:
Signed-off-by: Jacek Caban <jacek(a)codeweavers.com> @@ -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).
When we still had a Windows 2000 testbot VM it was required, but I think it is obsolete now and can be removed. Alexandre might know better if there is any other reason why we should keep it.
The reason was not because the functions may be missing, but because the PSDK didn't have an ntdll import library, so we couldn't import anything from ntdll if we wanted to support building against the PSDK. It seems the latest PSDK now has an import library, so we can probably stop worrying about that case. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Jacek Caban -
Sebastian Lackner