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@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@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@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@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
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
2005/12/10, Mike McCormack mike@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@gmail.com writes:
Cyril> 2005/12/10, Mike McCormack mike@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...
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@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@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.
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@winehq.org:
Cyril Margorin comargo@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@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@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@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
-
Alexandre Julliard -
Cyril Margorin -
Mike McCormack -
Uwe Bonnes -
Vitaliy Margolen -
Willie Sippel