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

[Jacek Caban]

On 03.03.2017 19:13, Sebastian Lackner wrote:
> 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.

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