CONSOLE_INFORMATION::IsConsoleLocked and CONSOLE_INFORMATION::GetCSRecursionCount inappropriately groveling into opaque CRITICAL_SECTION object internals #1032

New Issue

claunia · 2026-01-30T22:14:18Z

claunia commented

2026-01-30 22:14:18 +00:00

Originally created by @DHowett-MSFT on GitHub (May 13, 2019).

These two functions appear to be peering into the internal opaque CRITICAL_SECTION object. The CRITICAL_SECTION definition is opaque and shouldn't be used by apps (it can - and has - changed layout between OS versions so long as the size is the same).

The CONSOLE_INFORMATION object should track an owning thread ID and critical section entry count itself if it needs to get this information and shouldn't try to assume that it knows about the critical section implementation details (particularly for OSS'd code we'd like to avoid encouraging that sort of thing as it inevitably leads to broken apps on future OS updates etc.).

bool CONSOLE_INFORMATION::IsConsoleLocked() const
{
// The critical section structure's OwningThread field contains the ThreadId despite having the HANDLE type.
// This requires us to hard cast the ID to compare.
return _csConsoleLock.OwningThread == (HANDLE)GetCurrentThreadId();
}

ULONG CONSOLE_INFORMATION::GetCSRecursionCount()
{
return _csConsoleLock.RecursionCount;
}

Originally created by @DHowett-MSFT on GitHub (May 13, 2019). > These two functions appear to be peering into the internal opaque CRITICAL_SECTION object. The CRITICAL_SECTION definition is opaque and shouldn't be used by apps (it can - and has - changed layout between OS versions so long as the size is the same). > > The CONSOLE_INFORMATION object should track an owning thread ID and critical section entry count itself if it needs to get this information and shouldn't try to assume that it knows about the critical section implementation details (particularly for OSS'd code we'd like to avoid encouraging that sort of thing as it inevitably leads to broken apps on future OS updates etc.). > > bool CONSOLE_INFORMATION::IsConsoleLocked() const > { > // The critical section structure's OwningThread field contains the ThreadId despite having the HANDLE type. > // This requires us to hard cast the ID to compare. > return _csConsoleLock.OwningThread == (HANDLE)GetCurrentThreadId(); > } > > > ULONG CONSOLE_INFORMATION::GetCSRecursionCount() > { > return _csConsoleLock.RecursionCount; > } >

claunia added the Product-Conhost Issue-Bug Area-Server labels 2026-01-30 22:14:18 +00:00

claunia commented

2026-01-30 22:14:19 +00:00

@miniksa commented on GitHub (May 13, 2019):

I've known for a while that this is bad. I want to convert to using some less crappy lock as well, but the whole "performing work when the lock count hits zero" part of the conhost.exe server is load bearing and needs careful detangling.

Hopefully I can get this onto a lock that has an auto-free on scope exit as well to clean up all the freaking wil::scope_exits around the codebase as well.

@miniksa commented on GitHub (May 13, 2019): I've known for a while that this is bad. I want to convert to using some less crappy lock as well, but the whole "performing work when the lock count hits zero" part of the `conhost.exe` server is load bearing and needs careful detangling. Hopefully I can get this onto a lock that has an auto-free on scope exit as well to clean up all the freaking `wil::scope_exit`s around the codebase as well.