[ros-kernel] ExAllocatePool
Vizzini
vizzini at plasmic.com
Mon Feb 23 18:48:28 CET 2004
On Sun, 2004-02-22 at 06:32, Filip Navara wrote:
> As Waldo Alvarez Cañizares pointed in one of his emails, the
> ExAllocatePool function shouldn't zero out the memory returned. I tried
> to fix it, but it's very hard to guess where it's really needed to zero
> the memory or not. With the attached patch it's possible to boot and
> work in the GUI, but obviously some places were ommited and it can cause
> weird crashes (although I haven't seen any for a long time).
I'm curious about what crashes you were seeing. As far as I can tell,
the crashes would only be due to code that *depends* on getting
pre-zeroed kernel-mode memory? We had a discussion about this on IRC,
and the consensus was that the kernel's heap management routines don't
(or shouldn't) guarantee zeroed memory (although they may hand out
zeroed memory early on, etc).
Then again, I'm sure we have lots of buffer management bugs that happen
to be masked because they are writing into zeroed memory --> CRT string
library bugs could be masked by this.
> I'm sending
> it here for verification and asking the kernel developers to review it.
> We really need to fix this bug as soon as possible, otherwise it will be
> even bigger pain to fix it then now.
The only thing that looked fishy was ObCreateObject, but there are some
other comments in-line. Others with knowledge in these areas should
double-check me where I commented, and there were a few cases where I
just don't have enough familiarity to even suggest anything
intelligently.
My thoughts:
> diff -r -u -w reactos/ntoskrnl/io/buildirp.c reactos/ntoskrnl/io/buildirp.c
> --- reactos/ntoskrnl/io/buildirp.c Sat Feb 14 17:34:12 2004
> +++ reactos/ntoskrnl/io/buildirp.c Sun Feb 22 09:42:16 2004
> @@ -259,6 +259,13 @@
> RtlCopyMemory(Irp->AssociatedIrp.SystemBuffer,
> InputBuffer,
> InputBufferLength);
> + RtlZeroMemory(Irp->AssociatedIrp.SystemBuffer + InputBufferLength,
> + BufferLength - InputBufferLength);
> + }
> + else
> + {
> + RtlZeroMemory(Irp->AssociatedIrp.SystemBuffer,
> + BufferLength);
> }
> Irp->UserBuffer = OutputBuffer;
> break;
This is saying that the input buffer for METHOD_BUFFERED IOCTLs is not
pre-zeroed, meaning that the driver will get an input buffer that could
have garbage following the valid data. The output buffer would also not
be zeroed (it's the same buffer). Neither of these situations seem
problematic to me.
> @@ -273,12 +280,13 @@
> ExAllocatePoolWithTag(NonPagedPool,OutputBufferLength,
> TAG_SYS_BUF);
>
> -
> if (Irp->AssociatedIrp.SystemBuffer == NULL)
> {
> IoFreeIrp(Irp);
> return(NULL);
> }
> +
> + RtlZeroMemory(Irp->AssociatedIrp.SystemBuffer, OutputBufferLength);
> Irp->UserBuffer = OutputBuffer;
> }
Same basic idea, although I'm not sure the surrounding code is obviously
correct. It may be right; I just need to think about it some more.
Regardless, your change shouldn't be bad.
> diff -r -u -w reactos/ntoskrnl/io/irp.c reactos/ntoskrnl/io/irp.c
> --- reactos/ntoskrnl/io/irp.c Sat Feb 14 17:34:12 2004
> +++ reactos/ntoskrnl/io/irp.c Sun Feb 22 09:42:16 2004
> @@ -200,6 +200,7 @@
> return(NULL);
> }
>
> + RtlZeroMemory(Irp, IoSizeOfIrp(StackSize));
> IoInitializeIrp(Irp,
> IoSizeOfIrp(StackSize),
> StackSize);
This is definitely not a problem, as long as IoInitializeIrp does its
job.
> diff -r -u -w reactos/ntoskrnl/io/mdl.c reactos/ntoskrnl/io/mdl.c
> --- reactos/ntoskrnl/io/mdl.c Sat Feb 14 17:34:12 2004
> +++ reactos/ntoskrnl/io/mdl.c Sun Feb 22 09:42:16 2004
> @@ -49,6 +49,7 @@
> MmSizeOfMdl(VirtualAddress,Length),
> TAG_MDL);
> }
> + RtlZeroMemory(Mdl, MmSizeOfMdl(VirtualAddress,Length));
> MmInitializeMdl(Mdl, (char*)VirtualAddress, Length);
> if (Irp!=NULL && !SecondaryBuffer)
> {
Same deal as above. OK with me.
> diff -r -u -w reactos/ntoskrnl/mm/aspace.c reactos/ntoskrnl/mm/aspace.c
> --- reactos/ntoskrnl/mm/aspace.c Sat Feb 14 17:34:12 2004
> +++ reactos/ntoskrnl/mm/aspace.c Sun Feb 22 09:42:16 2004
> @@ -88,6 +88,7 @@
> AddressSpace->PageTableRefCountTable =
> ExAllocatePoolWithTag(NonPagedPool, 768 * sizeof(USHORT),
> TAG_PTRC);
> + RtlZeroMemory(AddressSpace->PageTableRefCountTable, 768 * sizeof(USHORT));
> AddressSpace->PageTableRefCountTableSize = 768;
> }
> else
I can't really comment on this one without thinking about it a lot
more. Anyone else?
> diff -r -u -w reactos/ntoskrnl/mm/freelist.c reactos/ntoskrnl/mm/freelist.c
> --- reactos/ntoskrnl/mm/freelist.c Sat Feb 14 17:34:14 2004
> +++ reactos/ntoskrnl/mm/freelist.c Sun Feb 22 09:42:16 2004
> @@ -718,7 +718,7 @@
> {
> ULONG Start = PhysicalAddress.u.LowPart / PAGE_SIZE;
>
> - DPRINT("MmGetReferenceCountPage(PhysicalAddress %x)\n", PhysicalAddress);
> + DPRINT("MmIsUsablePage(PhysicalAddress %x)\n", PhysicalAddress);
>
> if (PhysicalAddress.u.LowPart == 0)
> {
Color me curious --> this looks like an incorrect change (it's in
MmIsUsablePage()).
> diff -r -u -w reactos/ntoskrnl/mm/npool.c reactos/ntoskrnl/mm/npool.c
> --- reactos/ntoskrnl/mm/npool.c Sat Feb 14 17:34:16 2004
> +++ reactos/ntoskrnl/mm/npool.c Sun Feb 22 09:42:16 2004
> @@ -1633,7 +1633,7 @@
> #endif
> KeReleaseSpinLock(&MmNpoolLock, oldIrql);
> block = block_to_address(best);
> - memset(block,0,Size);
> + /* RtlZeroMemory(block, Size); */
> return(block);
> #endif /* WHOLE_PAGE_ALLOCATIONS */
> }
This just changes ExAllocateNonPagedPoolWithTag() so that it doesn't
zero blocks pre-return. That is, after all, the point of this exercise,
and I think it's fine.
> diff -r -u -w reactos/ntoskrnl/mm/pagefile.c reactos/ntoskrnl/mm/pagefile.c
> --- reactos/ntoskrnl/mm/pagefile.c Sat Feb 14 17:34:18 2004
> +++ reactos/ntoskrnl/mm/pagefile.c Sun Feb 22 09:42:16 2004
> @@ -811,6 +811,8 @@
> return(STATUS_NO_MEMORY);
> }
>
> + RtlZeroMemory(CurrentRetDescList, Size);
> +
> #if defined(__GNUC__)
> Vcn.QuadPart = 0LL;
> #else
Looks OK to me, but I'm not very familiar with this area of the code.
Anyone else?
> @@ -858,6 +860,7 @@
> NtClose(FileHandle);
> return(STATUS_NO_MEMORY);
> }
> + RtlZeroMemory(CurrentRetDescList->Next, Size);
> Vcn.QuadPart = CurrentRetDescList->RetrievalPointers.Pair[CurrentRetDescList->RetrievalPointers.NumberOfPairs-1].Vcn;
> CurrentRetDescList = CurrentRetDescList->Next;
> }
Ditto.
> @@ -881,6 +884,8 @@
> return(STATUS_NO_MEMORY);
> }
>
> + RtlZeroMemory(PagingFile, sizeof(*PagingFile));
> +
> PagingFile->FileObject = FileObject;
> PagingFile->MaximumSize.QuadPart = MaximumSize->QuadPart;
> PagingFile->CurrentSize.QuadPart = InitialSize->QuadPart;
Ditto.
> @@ -924,6 +929,9 @@
> return(STATUS_NO_MEMORY);
> }
>
> + RtlZeroMemory(PagingFile->AllocMap, AllocMapSize * sizeof(ULONG));
> + RtlZeroMemory(PagingFile->RetrievalPointers, Size);
> +
> Count = 0;
> PagingFile->RetrievalPointers->NumberOfPairs = ExtentCount;
> PagingFile->RetrievalPointers->StartVcn = RetDescList->RetrievalPointers.StartVcn;
Ditto.
> diff -r -u -w reactos/ntoskrnl/mm/pool.c reactos/ntoskrnl/mm/pool.c
> --- reactos/ntoskrnl/mm/pool.c Sat Feb 14 17:34:18 2004
> +++ reactos/ntoskrnl/mm/pool.c Sun Feb 22 09:42:16 2004
> @@ -23,11 +23,7 @@
>
> /* FUNCTIONS ***************************************************************/
>
> -#if defined(__GNUC__)
> -PVOID STDCALL STATIC
> -#else
> STATIC PVOID STDCALL
> -#endif
> EiAllocatePool(POOL_TYPE PoolType,
> ULONG NumberOfBytes,
> ULONG Tag,
I assume this is a left-over from your STDCALL fixing?
> @@ -35,7 +31,6 @@
> {
> PVOID Block;
>
> -
> switch(PoolType)
> {
> case NonPagedPool:
Whitespace only.
> diff -r -u -w reactos/ntoskrnl/mm/ppool.c reactos/ntoskrnl/mm/ppool.c
> --- reactos/ntoskrnl/mm/ppool.c Sun Feb 15 22:12:38 2004
> +++ reactos/ntoskrnl/mm/ppool.c Sun Feb 22 09:42:16 2004
> @@ -406,8 +406,7 @@
> ExReleaseFastMutex(&MmPagedPoolLock);
>
> BlockAddress = block_to_address ( NewBlock );
> -
> - memset(BlockAddress, 0, NumberOfBytes);
> +/* RtlZeroMemory(BlockAddress, NumberOfBytes);*/
>
> #if MM_PPOOL_REDZONE_BYTES
> NewBlock->UserSize = NumberOfBytes;
Looks like you replaced a RtlZerlMemory with a memset(x,0,x)? Did you
mean to do that?
> diff -r -u -w reactos/ntoskrnl/mm/region.c reactos/ntoskrnl/mm/region.c
> --- reactos/ntoskrnl/mm/region.c Sat Feb 14 17:34:22 2004
> +++ reactos/ntoskrnl/mm/region.c Sun Feb 22 09:42:16 2004
> @@ -84,6 +84,10 @@
> ExFreePool(NewRegion2);
> return(NULL);
> }
> +
> + RtlZeroMemory(NewRegion1, sizeof(MM_REGION));
> + RtlZeroMemory(NewRegion2, sizeof(MM_REGION));
> +
> NewRegion1->Type = NewType;
> NewRegion1->Protect = NewProtect;
> InternalLength = ((char*)InitialBaseAddress + InitialRegion->Length) - (char*)StartAddress;
Seems reasonable.
> @@ -276,6 +280,7 @@
>
> Region = ExAllocatePoolWithTag(NonPagedPool, sizeof(MM_REGION),
> TAG_MM_REGION);
> + RtlZeroMemory(Region, sizeof(MM_REGION));
> Region->Type = Type;
> Region->Protect = Protect;
> Region->Length = Length;
Same deal. I don't think regions have semantics that dictate being zero
initially. Anyone else?
> diff -r -u -w reactos/ntoskrnl/mm/section.c reactos/ntoskrnl/mm/section.c
> --- reactos/ntoskrnl/mm/section.c Sat Feb 14 17:34:32 2004
> +++ reactos/ntoskrnl/mm/section.c Sun Feb 22 09:42:16 2004
> @@ -2322,6 +2322,7 @@
> ObDereferenceObject(Section);
> return(STATUS_NO_MEMORY);
> }
> + RtlZeroMemory(Segment, sizeof(MM_SECTION_SEGMENT));
> Section->Segment = Segment;
> Segment->ReferenceCount = 1;
> ExInitializeFastMutex(&Segment->Lock);
This just zero-initializes a page file section. I can't imagine that
should matter, outside of bugs.
> @@ -2539,6 +2540,7 @@
> ObDereferenceObject(FileObject);
> return(STATUS_NO_MEMORY);
> }
> + RtlZeroMemory(Segment, sizeof(MM_SECTION_SEGMENT));
> Section->Segment = Segment;
> Segment->ReferenceCount = 1;
> ExInitializeFastMutex(&Segment->Lock);
Similar deal.
> @@ -2865,6 +2867,7 @@
> ExFreePool(ImageSections);
> return(STATUS_NO_MEMORY);
> }
> + RtlZeroMemory(ImageSectionObject, Size);
> Section->ImageSection = ImageSectionObject;
> ImageSectionObject->NrSegments = NrSegments;
> ImageSectionObject->ImageBase = (PVOID)PEHeader.OptionalHeader.ImageBase;
Ditto.
> diff -r -u -w reactos/ntoskrnl/mm/slab.c reactos/ntoskrnl/mm/slab.c
> --- reactos/ntoskrnl/mm/slab.c Sat Feb 14 17:34:32 2004
> +++ reactos/ntoskrnl/mm/slab.c Sun Feb 22 09:42:16 2004
> @@ -84,6 +84,7 @@
> {
> return(NULL);
> }
> + RtlZeroMemory(Slab, sizeof(SLAB_CACHE));
>
> Slab->Constructor = Constructor;
> Slab->Destructor = Destructor;
Not really familiar with this section of code. Unless there are
zero-init semantics associated with slab caches, this should be fine.
> diff -r -u -w reactos/ntoskrnl/ob/object.c reactos/ntoskrnl/ob/object.c
> --- reactos/ntoskrnl/ob/object.c Sat Feb 14 17:34:34 2004
> +++ reactos/ntoskrnl/ob/object.c Sun Feb 22 09:42:16 2004
> @@ -377,6 +377,7 @@
> Type->Tag);
> if (Header == NULL)
> return STATUS_INSUFFICIENT_RESOURCES;
> + RtlZeroMemory(Header, OBJECT_ALLOC_SIZE(ObjectSize));
>
> /* Initialize the object header */
> Header->HandleCount = 0;
The way these things are spaced in the code, it almost looks like
someone went through and intentionally added these things after the
fact... Anyway, it seems like Header->CloseInProgress should be
initialized to FALSE. The Header->Entry, Header->Parent, and
Header->SecurityDescriptor should be initialized somewhere, but I
haven't looked through the code to see if they are somewhere else.
> diff -r -u reactos/subsys/win32k/objects/text.c reactos/subsys/win32k/objects/text.c
> --- reactos/subsys/win32k/objects/text.c Sun Feb 22 08:35:22 2004
> +++ reactos/subsys/win32k/objects/text.c Sun Feb 22 11:06:48 2004
> @@ -283,10 +283,11 @@
> {
> iFileData = NULL;
> pBuff = ExAllocatePool(NonPagedPool,0x4000);
> + RtlZeroMemory(pBuff, 0x4000);
> RtlInitUnicodeString(&cchFilename,0);
> cchFilename.MaximumLength = 0x1000;
> cchFilename.Buffer = ExAllocatePoolWithTag(PagedPool,cchFilename.MaximumLength, TAG_STRING);
> -
> + RtlZeroMemory(cchFilename.Buffer, cchFilename.MaximumLength);
> cchFilename.Length = 0;
>
> Status = NtQueryDirectoryFile( hDirectory,
Definitely Ge's thing more than mine. Looks OK to me though.
-Vizzini
More information about the Ros-kernel
mailing list