[ros-dev] bug in vfatfs.sys?
Alex Ionescu
ionucu at videotron.ca
Fri Jan 4 14:23:19 CET 2008
On 3-Jan-08, at 4:41 PM, Oriol wrote:
> I get an error while trying to install Firefox 2.0 on a bootcd
> compilation, finally i found the possibly bug is in vfatfs.sys i
> modified some lines but is not my code.....it seems to install
> Firefox.
> Also, testing firefox i found problems with gdi and also done some
> modifications, still testing them..
> Here is my patch..NO WARRANTY he he..
>
> Index: tools/rbuild/configuration.cpp
> ===================================================================
> --- tools/rbuild/configuration.cpp (revision 31587)
> +++ tools/rbuild/configuration.cpp (working copy)
> @@ -28,7 +28,7 @@
> CheckDependenciesForModuleOnly = false;
> CompilationUnitsEnabled = true;
> MakeHandlesInstallDirectories = false;
> - GenerateProxyMakefilesInSourceTree = false;
> + GenerateProxyMakefilesInSourceTree = true;
> InstallFiles = false;
> UseConfigurationInPath = false;
> UseVSVersionInPath = false;
What? Why? How does this help fix a VFAT bug?
> Index: ntoskrnl/include/internal/mm.h
> ===================================================================
> --- ntoskrnl/include/internal/mm.h (revision 31587)
> +++ ntoskrnl/include/internal/mm.h (working copy)
> @@ -103,7 +103,7 @@
> /*
> * Maximum size of the kmalloc area (this is totally arbitary)
> */
> -#define MM_KERNEL_MAP_SIZE (16*1024*1024)
> +#define MM_KERNEL_MAP_SIZE (64*1024*1024)
> #define MM_KERNEL_MAP_BASE (0xf0c00000)
>
> /*
What? Nobody uses this value anymore, and why would increasing it to
64MB help? These values need to go away.
> Index: ntoskrnl/mm/rpoolmgr.h
> ===================================================================
> --- ntoskrnl/mm/rpoolmgr.h (revision 31587)
> +++ ntoskrnl/mm/rpoolmgr.h (working copy)
> @@ -32,7 +32,7 @@
>
> #ifndef R_QUECOUNT
> // 16, 32, 64, 128, 256, 512
> -#define R_QUECOUNT 6
> +#define R_QUECOUNT 512
> #endif//R_QUECOUNT
>
This slows down the pool considerably.
>
> #ifndef R_RZ
> @@ -50,7 +50,7 @@
>
> #ifndef R_STACK
> // R_STACK is the number of stack entries to store in blocks for
> debug purposes
> -#define R_STACK 6
> +#define R_STACK 32
> #else // R_STACK
> #if R_STACK > 0 && R_STACK < 6
> /* Increase the frame depth to get a reasonable back trace */
> @@ -152,7 +152,7 @@
> }
> #endif//R_STACK
This just makes debugging easier but uses 6x more memory per pool
allocation and is much slower.
>
> -static int
> +static rulong
> RQueWhich ( rulong size )
> {
> rulong que, quesize;
> @@ -888,7 +888,7 @@
> rulong UsedSize;
> PR_FREE FreeBlock;
> rulong UserSize;
> - int que;
> + long que;
>
> ASSERT(pool);
> if ( !Addr )
What? Huh? How does this fix VFAT or do anything?
> Index: base/system/winlogon/screensaver.c
> ===================================================================
> --- base/system/winlogon/screensaver.c (revision 31587)
> +++ base/system/winlogon/screensaver.c (working copy)
> @@ -3,7 +3,7 @@
> * PROJECT: ReactOS Winlogon
> * FILE: base/system/winlogon/screensaver.c
> * PURPOSE: Screen saver management
> - * PROGRAMMERS: Herv� Poussineau (hpoussin at reactos.org)
> + * PROGRAMMERS: Herv� Poussineau (hpoussin at reactos.org)
> */
>
> /* INCLUDES
> *****************************************************************/
> @@ -43,20 +43,30 @@
> OUT LPDWORD Timeout)
> {
> BOOL Enabled;
> -
> + if(Timeout==NULL)
> + {
> + ERR("TimeOut is NULL!!!");
> + return;
> + }
Okay...but...this isn't an optional parameter, so it *should* crash
if it's 0.
> if (!SystemParametersInfoW(SPI_GETSCREENSAVETIMEOUT, 0, Timeout, 0))
> {
> - WARN("WL: Unable to get screen saver timeout (error %lu).
> Disabling it\n", GetLastError());
> - *Timeout = INFINITE;
> + ERR("WL: Unable to get screen saver timeout (error %lu).
> Disabling it\n", GetLastError());
> + if(Timeout==NULL)
> + {
> + ERR("TimeOut is still NULL!!!");
> + return;
> + }
How can it be still NULL if you've already exited the function above?!
> + else
> + *Timeout = INFINITE;
> }
> else if (!SystemParametersInfoW(SPI_GETSCREENSAVEACTIVE, 0,
> &Enabled, 0))
> {
> - WARN("WL: Unable to check if screen saver is enabled (error %
> lu). Disabling it\n", GetLastError());
> + ERR("WL: Unable to check if screen saver is enabled (error %lu).
> Disabling it\n", GetLastError());
> *Timeout = INFINITE;
> }
> else if (!Enabled)
> {
> - TRACE("WL: Screen saver is disabled\n");
> + ERR("WL: Screen saver is disabled\n");
> *Timeout = INFINITE;
> }
> else
> @@ -84,7 +94,7 @@
> if (!ImpersonateLoggedOnUser(Session->UserToken))
> {
> ERR("ImpersonateLoggedOnUser() failed with error %lu\n",
> GetLastError());
> - return 0;
> + goto cleanup;
> }
>
> Session->hUserActivity = CreateEventW(NULL, FALSE, FALSE, NULL);
What do these useless screensaver debug print changes have to do with
VFAT?
> Index: base/system/winlogon/setup.c
> ===================================================================
> --- base/system/winlogon/setup.c (revision 31587)
> +++ base/system/winlogon/setup.c (working copy)
> @@ -19,13 +19,15 @@
> DWORD
> GetSetupType(VOID)
> {
> + //CONTEXT("GetSetupType");
> +
Huh?
> DWORD dwError;
> HKEY hKey;
> DWORD dwType;
> DWORD dwSize;
> DWORD dwSetupType;
>
> - TRACE("GetSetupType()\n");
> + ERR("GetSetupType()\n");
>
> /* Open key */
> dwError = RegOpenKeyExW(
> @@ -48,11 +50,12 @@
> &dwSize);
>
> /* Close key, and check if returned values are correct */
> - RegCloseKey(hKey);
> + if(dwError==ERROR_SUCCESS)
> + RegCloseKey(hKey);
You're leaking the key handle now!
> if (dwError != ERROR_SUCCESS || dwType != REG_DWORD || dwSize !=
> sizeof(DWORD))
> return 0;
>
> - TRACE("GetSetupType() returns %lu\n", dwSetupType);
> + ERR("GetSetupType() returns %lu\n", dwSetupType);
> return dwSetupType;
> }
>
> @@ -71,7 +74,7 @@
> DWORD dwSize;
> DWORD dwExitCode;
>
> - TRACE("RunSetup() called\n");
> + ERR("RunSetup() called\n");
>
> /* Open key */
> dwError = RegOpenKeyExW(
> @@ -107,7 +110,7 @@
> else
> return FALSE;
>
> - TRACE("Should run '%s' now\n", debugstr_w(CommandLine));
> + ERR("Should run '%s' now\n", debugstr_w(CommandLine));
>
> /* Start process */
> StartupInfo.cb = sizeof(StartupInfo);
> @@ -130,7 +133,7 @@
> &ProcessInformation);
> if (!Result)
> {
> - TRACE("Failed to run setup process\n");
> + ERR("Failed to run setup process\n");
> return FALSE;
> }
>
> @@ -143,7 +146,7 @@
> CloseHandle(ProcessInformation.hThread);
> CloseHandle(ProcessInformation.hProcess);
>
> - TRACE ("RunSetup() done\n");
> + ERR ("RunSetup() done\n");
>
> return TRUE;
> }
How does spamming the debug log help VFAT?
> Index: dll/win32/newdev/newdev.c
> ===================================================================
> --- dll/win32/newdev/newdev.c (revision 31587)
> +++ dll/win32/newdev/newdev.c (working copy)
> @@ -1,7 +1,7 @@
> /*
> * New device installer (newdev.dll)
> *
> - * Copyright 2005-2006 Herv� Poussineau (hpoussin at reactos.org)
> + * Copyright 2005-2006 Herv� Poussineau (hpoussin at reactos.org)
> * 2005 Christoph von Wittich (Christoph at ActiveVB.de)
> *
> * This library is free software; you can redistribute it and/or
> @@ -718,7 +718,7 @@
> {
> TRACE("SetupDiGetDeviceRegistryProperty() failed with error 0x%x
> (InstanceId %s)\n",
> GetLastError(), debugstr_w(InstanceId));
> - goto cleanup;
> + //goto cleanup;
Now this code won't return and who knows what side effects that
causes...
> }
>
> if (SetupDiGetDeviceRegistryPropertyW(
> Index: dll/win32/gdi32/objects/dc.c
> ===================================================================
> --- dll/win32/gdi32/objects/dc.c (revision 31587)
> +++ dll/win32/gdi32/objects/dc.c (working copy)
> @@ -58,12 +58,12 @@
> hspool,
> (PVOID) NULL, // NULL for now.
> (PVOID) &UMdhpdev );
> -#if 0
> +#if 1
> // Handle something other than a normal dc object.
> if (GDI_HANDLE_GET_TYPE(hDC) != GDI_OBJECT_TYPE_DC)
> {
> - PDC_ATTR Dc_Attr;
> - PLDC pLDC;
> + PDC_ATTR Dc_Attr=NULL;
> + PLDC pLDC=NULL;
>
> GdiGetHandleUserData((HGDIOBJ) hDC, GDI_OBJECT_TYPE_DC,
> (PVOID) &Dc_Attr);
>
> @@ -254,7 +254,7 @@
> DeleteDC(HDC hDC)
> {
> BOOL Ret = TRUE;
> -#if 0
> +#if 1
> PDC_ATTR Dc_Attr;
> PLDC pLDC;
>
> @@ -1342,9 +1342,9 @@
> SelectObject(HDC hDC,
> HGDIOBJ hGdiObj)
> {
> - PDC_ATTR pDc_Attr;
> -// HGDIOBJ hOldObj = NULL;
> -// PTEB pTeb;
> + PDC_ATTR pDc_Attr=NULL;
> + HGDIOBJ hOldObj = NULL;
> + //PTEB pTeb=NULL;
>
> if(!GdiGetHandleUserData(hDC, GDI_OBJECT_TYPE_DC, (PVOID)
> &pDc_Attr))
> {
> @@ -1355,9 +1355,10 @@
> hGdiObj = GdiFixUpHandle(hGdiObj);
> if (!GdiIsHandleValid(hGdiObj))
> {
> + DPRINT1("SelectObject:Handle not valid");
> return NULL;
> }
> -
> +
> UINT uType = GDI_HANDLE_GET_TYPE(hGdiObj);
>
> switch (uType)
> @@ -1369,28 +1370,39 @@
> return NtGdiSelectBitmap(hDC, hGdiObj);
>
> case GDI_OBJECT_TYPE_BRUSH:
> -#if 0 // enable this when support is ready in win32k
> - hOldObj = pDc_Attr->hbrush;
> - pDc_Attr->ulDirty_ |= DC_BRUSH_DIRTY;
> - pDc_Attr->hbrush = hGdiObj;
> +#if 1 // enable this when support is ready in win32k
> + if(pDc_Attr)
> + {
> + DPRINT1("SelectObject:Selectng brush & returning old brush");
> + hOldObj = pDc_Attr->hbrush;
> + pDc_Attr->ulDirty_ |= DC_BRUSH_DIRTY;
> + pDc_Attr->hbrush = hGdiObj;
> + }
> + NtGdiSelectBrush(hDC, hGdiObj);
> return hOldObj;
> #endif
> - return NtGdiSelectBrush(hDC, hGdiObj);
> +
>
> case GDI_OBJECT_TYPE_PEN:
> // case GDI_OBJECT_TYPE_EXTPEN:
> -#if 0 // enable this when support is ready in win32k
> - hOldObj = pDc_Attr->hpen;
> - pDc_Attr->ulDirty_ |= DC_PEN_DIRTY;
> - pDc_Attr->hpen = hGdiObj;
> +#if 1 // enable this when support is ready in win32k
> + if(pDc_Attr)
> + {
> + DPRINT1("SelectObject:Selectng pen & returning old pen");
> + hOldObj = pDc_Attr->hpen;
> + pDc_Attr->ulDirty_ |= DC_PEN_DIRTY;
> + pDc_Attr->hpen = hGdiObj;
> + }
> + NtGdiSelectPen(hDC, hGdiObj);
> return hOldObj;
> #endif
> - return NtGdiSelectPen(hDC, hGdiObj);
> +
>
> case GDI_OBJECT_TYPE_FONT:
> #if 0
> pTeb = NtCurrentTeb();
> - if (((pTeb->GdiTebBatch.HDC == 0) ||
> +
> + if (pTeb!=NULL && ((pTeb->GdiTebBatch.HDC == 0) ||
> (pTeb->GdiTebBatch.HDC == (ULONG)hDC)) &&
> ((pTeb->GdiTebBatch.Offset + sizeof(GDIBSOBJECT))
> <= GDIBATCHBUFSIZE) &&
> (!(pDc_Attr->ulDirty_ & DC_DIBSECTION)))
The TEB can't be NULL, and why did you re-enable James' disabled
code? He disabled it for a reason. How is this related to VFAT?
> Index: dll/win32/gdi32/misc/misc.c
> ===================================================================
> --- dll/win32/gdi32/misc/misc.c (revision 31587)
> +++ dll/win32/gdi32/misc/misc.c (working copy)
> @@ -129,18 +129,23 @@
> // Need to test if we have Read & Write access to the VM
> address space.
> //
> BOOL Result = TRUE;
> - if(Entry->UserData)
> + if(Entry && Entry->UserData)
> {
> volatile CHAR *Current = (volatile CHAR*)Entry->UserData;
> - _SEH_TRY
> + if(Current==NULL)
> + Result=FALSE;
How can Current be NULL? You've already checked it's not NULL and
you've got SEH below...
> + else
> {
> - *Current = *Current;
> + _SEH_TRY
> + {
> + *Current = *Current;
> + }
> + _SEH_HANDLE
> + {
> + Result = FALSE;
> + }
> + _SEH_END
> }
> - _SEH_HANDLE
> - {
> - Result = FALSE;
> - }
> - _SEH_END
> }
> else
> Result = FALSE; // Can not be zero.
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
This almost look like a robot-generated patch of completely random,
useless and broken code changes. How is any of this useful to FireFox
and VFAT?
Best regards,
Alex Ionescu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.reactos.org/pipermail/ros-dev/attachments/20080104/96316319/attachment-0001.html
More information about the Ros-dev
mailing list