December 2005 - Wine-Devel - list.winehq.org

Re: LoadImage (4bpp) / CopyImage() crashing

by Mike McCormack

Cyril Margorin wrote: > Unfortunately, problem is that GetBitmapBits function in wine is > incorrect, and may causes a buffer overflow, but the GetDIBits isn't. Great. Then we need to fix it, not try avoid it. Mike

Dec. 9, 2005

Re: LoadImage (4bpp) / CopyImage() crashing

by Vitaliy Margolen

Friday, December 9, 2005, 3:12:23 PM, Cyril Margorin wrote: > Hello, > Could anyone tell me, why function CopyImage uses > GetBitmapBits/SetBitmapBits, while MSDN marks these function as > absolete? Because what MSDN says doesn't really mean it's the same on Wine. But my guess would be that it's been implemented that way and no one changed it. > I've tried to move from GetBitmapBits/SetBitmapBits to > GetDIBits/SetDIBits. It seems to me that this way is correct, if keep > in mind, that GetBitmapBits is wrong. > > In attachment you may find a patch for it. Please use unified format from the to of the tree. (looks like you beat me to it). But your patch looks like 10x more then what was there before. Also you are using CreateDCW which a really heavy function IMHO for what you need. Why don't use GetDC(0)?

Dec. 9, 2005

Re: LoadImage (4bpp) / CopyImage() crashing

by Cyril Margorin

Unfortunately, problem is that GetBitmapBits function in wine is incorrect, and may causes a buffer overflow, but the GetDIBits isn't. 2005/12/10, Mike McCormack <mike(a)codeweavers.com>: > Cyril Margorin wrote: > > > Could anyone tell me, why function CopyImage uses > > GetBitmapBits/SetBitmapBits, while MSDN marks these function as > > absolete? > > No matter what MSDN says, Wine still has to provide implementations of > those functions, as programs still use them. Since we *must* have > implementations of them, then there's no reason why we shouldn't use > those implementations in our code too. > > Perhaps MSDN should say "we wrote new fancy versions of these functions > and we want everybody to use them". Don't pay too much attention to it. -- Cyril Margorin

Dec. 9, 2005

Re: LoadImage (4bpp) / CopyImage() crashing

by Mike McCormack

Cyril Margorin wrote: > Could anyone tell me, why function CopyImage uses > GetBitmapBits/SetBitmapBits, while MSDN marks these function as > absolete? No matter what MSDN says, Wine still has to provide implementations of those functions, as programs still use them. Since we *must* have implementations of them, then there's no reason why we shouldn't use those implementations in our code too. Perhaps MSDN should say "we wrote new fancy versions of these functions and we want everybody to use them". Don't pay too much attention to it. Mike

Dec. 9, 2005

Re: WIDL: Don't Output __RPC_FAR In Generated Code

by Hans Leidekker

On Friday 09 December 2005 20:21, Robert Shearman wrote: (note: I see no problem with this patch) print_server("*(("); write_type(server, def->type, def, def->tname); - fprintf(server, " __RPC_FAR *)_StubMsg.Buffer)++ = _RetVal;\n"); + fprintf(server, " *)_StubMsg.Buffer)++ = _RetVal;\n"); I remember sending a patch for the MIDL generated oleaut32/oaidl_p.c to remove all these lvalue casts that had become illegal with the arrival of gcc4 ("invalid lvalue in assignment"). To avoid these in widl the code could read something like this: print_server("*("); write_type(server, def->type, def, def->tname); fprintf(server, " *)_StubMsg.Buffer = _RetVal;\n"); fprintf(server, "_StubMsg.Buffer += sizeof("); write_type(server, def->type, def, def->tname); fprintf(server, ");\n"); I don't have my repository at hand now but I can send a patch this weekend. -Hans

Dec. 9, 2005

Re: LoadImage (4bpp) / CopyImage() crashing

by Cyril Margorin

I'm sorry, previouse diff was made with '-u' flag. In attachment - fixed one. 2005/12/10, Cyril Margorin <comargo(a)gmail.com>: > Hello, > > Could anyone tell me, why function CopyImage uses > GetBitmapBits/SetBitmapBits, while MSDN marks these function as > absolete? > > I've tried to move from GetBitmapBits/SetBitmapBits to > GetDIBits/SetDIBits. It seems to me that this way is correct, if keep > in mind, that GetBitmapBits is wrong. > > In attachment you may find a patch for it. > > Am I wrong somewhere? > > 2005/11/25, Cyril Margorin <comargo(a)gmail.com>: > > Hello, > > > > During research the crashing application I've found a problem that can > > be easily reproduced by test. > > (http://www.winehq.org/pipermail/wine-patches/2005-November/022384.html) > > > > By investigation of problem I've found that original DIB bit depth is > > 4bpp, Physical pixmap bit depth is (equal to screen) 24bpp. > > In x11drv/bitmap.c X11_GetBitmapBits takes in account just only bit > > depth of physical pixmap, and copies it to provided buffer as-is. > > I.e. it tries to fill buffer with size (e.g.) 48x48x4bpp with > > 48x48x24bpp that causes buffer overrun and late falling in > > SetBitmapBits function. > > > -- > > Cyril Margorin > > > > > -- > Cyril Margorin > > > -- Cyril Margorin

Dec. 9, 2005

Re: LoadImage (4bpp) / CopyImage() crashing

by Cyril Margorin

Hello, Could anyone tell me, why function CopyImage uses GetBitmapBits/SetBitmapBits, while MSDN marks these function as absolete? I've tried to move from GetBitmapBits/SetBitmapBits to GetDIBits/SetDIBits. It seems to me that this way is correct, if keep in mind, that GetBitmapBits is wrong. In attachment you may find a patch for it. Am I wrong somewhere? 2005/11/25, Cyril Margorin <comargo(a)gmail.com>: > Hello, > > During research the crashing application I've found a problem that can > be easily reproduced by test. > (http://www.winehq.org/pipermail/wine-patches/2005-November/022384.html) > > By investigation of problem I've found that original DIB bit depth is > 4bpp, Physical pixmap bit depth is (equal to screen) 24bpp. > In x11drv/bitmap.c X11_GetBitmapBits takes in account just only bit > depth of physical pixmap, and copies it to provided buffer as-is. > I.e. it tries to fill buffer with size (e.g.) 48x48x4bpp with > 48x48x24bpp that causes buffer overrun and late falling in > SetBitmapBits function. > -- > Cyril Margorin > -- Cyril Margorin

Dec. 9, 2005

Re: loader: Reserve and allocate SharedUserData.

by Robert Shearman

Vitaliy Margolen wrote: >Friday, December 9, 2005, 12:18:26 PM, Robert Shearman wrote: > > >>Vitaliy Margolen wrote: >> >> >>>+ /* Map SharedUserData */ >>>+ NtAllocateVirtualMemory( NtCurrentProcess(), (PVOID)&SUD, 0, >>>&size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); >>> >>> >>> >>Wouldn't it be better to use NtCreateSection/NtMapViewOfSection instead? >> >> > >If we wanted to make it a real UserSharedData - sure. But we don't >really want to. At least not right now. Also the problem would be a >version information (that this structure is full of). > > Ok, fair enough. -- Rob Shearman

Dec. 9, 2005

Re: loader: Reserve and allocate SharedUserData.

by Vitaliy Margolen

Friday, December 9, 2005, 12:18:26 PM, Robert Shearman wrote: > Vitaliy Margolen wrote: >>+ /* Map SharedUserData */ >>+ NtAllocateVirtualMemory( NtCurrentProcess(), (PVOID)&SUD, 0, >>&size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); >> > Wouldn't it be better to use NtCreateSection/NtMapViewOfSection instead? If we wanted to make it a real UserSharedData - sure. But we don't really want to. At least not right now. Also the problem would be a version information (that this structure is full of). Vitaliy.

Dec. 9, 2005

Re: loader: Reserve and allocate SharedUserData.

by Robert Shearman

Vitaliy Margolen wrote: >diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c >index f0a4eed..8fbcad1 100644 >--- a/dlls/ntdll/loader.c >+++ b/dlls/ntdll/loader.c >@@ -42,6 +42,7 @@ > #include "wine/debug.h" > #include "wine/server.h" > #include "ntdll_misc.h" >+#include "ddk/wdm.h" > > WINE_DEFAULT_DEBUG_CHANNEL(module); > WINE_DECLARE_DEBUG_CHANNEL(relay); >@@ -2166,6 +2167,8 @@ void __wine_process_init( int argc, char > ANSI_STRING func_name; > void (* DECLSPEC_NORETURN init_func)(); > extern mode_t FILE_umask; >+ PKSHARED_USER_DATA SUD = (PVOID)0x7ffe0000; > > This is a bad choice of name. SharedUserData or shared_user_data would be better. >+ ULONG size = 0x1000; > > thread_init(); > >@@ -2176,6 +2179,10 @@ void __wine_process_init( int argc, char > /* setup the load callback and create ntdll modref */ > wine_dll_set_callback( load_builtin_callback ); > >+ /* Map SharedUserData */ >+ NtAllocateVirtualMemory( NtCurrentProcess(), (PVOID)&SUD, 0, &size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); > > Wouldn't it be better to use NtCreateSection/NtMapViewOfSection instead? >+ SUD->TickCountLowDeprecated = 123456; >+ > if ((status = load_builtin_dll( NULL, kernel32W, 0, &wm )) != STATUS_SUCCESS) > { > MESSAGE( "wine: could not load kernel32.dll, status %lx\n", status ); > > -- Rob Shearman

Dec. 9, 2005