[ros-kernel] FIXES FIXES FIXES Lots of fixes
Waldo Alvarez Cañizares
wac at lab.matcom.uh.cu
Fri Feb 13 18:09:59 CET 2004
Hello Guys:
I could reproduce the bug extracting one bitmap from Winrar resources and displaying it with the strechtblt test. It fixes Winrar wrecked bitmaps read further.
I Fixed this to prevent it from allocating block with size 0 from Nonpaged pool
in surface.c
HBITMAP STDCALL
EngCreateBitmap(IN SIZEL Size,
IN LONG Width,
IN ULONG Format,
IN ULONG Flags,
IN PVOID Bits)
{
HBITMAP NewBitmap;
SURFOBJ *SurfObj;
SURFGDI *SurfGDI;
PVOID UncompressedBits;
ULONG UncompressedFormat;
NewBitmap = (PVOID)CreateGDIHandle(sizeof(SURFGDI), sizeof(SURFOBJ));
if( !ValidEngHandle( NewBitmap ) ) return 0;
SurfObj = (SURFOBJ*) AccessUserObject( (ULONG) NewBitmap );
SurfGDI = (SURFGDI*) AccessInternalObject( (ULONG) NewBitmap );
ASSERT( SurfObj );
ASSERT( SurfGDI );
SurfGDI->BitsPerPixel = BitsPerFormat(Format);
if (BMF_4RLE == Format)
{
SurfObj->lDelta = DIB_GetDIBWidthBytes(Size.cx, BitsPerFormat(BMF_4BPP));
SurfObj->cjBits = SurfObj->lDelta * Size.cy;
UncompressedFormat = BMF_4BPP;
UncompressedBits = EngAllocMem(FL_ZERO_MEMORY, SurfObj->cjBits, 0);
Decompress4bpp(Size, (BYTE *)Bits, (BYTE *)UncompressedBits, SurfObj->lDelta);
}
else
{
if (BMF_8RLE == Format)
{
SurfObj->lDelta = DIB_GetDIBWidthBytes(Size.cx, BitsPerFormat(BMF_8BPP));
SurfObj->cjBits = SurfObj->lDelta * Size.cy;
UncompressedFormat = BMF_8BPP;
UncompressedBits = EngAllocMem(FL_ZERO_MEMORY, SurfObj->cjBits, 0);
Decompress8bpp(Size, (BYTE *)Bits, (BYTE *)UncompressedBits, SurfObj->lDelta);
}
else
{
SurfObj->lDelta = Width;
SurfObj->cjBits = SurfObj->lDelta * Size.cy;
UncompressedBits = Bits;
UncompressedFormat = Format;
}
}
if(NULL != UncompressedBits)
{
SurfObj->pvBits = UncompressedBits;
}
else
{
if (0 == SurfObj->cjBits) <---
{ |
SurfObj->pvBits = NULL; |--- Add this
} |
else |
{ <---
if(Flags & BMF_USERMEM)
{
SurfObj->pvBits = EngAllocUserMem(SurfObj->cjBits, 0);
}
else
{
if(Flags & BMF_NOZEROINIT)
{
SurfObj->pvBits = EngAllocMem(0, SurfObj->cjBits, 0);
}
else
{
SurfObj->pvBits = EngAllocMem(FL_ZERO_MEMORY, SurfObj->cjBits, 0);
}
}
} <---- And this of course
}
SurfObj->dhsurf = 0; // device managed surface
SurfObj->hsurf = 0;
SurfObj->sizlBitmap = Size;
SurfObj->iBitmapFormat = UncompressedFormat;
SurfObj->iType = STYPE_BITMAP;
SurfObj->fjBitmap = Flags & (BMF_TOPDOWN | BMF_NOZEROINIT);
SurfObj->pvScan0 = SurfObj->pvBits;
InitializeFuncs(SurfGDI, UncompressedFormat);
// Use flags to determine bitmap type -- TOP_DOWN or whatever
return NewBitmap;
}
This fixes problems with wrecked bitmaps for me. The problem was that the number in
IntPart wasn't sometimes a multiple of srcPitch and when copying from the src
bitmap the address was some bytes before.
in subsys\win32k\dib\dib16bpp.c Check that!!
//NOTE: If you change something here, please do the same in other dibXXbpp.c files!
void ScaleRectAvg16(PIXEL *Target, PIXEL *Source, int SrcWidth, int SrcHeight,
int TgtWidth, int TgtHeight, int srcPitch, int dstPitch)
{
int NumPixels = TgtHeight;
int FractPart = (SrcHeight) % TgtHeight;
int Mid = TgtHeight / 2;
int E = 0;
int skip;
int IntPart = ((SrcHeight / TgtHeight) * (SrcWidth + 1)); <------------------ change here by something equivalent to: IntPart = ((SrcHeight / TgtHeight) * srcPitch) / 2;
(this one works but maybe is not the best way)
PIXEL *ScanLine, *ScanLineAhead;
PIXEL *PrevSource = NULL;
PIXEL *PrevSourceAhead = NULL;
skip = (TgtHeight < SrcHeight) ? 0 : (TgtHeight / (2*SrcHeight) + 1);
NumPixels -= skip;
ScanLine = (PIXEL*)ExAllocatePool(NonPagedPool, TgtWidth*sizeof(PIXEL)); // FIXME: Should we use PagedPool here?
ScanLineAhead = (PIXEL *)ExAllocatePool(NonPagedPool, TgtWidth*sizeof(PIXEL));
while (NumPixels-- > 0) {
...
Notice that there is no need to allocate extra memory from the pool. At least for ScanLine and maybe for both.
The memory for the target could be used for that directly. The memcpy
is not needed either. That could give us some speed and that way it doesn't matters
if the destination bitmap goes to the paged/nonpaged pool too.
Notice that some other ScaleRectAvgXXX are probably broken too.
Copying code is proven to be bad. If I choose for example to use an algorithm that gives more
quality than that one I would have to change them all. Maybe a macro or a function call expecting the
size of one pixel. Image quality is something users appreciate. I'm not saying we
should change those functions but maybe someone feels unconftable with that scaling algo.
And decides to use another at expense of some speed.
I'm getting a pagefault from here when the process terminates with an unhandled exception
(ROS 0.2.0):
PsTerminateCurrentThread(NTSTATUS ExitStatus)
/*
* FUNCTION: Terminates the current thread
*/
{
KIRQL oldIrql;
PETHREAD CurrentThread;
PLIST_ENTRY current_entry;
PKMUTANT Mutant;
BOOLEAN Last;
PEPROCESS CurrentProcess;
SIZE_T Length = PAGE_SIZE;
KeLowerIrql(PASSIVE_LEVEL);
CurrentThread = PsGetCurrentThread();
CurrentProcess = CurrentThread->ThreadsProcess;
KeAcquireSpinLock(&PiThreadListLock, &oldIrql);
DPRINT("terminating %x\n",CurrentThread);
CurrentThread->ExitStatus = ExitStatus;
KeCancelTimer(&CurrentThread->Tcb.Timer);
/* Remove the thread from the thread list of its process */
RemoveEntryList(&CurrentThread->Tcb.ProcessThreadListEntry); <------- Pagefault from Here!!
Last = IsListEmpty(&CurrentProcess->ThreadListHead);
KeReleaseSpinLock(&PiThreadListLock, oldIrql);
...
There are some chances for this to happen
- The list is destroyed somewhere else when elements are added or removed
or some function ovrewriting it, the later could be hard to track.
- Bad Sincronization
Who is changing the structure?:
NTSTATUS
PsInitializeThread(HANDLE ProcessHandle,
PETHREAD* ThreadPtr,
PHANDLE ThreadHandle,
ACCESS_MASK DesiredAccess,
POBJECT_ATTRIBUTES ThreadAttributes,
BOOLEAN First)
{
PETHREAD Thread;
NTSTATUS Status;
KIRQL oldIrql;
PEPROCESS Process;
/*
* Reference process
*/
if (ProcessHandle != NULL)
{
Status = ObReferenceObjectByHandle(ProcessHandle,
PROCESS_CREATE_THREAD,
PsProcessType,
UserMode,
(PVOID*)&Process,
NULL);
if (Status != STATUS_SUCCESS)
{
DPRINT("Failed at %s:%d\n",__FILE__,__LINE__);
return(Status);
}
DPRINT( "Creating thread in process %x\n", Process );
}
else
{
Process = PsInitialSystemProcess;
ObReferenceObjectByPointer(Process,
PROCESS_CREATE_THREAD,
PsProcessType,
UserMode);
}
/*
* Create and initialize thread
*/
Status = ObCreateObject(UserMode,
PsThreadType,
ThreadAttributes,
UserMode,
NULL,
sizeof(ETHREAD),
0,
0,
(PVOID*)&Thread);
if (!NT_SUCCESS(Status))
{
return(Status);
}
Status = ObInsertObject ((PVOID)Thread,
NULL,
DesiredAccess,
0,
NULL,
ThreadHandle);
if (!NT_SUCCESS(Status))
{
ObDereferenceObject (Thread);
return Status;
}
DPRINT("Thread = %x\n",Thread);
PiNrThreads++;
KeInitializeThread(&Process->Pcb, &Thread->Tcb, First);
Thread->ThreadsProcess = Process;
/*
* FIXME: What lock protects this? <---------- Here. Wow running wild free!
*/ |
InsertTailList(&Thread->ThreadsProcess->ThreadListHead, <---
&Thread->Tcb.ProcessThreadListEntry);
InitializeListHead(&Thread->TerminationPortList);
KeInitializeSpinLock(&Thread->ActiveTimerListLock);
...
I think is time to lock it if still unlocked in current sources.
KeAcquireSpinLock(&PiThreadListLock, &oldIrql);
should do that, but maybe is better to lock the thread list per process.
I'm not sure about that.
This function:
NtConnectPort in ntoskrnl\lpc\connect.c
is requesting ExFreePool to release a block of memory at address 0 NULL!!
probably to MmCopyToCaller to copy 0 bytes too. The later is slower but correct.
if (!NT_SUCCESS(Status))
{
/* FIXME: Again, check what NT does here. */
if (UnsafeConnectDataLength != NULL)
{
if (ExGetPreviousMode() != KernelMode)
{
if (ConnectData != NULL) <--- I putted this but that's a workaround
{
MmCopyToCaller(UnsafeConnectData,
ConnectData,
ConnectDataLength);
ExFreePool(ConnectData);
}
}
MmCopyToCaller(UnsafeConnectDataLength,
&ConnectDataLength,
sizeof(ULONG));
}
return(Status);
}
that's not the only place in the function where the request is made there are 2 or 3 more places
I also think it would be better to do this in this function
if (KernelMode == ExGetPreviousMode())
{
...
}
else
{
...
MmCopyFromCaller(...);
...
MmCopyToCaller(...);
...
}
instead of constantly checking if the Previous mode was KernelMode.
That makes it clearer and probably increases speed a little (less function calls and less
misspredicted branches), it increases code size too but not much.
I think that the function is very big and that could be splitted into smaller
pieces. Remember the human brain can't keep track of very long things, Torvalds is right
about that. Is a constructive critic.
One last thing, in the nonpaged pool everytime a block is requested it is cleaared. also when you
allocate a page it is cleared and in timer.c you can read
NTSTATUS FASTCALL
InitTimerImpl(VOID)
{
...
HandleLessTimersBitMapBuffer = ExAllocatePool(PagedPool, BitmapBytes);
RtlInitializeBitMap(&HandleLessTimersBitMap,
HandleLessTimersBitMapBuffer,
BitmapBytes * 8);
//yes we need this, since ExAllocatePool isn't supposed to zero out allocated memory
RtlClearAllBits(&HandleLessTimersBitMap);
Status = PsCreateSystemThread(&MsgTimerThreadHandle,
THREAD_ALL_ACCESS,
NULL,
NULL,
...
Actually the memory is cleared 3 TIMES!
one in MmAllocPage
one in ExAllocatePool
and in InitTimerImpl
One time is a waste or a need it depends, two is too much but 3 sorry guys but that
is totally braindead.
I can't remove the memset in ExAllocatePool because somewhere else
it is not cleared and gives me a page fault when booting ROS. Still have to find out
where is that. I think even giving applications memory that contain data remainings from
other applications is still secure (Applications should take care of securing sensitive data)
there are programs that install drivers for that. For remainigs from the OS we can
Explicitly clear sensitive data before releasing the memory like passwords/Hashes etc.
I go for KillTimer this weekend.
Regards
Waldo Alvarez
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/ms-tnef
Size: 22468 bytes
Desc: not available
Url : http://reactos.com:8080/pipermail/ros-kernel/attachments/20040213/55c85d27/attachment-0001.bin
More information about the Ros-kernel
mailing list