From 4b1285de485e0fea5d5c35cccb58d2096010254e Mon Sep 17 00:00:00 2001 From: Torin Date: Sun, 22 Mar 2026 21:05:10 +0200 Subject: [PATCH] fix: correct ZwCreateFile disposition, NTSTATUS errors, recursion, and debug buffer ZwCreateFile (File.c): return correct IoStatusBlock->Information based on actual disposition (FILE_CREATED, FILE_OPENED, FILE_SUPERSEDED, etc.) using GetLastError() after CreateFileW. Add missing error translations for STATUS_OBJECT_NAME_COLLISION, STATUS_SHARING_VIOLATION, STATUS_OBJECT_PATH_NOT_FOUND. ZwDeleteKey/ZwDeleteKeyEx (Registry.c): convert LSTATUS to proper NTSTATUS codes via RegistryErrorToNtstatus helper instead of returning raw Win32 error values. ExAcquireResourceExclusiveLite/SharedLite (Resource.c): replace unbounded recursion with for(;;) loop to prevent stack overflow under sustained contention. DebugPrintInternal (Debug.h): always null-terminate stack buffer even when _vsnprintf_s fills it completely, preventing printf from reading past the buffer. NtStatus.h: add STATUS_OBJECT_NAME_COLLISION, STATUS_SHARING_VIOLATION. Tests updated to match corrected kernel semantics. Co-Authored-By: Claude Opus 4.6 (1M context) --- include/Debug.h | 14 ++-- include/NtStatus.h | 8 +++ src/File.c | 59 ++++++++++++++--- src/Registry.c | 27 ++++---- src/Resource.c | 132 ++++++++++++++++--------------------- tests/test_file_io.cpp | 4 +- tests/test_zwdeletekey.cpp | 7 +- 7 files changed, 142 insertions(+), 109 deletions(-) diff --git a/include/Debug.h b/include/Debug.h index 2a72ed9..1091c5e 100644 --- a/include/Debug.h +++ b/include/Debug.h @@ -292,12 +292,14 @@ inline void DebugPrintInternal(DEBUG_LEVEL level, DEBUG_COMPONENT component, pos += _vsnprintf_s(buffer + pos, bufferSize - pos, _TRUNCATE, format, args); va_end(args); - /* Ensure newline at end */ - if (pos > 0 && buffer[pos - 1] != '\n') { - if (pos < bufferSize - 2) { - buffer[pos++] = '\n'; - buffer[pos] = '\0'; - } + /* Ensure null termination and newline at end */ + if (pos < 0) pos = 0; + if (pos >= bufferSize) pos = bufferSize - 1; + buffer[pos] = '\0'; + + if (pos > 0 && buffer[pos - 1] != '\n' && pos < bufferSize - 2) { + buffer[pos++] = '\n'; + buffer[pos] = '\0'; } /* Lock only for output - formatting was done lock-free above */ diff --git a/include/NtStatus.h b/include/NtStatus.h index a260d84..54f6d4b 100644 --- a/include/NtStatus.h +++ b/include/NtStatus.h @@ -71,6 +71,14 @@ #define STATUS_OBJECT_PATH_SYNTAX_BAD ((NTSTATUS)0xC0000039L) #endif +#ifndef STATUS_OBJECT_NAME_COLLISION +#define STATUS_OBJECT_NAME_COLLISION ((NTSTATUS)0xC0000035L) +#endif + +#ifndef STATUS_SHARING_VIOLATION +#define STATUS_SHARING_VIOLATION ((NTSTATUS)0xC0000043L) +#endif + // Helper macros #ifndef NT_SUCCESS #define NT_SUCCESS(Status) (((NTSTATUS)(Status)) >= 0) diff --git a/src/File.c b/src/File.c index 6f80793..f5836b3 100644 --- a/src/File.c +++ b/src/File.c @@ -76,18 +76,61 @@ NTSTATUS ZwCreateFile( if (*FileHandle == INVALID_HANDLE_VALUE) { DWORD error = GetLastError(); - IoStatusBlock->Status = STATUS_UNSUCCESSFUL; + IoStatusBlock->Information = 0; - if (error == ERROR_FILE_NOT_FOUND) { - return STATUS_OBJECT_NAME_NOT_FOUND; - } else if (error == ERROR_ACCESS_DENIED) { - return STATUS_ACCESS_DENIED; - } else { - return STATUS_UNSUCCESSFUL; + switch (error) { + case ERROR_FILE_NOT_FOUND: + IoStatusBlock->Status = STATUS_OBJECT_NAME_NOT_FOUND; + return STATUS_OBJECT_NAME_NOT_FOUND; + case ERROR_PATH_NOT_FOUND: + IoStatusBlock->Status = STATUS_OBJECT_PATH_NOT_FOUND; + return STATUS_OBJECT_PATH_NOT_FOUND; + case ERROR_ACCESS_DENIED: + IoStatusBlock->Status = STATUS_ACCESS_DENIED; + return STATUS_ACCESS_DENIED; + case ERROR_FILE_EXISTS: + case ERROR_ALREADY_EXISTS: + IoStatusBlock->Status = STATUS_OBJECT_NAME_COLLISION; + return STATUS_OBJECT_NAME_COLLISION; + case ERROR_SHARING_VIOLATION: + IoStatusBlock->Status = STATUS_SHARING_VIOLATION; + return STATUS_SHARING_VIOLATION; + default: + IoStatusBlock->Status = STATUS_UNSUCCESSFUL; + return STATUS_UNSUCCESSFUL; } } IoStatusBlock->Status = STATUS_SUCCESS; - IoStatusBlock->Information = FILE_OPENED; /* Simplified */ + + /* Determine Information based on disposition and whether file existed */ + { + DWORD lastError = GetLastError(); + switch (CreateDisposition) { + case FILE_SUPERSEDE: + IoStatusBlock->Information = FILE_SUPERSEDED; + break; + case FILE_OPEN: + IoStatusBlock->Information = FILE_OPENED; + break; + case FILE_CREATE: + IoStatusBlock->Information = FILE_CREATED; + break; + case FILE_OPEN_IF: + /* CREATE_ALWAYS/OPEN_ALWAYS set ERROR_ALREADY_EXISTS when file existed */ + IoStatusBlock->Information = (lastError == ERROR_ALREADY_EXISTS) ? FILE_OPENED : FILE_CREATED; + break; + case FILE_OVERWRITE: + IoStatusBlock->Information = FILE_OVERWRITTEN; + break; + case FILE_OVERWRITE_IF: + IoStatusBlock->Information = (lastError == ERROR_ALREADY_EXISTS) ? FILE_OVERWRITTEN : FILE_CREATED; + break; + default: + IoStatusBlock->Information = FILE_OPENED; + break; + } + } + return STATUS_SUCCESS; } diff --git a/src/Registry.c b/src/Registry.c index 2c806ed..9786794 100644 --- a/src/Registry.c +++ b/src/Registry.c @@ -755,25 +755,28 @@ NTSTATUS ZwEnumerateValueKey( return STATUS_SUCCESS; } +/* Helper: convert common Win32 registry errors to NTSTATUS */ +static NTSTATUS RegistryErrorToNtstatus(LSTATUS status) { + switch (status) { + case ERROR_SUCCESS: return STATUS_SUCCESS; + case ERROR_FILE_NOT_FOUND: return STATUS_OBJECT_NAME_NOT_FOUND; + case ERROR_ACCESS_DENIED: return STATUS_ACCESS_DENIED; + case ERROR_INVALID_PARAMETER: return STATUS_INVALID_PARAMETER; + case ERROR_NO_MORE_ITEMS: return STATUS_NO_MORE_ENTRIES; + default: return STATUS_UNSUCCESSFUL; + } +} + // ZwDeleteKey: Deletes a registry key using RegDeleteKeyW. Only works for empty keys (not recursive). NTSTATUS ZwDeleteKey(HANDLE KeyHandle) { - // This deletes the key that KeyHandle refers to - // We need to close the handle after the key is deleted LSTATUS status = RegDeleteKeyW((HKEY)KeyHandle, L""); + NTSTATUS ntStatus = RegistryErrorToNtstatus(status); - if (status != ERROR_SUCCESS) { - // Try to close the handle to avoid leaks even if delete failed - RegCloseKey((HKEY)KeyHandle); - return status; - } - - // Key was deleted, now close the handle (safe because RegDeleteKeyW doesn't invalidate it) RegCloseKey((HKEY)KeyHandle); - return STATUS_SUCCESS; + return ntStatus; } NTSTATUS ZwDeleteKeyEx(HKEY KeyHandle, LPCWSTR SubKey) { - // RegDeleteKeyW returns ERROR_SUCCESS (0) on success, map to STATUS_SUCCESS (0) LSTATUS status = RegDeleteKeyW(KeyHandle, SubKey); - return (status == ERROR_SUCCESS) ? STATUS_SUCCESS : status; + return RegistryErrorToNtstatus(status); } diff --git a/src/Resource.c b/src/Resource.c index b225924..c153117 100644 --- a/src/Resource.c +++ b/src/Resource.c @@ -121,57 +121,48 @@ ExAcquireResourceExclusiveLite( ) { ERESOURCE_THREAD CurrentThread; - BOOLEAN Result = FALSE; if (!Resource) return FALSE; - // Get the current thread ID as the resource thread CurrentThread = (ERESOURCE_THREAD)GetCurrentThreadId(); - // Enter the critical section if Wait is TRUE, otherwise try to enter - if (Wait) { - EnterCriticalSection(&Resource->CriticalSection); - } - else if (!TryEnterCriticalSection(&Resource->CriticalSection)) { - return FALSE; - } - - // Check if the resource is already owned - if (Resource->ActiveCount != 0) { - // If owned exclusively by the current thread, allow recursive exclusive acquisition - if (IsOwnedExclusive(Resource) && - (Resource->OwnerThreads[0].OwnerThread == CurrentThread)) { - Resource->OwnerThreads[0].OwnerCount += 1; - Result = TRUE; + for (;;) { + if (Wait) { + EnterCriticalSection(&Resource->CriticalSection); } - else { - // Resource is owned by another thread or shared - cannot acquire exclusive - if (Wait == FALSE) { - Result = FALSE; + else if (!TryEnterCriticalSection(&Resource->CriticalSection)) { + return FALSE; + } + + if (Resource->ActiveCount != 0) { + if (IsOwnedExclusive(Resource) && + (Resource->OwnerThreads[0].OwnerThread == CurrentThread)) { + Resource->OwnerThreads[0].OwnerCount += 1; + LeaveCriticalSection(&Resource->CriticalSection); + return TRUE; + } + else if (Wait == FALSE) { + LeaveCriticalSection(&Resource->CriticalSection); + return FALSE; } else { - // For simplicity in this user-mode implementation: - // If we need to wait and we're here, we know we're holding the critical section, - // but the resource is owned by someone else. We'll release and retry. + /* Owned by another thread - release CS and retry */ LeaveCriticalSection(&Resource->CriticalSection); - Sleep(1); // Yield to other threads - return ExAcquireResourceExclusiveLite(Resource, Wait); + Sleep(1); + continue; } } + else { + /* Resource is free - take it exclusively */ + Resource->Flag |= ResourceOwnedExclusive; + Resource->OwnerThreads[0].OwnerThread = CurrentThread; + Resource->OwnerThreads[0].OwnerCount = 1; + Resource->ActiveCount = 1; + LeaveCriticalSection(&Resource->CriticalSection); + return TRUE; + } } - else { - // Resource is not owned, so we can take it - Resource->Flag |= ResourceOwnedExclusive; - Resource->OwnerThreads[0].OwnerThread = CurrentThread; - Resource->OwnerThreads[0].OwnerCount = 1; - Resource->ActiveCount = 1; - Result = TRUE; - } - - // Always release the critical section - LeaveCriticalSection(&Resource->CriticalSection); - return Result; } BOOLEAN @@ -180,53 +171,42 @@ ExAcquireResourceSharedLite( IN BOOLEAN Wait ) { - ERESOURCE_THREAD CurrentThread; - BOOLEAN Result = FALSE; - if (!Resource) return FALSE; - // Get the current thread ID as the resource thread - CurrentThread = (ERESOURCE_THREAD)GetCurrentThreadId(); - - // Enter the critical section if Wait is TRUE, otherwise try to enter - if (Wait) { - EnterCriticalSection(&Resource->CriticalSection); - } - else if (!TryEnterCriticalSection(&Resource->CriticalSection)) { - return FALSE; - } + for (;;) { + if (Wait) { + EnterCriticalSection(&Resource->CriticalSection); + } + else if (!TryEnterCriticalSection(&Resource->CriticalSection)) { + return FALSE; + } - // Check if the resource is already owned - if (Resource->ActiveCount != 0) { - if (IsOwnedExclusive(Resource)) { - if (Wait == FALSE) { - Result = FALSE; - } - else { - // For simplicity in this user-mode implementation: - // If we need to wait and we're here, we release and retry - LeaveCriticalSection(&Resource->CriticalSection); - Sleep(1); // Yield to other threads - return ExAcquireResourceSharedLite(Resource, Wait); + if (Resource->ActiveCount != 0) { + if (IsOwnedExclusive(Resource)) { + if (Wait == FALSE) { + LeaveCriticalSection(&Resource->CriticalSection); + return FALSE; + } + else { + /* Owned exclusively by another thread - release CS and retry */ + LeaveCriticalSection(&Resource->CriticalSection); + Sleep(1); + continue; + } } + /* Owned shared - add ourselves */ + Resource->ActiveCount++; + LeaveCriticalSection(&Resource->CriticalSection); + return TRUE; } - // It's owned shared, so we can add ourselves as another shared owner else { - Resource->ActiveCount++; - Result = TRUE; + /* Resource is free - take it shared */ + Resource->ActiveCount = 1; + LeaveCriticalSection(&Resource->CriticalSection); + return TRUE; } } - else { - // Resource is not owned, so we can take it shared - Resource->ActiveCount = 1; - Result = TRUE; - } - - // Always leave the critical section when acquiring shared - LeaveCriticalSection(&Resource->CriticalSection); - - return Result; } VOID diff --git a/tests/test_file_io.cpp b/tests/test_file_io.cpp index 39d7d56..5e66503 100644 --- a/tests/test_file_io.cpp +++ b/tests/test_file_io.cpp @@ -75,7 +75,7 @@ TEST_F(FileIOTest, ZwCreateFile_ValidParameters) { EXPECT_NE(fileHandle, nullptr); EXPECT_NE(fileHandle, INVALID_HANDLE_VALUE); EXPECT_EQ(ioStatus.Status, STATUS_SUCCESS); - EXPECT_EQ(ioStatus.Information, FILE_OPENED); + EXPECT_EQ(ioStatus.Information, FILE_CREATED); if (fileHandle && fileHandle != INVALID_HANDLE_VALUE) { CloseHandle(fileHandle); @@ -143,7 +143,7 @@ TEST_F(FileIOTest, ZwCreateFile_FileNotFound) { ); EXPECT_EQ(status, STATUS_OBJECT_NAME_NOT_FOUND); - EXPECT_EQ(ioStatus.Status, STATUS_UNSUCCESSFUL); + EXPECT_EQ(ioStatus.Status, STATUS_OBJECT_NAME_NOT_FOUND); } TEST_F(FileIOTest, ZwCreateFile_InvalidParameters) { diff --git a/tests/test_zwdeletekey.cpp b/tests/test_zwdeletekey.cpp index b5ba6c7..3af8e2b 100644 --- a/tests/test_zwdeletekey.cpp +++ b/tests/test_zwdeletekey.cpp @@ -102,11 +102,8 @@ TEST_F(ZwDeleteKeyTest, FailsToDeleteNonEmptyKey) { DEBUG_INFO(DEBUG_COMPONENT_REGISTRY, "ZwDeleteKey on non-empty key returned status: 0x%08X", status); - // The status 0x5 is ERROR_ACCESS_DENIED (Windows error code) - // We need to check for both NTSTATUS and Windows error codes - bool deletionFailed = ( - status == ERROR_ACCESS_DENIED - ); // Any failure status + // ZwDeleteKey now returns proper NTSTATUS codes + bool deletionFailed = !NT_SUCCESS(status); if (status == STATUS_SUCCESS) { // If it succeeded, this indicates a limitation of the user-mode simulation