[ion] 52581: [NTDLL]: Fix multiple levels of fail in LdrGetDllHandleEx (returning with lock held, leaking memory, using NULL pointers, using magic flags, missing ASSERTs, buffer overflows, usage of...
Author: ion Date: Sat Jul 9 13:37:47 2011 New Revision: 52581
URL: http://svn.reactos.org/svn/reactos?rev=52581&view=rev Log: [NTDLL]: Fix multiple levels of fail in LdrGetDllHandleEx (returning with lock held, leaking memory, using NULL pointers, using magic flags, missing ASSERTs, buffer overflows, usage of incorrect constants, ignoring status failures, lack of varaible initialization, incorrect string initialization).
Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c trunk/reactos/include/ndk/ldrtypes.h trunk/reactos/include/psdk/ntdef.h trunk/reactos/include/psdk/winnt.h
Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/ldr/ldrapi.c?rev=... ============================================================================== --- trunk/reactos/dll/ntdll/ldr/ldrapi.c [iso-8859-1] (original) +++ trunk/reactos/dll/ntdll/ldr/ldrapi.c [iso-8859-1] Sat Jul 9 13:37:47 2011 @@ -417,31 +417,36 @@ IN PUNICODE_STRING DllName, OUT PVOID *DllHandle OPTIONAL) { - NTSTATUS Status = STATUS_DLL_NOT_FOUND; + NTSTATUS Status; PLDR_DATA_TABLE_ENTRY LdrEntry; - UNICODE_STRING RedirectName, DllString1; - UNICODE_STRING RawDllName; - PUNICODE_STRING pRedirectName = &RedirectName; - PUNICODE_STRING CompareName; + UNICODE_STRING RedirectName, DllString1, RawDllName; + PUNICODE_STRING pRedirectName, CompareName; PWCHAR p1, p2, p3; - BOOLEAN Locked = FALSE; - BOOLEAN RedirectedDll = FALSE; - ULONG Cookie; - ULONG LoadFlag; + BOOLEAN Locked, RedirectedDll; + ULONG_PTR Cookie; + ULONG LoadFlag, Length;
/* Initialize the strings */ - RtlInitUnicodeString(&DllString1, NULL); - RtlInitUnicodeString(&RawDllName, NULL); + RtlInitEmptyUnicodeString(&DllString1, NULL, 0); + RtlInitEmptyUnicodeString(&RawDllName, NULL, 0); RedirectName = *DllName; + pRedirectName = &RedirectName; + + /* Initialize state */ + RedirectedDll = Locked = FALSE; + LdrEntry = NULL; + Cookie = 0;
/* Clear the handle */ if (DllHandle) *DllHandle = NULL;
- /* Check for a valid flag */ - if ((Flags & ~3) || (!DllHandle && !(Flags & 2))) + /* Check for a valid flag combination */ + if ((Flags & ~(LDR_GET_DLL_HANDLE_EX_PIN | LDR_GET_DLL_HANDLE_EX_UNCHANGED_REFCOUNT)) || + (!DllHandle && !(Flags & LDR_GET_DLL_HANDLE_EX_PIN))) { DPRINT1("Flags are invalid or no DllHandle given\n"); - return STATUS_INVALID_PARAMETER; + Status = STATUS_INVALID_PARAMETER; + goto Quickie; }
/* If not initializing */ @@ -449,6 +454,9 @@ { /* Acquire the lock */ Status = LdrLockLoaderLock(0, NULL, &Cookie); + if (!NT_SUCCESS(Status)) goto Quickie; + + /* Remember we own it */ Locked = TRUE; }
@@ -474,6 +482,9 @@ /* Unrecoverable SxS failure; */ goto Quickie; } + + /* Set default failure code */ + Status = STATUS_DLL_NOT_FOUND;
/* Use the cache if we can */ if (LdrpGetModuleHandleCache) @@ -516,17 +527,15 @@
/* Return success */ Status = STATUS_SUCCESS; - - goto FoundEntry; + goto Quickie; } }
DontCompare: /* Find the name without the extension */ p1 = pRedirectName->Buffer; + p2 = NULL; p3 = &p1[pRedirectName->Length / sizeof(WCHAR)]; -StartLoop: - p2 = NULL; while (p1 != p3) { if (*p1++ == L'.') @@ -535,26 +544,35 @@ } else if (*p1 == L'\') { - goto StartLoop; + p2 = NULL; } }
/* Check if no extension was found or if we got a slash */ - if (!p2 || *p2 == L'\' || *p2 == L'/') + if (!(p2) || (*p2 == L'\') || (*p2 == L'/')) { /* Check that we have space to add one */ - if (pRedirectName->Length + LdrApiDefaultExtension.Length >= MAXLONG) + Length = pRedirectName->Length + + LdrApiDefaultExtension.Length + sizeof(UNICODE_NULL); + if (Length >= UNICODE_STRING_MAX_BYTES) { /* No space to add the extension */ - return STATUS_NAME_TOO_LONG; + Status = STATUS_NAME_TOO_LONG; + goto Quickie; }
/* Setup the string */ - RawDllName.MaximumLength = pRedirectName->Length + LdrApiDefaultExtension.Length + sizeof(UNICODE_NULL); + RawDllName.MaximumLength = Length; + ASSERT(Length >= sizeof(UNICODE_NULL)); RawDllName.Length = RawDllName.MaximumLength - sizeof(UNICODE_NULL); RawDllName.Buffer = RtlAllocateHeap(RtlGetProcessHeap(), 0, RawDllName.MaximumLength); + if (!RawDllName.Buffer) + { + Status = STATUS_NO_MEMORY; + goto Quickie; + }
/* Copy the buffer */ RtlMoveMemory(RawDllName.Buffer, @@ -572,11 +590,11 @@ else { /* Check if there's something in the name */ - if (pRedirectName->Length) + Length = pRedirectName->Length; + if (Length) { /* Check and remove trailing period */ - if (pRedirectName->Buffer[(pRedirectName->Length - 2) / - sizeof(WCHAR)] == '.') + if (pRedirectName->Buffer[Length / sizeof(WCHAR) - sizeof(ANSI_NULL)] == '.') { /* Decrease the size */ pRedirectName->Length -= sizeof(WCHAR); @@ -589,6 +607,11 @@ RawDllName.Buffer = RtlAllocateHeap(RtlGetProcessHeap(), 0, RawDllName.MaximumLength); + if (!RawDllName.Buffer) + { + Status = STATUS_NO_MEMORY; + goto Quickie; + }
/* Copy the buffer */ RtlMoveMemory(RawDllName.Buffer, @@ -625,46 +648,41 @@ /* Make sure to NULL this */ LdrEntry = NULL; } -FoundEntry: +Quickie: + /* The success path must have a valid loader entry */ + ASSERT((LdrEntry != NULL) == NT_SUCCESS(Status)); + + /* Check if we got an entry and success */ DPRINT("Got LdrEntry->BaseDllName %wZ\n", LdrEntry ? &LdrEntry->BaseDllName : NULL); - - /* Check if we got an entry */ - if (LdrEntry) - { - /* Check for success */ - if (NT_SUCCESS(Status)) - { - /* Check if the DLL is locked */ - if (LdrEntry->LoadCount != -1) - { - /* Check what flag we got */ - if (!(Flags & 1)) - { - /* Check what to do with the load count */ - if (Flags & 2) - { - /* Pin it */ - LdrEntry->LoadCount = -1; - LoadFlag = LDRP_UPDATE_PIN; - } - else - { - /* Increase the load count */ - LdrEntry->LoadCount++; - LoadFlag = LDRP_UPDATE_REFCOUNT; - } - - /* Update the load count now */ - LdrpUpdateLoadCount2(LdrEntry, LoadFlag); - LdrpClearLoadInProgress(); - } - } - - /* Check if the caller is requesting the handle */ - if (DllHandle) *DllHandle = LdrEntry->DllBase; - } - } -Quickie: + if ((LdrEntry) && (NT_SUCCESS(Status))) + { + /* Check if the DLL is locked */ + if ((LdrEntry->LoadCount != 0xFFFF) && + !(Flags & LDR_GET_DLL_HANDLE_EX_UNCHANGED_REFCOUNT)) + { + /* Check what to do with the load count */ + if (Flags & LDR_GET_DLL_HANDLE_EX_PIN) + { + /* Pin it */ + LdrEntry->LoadCount = 0xFFFF; + LoadFlag = LDRP_UPDATE_PIN; + } + else + { + /* Increase the load count */ + LdrEntry->LoadCount++; + LoadFlag = LDRP_UPDATE_REFCOUNT; + } + + /* Update the load count now */ + LdrpUpdateLoadCount2(LdrEntry, LoadFlag); + LdrpClearLoadInProgress(); + } + + /* Check if the caller is requesting the handle */ + if (DllHandle) *DllHandle = LdrEntry->DllBase; + } + /* Free string if needed */ if (DllString1.Buffer) RtlFreeUnicodeString(&DllString1);
@@ -672,12 +690,16 @@ if (RawDllName.Buffer) { /* Free the heap-allocated buffer */ - RtlFreeHeap(RtlGetProcessHeap(), 0, RawDllName.Buffer); + Status = RtlFreeHeap(RtlGetProcessHeap(), 0, RawDllName.Buffer); RawDllName.Buffer = NULL; }
/* Release lock */ - if (Locked) LdrUnlockLoaderLock(LDR_LOCK_LOADER_LOCK_FLAG_RAISE_ON_ERRORS, Cookie); + if (Locked) + { + Status = LdrUnlockLoaderLock(LDR_LOCK_LOADER_LOCK_FLAG_RAISE_ON_ERRORS, + Cookie); + }
/* Return */ return Status;
Modified: trunk/reactos/include/ndk/ldrtypes.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/ndk/ldrtypes.h?rev=... ============================================================================== --- trunk/reactos/include/ndk/ldrtypes.h [iso-8859-1] (original) +++ trunk/reactos/include/ndk/ldrtypes.h [iso-8859-1] Sat Jul 9 13:37:47 2011 @@ -61,23 +61,29 @@ // // Dll Characteristics for LdrLoadDll // -#define LDR_IGNORE_CODE_AUTHZ_LEVEL 0x00001000 +#define LDR_IGNORE_CODE_AUTHZ_LEVEL 0x00001000
// // LdrAddRef Flags // -#define LDR_PIN_MODULE 0x00000001 +#define LDR_PIN_MODULE 0x00000001
// // LdrLockLoaderLock Flags // -#define LDR_LOCK_LOADER_LOCK_FLAG_RAISE_ON_ERRORS 0x00000001 -#define LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY 0x00000002 +#define LDR_LOCK_LOADER_LOCK_FLAG_RAISE_ON_ERRORS 0x00000001 +#define LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY 0x00000002
// // LdrUnlockLoaderLock Flags // #define LDR_UNLOCK_LOADER_LOCK_FLAG_RAISE_ON_ERRORS 0x00000001 + +// +// LdrGetDllHandleEx Flags +// +#define LDR_GET_DLL_HANDLE_EX_UNCHANGED_REFCOUNT 0x00000001 +#define LDR_GET_DLL_HANDLE_EX_PIN 0x00000002
#define LDR_LOCK_LOADER_LOCK_DISPOSITION_INVALID 0 #define LDR_LOCK_LOADER_LOCK_DISPOSITION_LOCK_ACQUIRED 1
Modified: trunk/reactos/include/psdk/ntdef.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/psdk/ntdef.h?rev=52... ============================================================================== --- trunk/reactos/include/psdk/ntdef.h [iso-8859-1] (original) +++ trunk/reactos/include/psdk/ntdef.h [iso-8859-1] Sat Jul 9 13:37:47 2011 @@ -460,6 +460,8 @@ } UNICODE_STRING, *PUNICODE_STRING; typedef const UNICODE_STRING* PCUNICODE_STRING; #define UNICODE_NULL ((WCHAR)0) +#define UNICODE_STRING_MAX_BYTES ((USHORT) 65534) +#define UNICODE_STRING_MAX_CHARS (32767)
typedef struct _CSTRING { USHORT Length;
Modified: trunk/reactos/include/psdk/winnt.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/include/psdk/winnt.h?rev=52... ============================================================================== --- trunk/reactos/include/psdk/winnt.h [iso-8859-1] (original) +++ trunk/reactos/include/psdk/winnt.h [iso-8859-1] Sat Jul 9 13:37:47 2011 @@ -284,6 +284,8 @@ #endif #define ANSI_NULL ((CHAR)0) #define UNICODE_NULL ((WCHAR)0) +#define UNICODE_STRING_MAX_BYTES ((USHORT) 65534) +#define UNICODE_STRING_MAX_CHARS (32767) typedef BYTE BOOLEAN,*PBOOLEAN; #endif typedef BYTE FCHAR;
-
ion＠svn.reactos.org