LoadImage (4bpp) / CopyImage() crashing
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. The current work-around to stop falling is Index: dlls/x11drv/bitmap.c =================================================================== RCS file: /home/wine/wine/dlls/x11drv/bitmap.c,v retrieving revision 1.18 diff -u -r1.18 bitmap.c --- dlls/x11drv/bitmap.c 26 Sep 2005 11:04:12 -0000 1.18 +++ dlls/x11drv/bitmap.c 25 Nov 2005 09:29:12 -0000 @@ -186,7 +186,19 @@ /* copy XImage to 16 bit padded image buffer with real bitsperpixel */ startline = buffer; - switch (physBitmap->pixmap_depth) + + /********************************************************************** + * CoMargo: the switching for physBitmap->pixmap_depth is not correct. + * It should take both physBitmap->pixmap_depth and bitmap.bmBitsPixel + * and convert from one bitdepth to another. + * Otherwise we meet buffer overrun. + */ + if(physBitmap->pixmap_depth != bitmap.bmBitsPixel) + { + FIXME("Pixel conversion from %d bitdepth to %d bitdepth MUST be done!\n",physBitmap->pixmap_depth,bitmap.bmBitsPixel); + } +/* switch (physBitmap->pixmap_depth) */ + switch(bitmap.bmBitsPixel) { case 1: for (h=0;h<height;h++) =================================================================== -- 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
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
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
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
2005/12/10, Mike McCormack <mike(a)codeweavers.com>:
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.
Yes, but I'm out of ideas how to fix it. The problem is that it takes in to account only physical pixmap (as it was called in x11drv/bitmap.c) but it should take pair physical pixmap/logical bitmap, and make pixels conversion from one to another. -- Cyril Margorin
"Cyril" == Cyril Margorin <comargo(a)gmail.com> writes:
Cyril> 2005/12/10, Mike McCormack <mike(a)codeweavers.com>: >> > 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. Cyril> Yes, but I'm out of ideas how to fix it. The problem is that it Cyril> takes in to account only physical pixmap (as it was called in Cyril> x11drv/bitmap.c) but it should take pair physical pixmap/logical Cyril> bitmap, and make pixels conversion from one to another. If you look in the archives, you will see that I also stumbles about crashes in CopyImage. My patches (by far not as elaborated as yours) where also not applied. I also was out of ideas and hoped somebody else would pick up the problem... -- Uwe Bonnes bon(a)elektron.ikp.physik.tu-darmstadt.de Institut fuer Kernphysik Schlossgartenstrasse 9 64289 Darmstadt --------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
Hello, I've just noticed that Alexander had put patch that eliminates crashing on CopyImage. The way, he solved this problem is simple and genious. But I've found, that copied image can't be drawed to DC. So I insist, that CopyImage should be rewrited. The reasons are: - GetBitmapBits function loses color table information, stored with DIB. - New created image should be easy selectable in DC, but it must have DIB section and associated created PhysicalPixmap. In attachment you can find my (next) view on that function. -- Cyril Margorin
Hello, Is something wrong with patch that I sent before? I wonder that no one tell me anything about it. 2006/1/12, Cyril Margorin <comargo(a)gmail.com>:
Hello,
I've just noticed that Alexander had put patch that eliminates crashing on CopyImage. The way, he solved this problem is simple and genious. But I've found, that copied image can't be drawed to DC. So I insist, that CopyImage should be rewrited. The reasons are: - GetBitmapBits function loses color table information, stored with DIB. - New created image should be easy selectable in DC, but it must have DIB section and associated created PhysicalPixmap.
In attachment you can find my (next) view on that function.
-- Cyril Margorin
-- Cyril Margorin
Cyril Margorin <comargo(a)gmail.com> writes:
Hello,
Is something wrong with patch that I sent before? I wonder that no one tell me anything about it.
You are creating a DIB section in all cases, that's not what the function is supposed to do. I'd suggest to start by writing some test cases. -- Alexandre Julliard julliard(a)winehq.org
Hello Alexandre. Ah.. ok... So, as I see.... The CopyImage should create DeviceDependedBitmap, and, in case of LR_CREATEDIBSECTION flag, it should create DIB section. Currently the DDB, that it creates, is wrong (it can't be selected in device context). Well, I'll try to think about it. 2006/1/23, Alexandre Julliard <julliard(a)winehq.org>:
Cyril Margorin <comargo(a)gmail.com> writes:
Hello,
Is something wrong with patch that I sent before? I wonder that no one tell me anything about it.
You are creating a DIB section in all cases, that's not what the function is supposed to do. I'd suggest to start by writing some test cases.
-- Alexandre Julliard julliard(a)winehq.org
-- Cyril Margorin
Hello Attached is my next view on solving of problem. I haven't create test for this problem yet, because current test application uses visual representation of problem (there is garbage in window in original version, and there is normal image in patched version). I hope I will create test in day or two. Changelog: Cyril Margorin <comargo(a)gmail.com> Fix of CopyImage function for non-compatible to ScreenDC bitmaps. Handling of LR_CREATEDIBSECTION flag.
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)?
the second email was send with patched created using such command: -- cvs diff -u dlls/user/cursoricon.c > user_cursoricon_copyimage.patch -- Is it correct? (I'm sorry, my cvs experience is not big enough). Also I added 'diff -u' line to my .cvsrc file. According to CreateDCW function - this block was looked up from same file. It uses static variable screen_dc.... But may be you are right, GetDC(0) should be used. 2005/12/10, Vitaliy Margolen <wine-devel(a)kievinfo.com>:
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)?
-- Cyril Margorin
Am Freitag, 25. November 2005 18:22 schrieb Cyril Margorin:
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.
I just noticed that the Poser 6.0 demo also seems to crash at X11_GetBitmapBits, but your patch makes no difference. Don't know if it's supposed to... Anyway, in case you're interested, the Poser 6 crash, including a +x11drv trace and a link to the demo, is filed as bug 4034: http://bugs.winehq.org/show_bug.cgi?id=4034
The current work-around to stop falling is Index: dlls/x11drv/bitmap.c =================================================================== RCS file: /home/wine/wine/dlls/x11drv/bitmap.c,v retrieving revision 1.18 diff -u -r1.18 bitmap.c --- dlls/x11drv/bitmap.c 26 Sep 2005 11:04:12 -0000 1.18 +++ dlls/x11drv/bitmap.c 25 Nov 2005 09:29:12 -0000 @@ -186,7 +186,19 @@ /* copy XImage to 16 bit padded image buffer with real bitsperpixel */
startline = buffer; - switch (physBitmap->pixmap_depth) + + /********************************************************************** + * CoMargo: the switching for physBitmap->pixmap_depth is not correct. + * It should take both physBitmap->pixmap_depth and bitmap.bmBitsPixel + * and convert from one bitdepth to another. + * Otherwise we meet buffer overrun. + */ + if(physBitmap->pixmap_depth != bitmap.bmBitsPixel) + { + FIXME("Pixel conversion from %d bitdepth to %d bitdepth MUST be done!\n",physBitmap->pixmap_depth,bitmap.bmBitsPixel); + } +/* switch (physBitmap->pixmap_depth) */ + switch(bitmap.bmBitsPixel) { case 1: for (h=0;h<height;h++) ===================================================================
-- Cyril Margorin
Ciao, Willie -- Willie Sippel //////// | Tritium Studios // | ______________________________ //// /// | http://www.tritium-studios.com <willie(a)froq.net>
participants (6)
-
Alexandre Julliard -
Cyril Margorin -
Mike McCormack -
Uwe Bonnes -
Vitaliy Margolen -
Willie Sippel