https://bugs.winehq.org/show_bug.cgi?id=37689
Bug ID: 37689 Summary: Sumatra PDF 3.0 crashes when opening .epub and .mobi files Product: Wine Version: 1.7.32 Hardware: x86 URL: https://kjkpub.s3.amazonaws.com/sumatrapdf/rel/Sumatra PDF-3.0-install.exe OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: gdiplus Assignee: wine-bugs@winehq.org Reporter: gohabsgo60@yahoo.ca Distribution: ---
Created attachment 50166 --> https://bugs.winehq.org/attachment.cgi?id=50166 backtrace
Sumatra PDF 3.0 crashes in gdiplus when opening .epub and .mobi files. .pdf and .djvu files don't make it crash.
works fine with native gdi+
i have attached a backtrace.
output has nothing relevant and is the same as when it doesn't crash, so i didn't bother attaching it.
https://bugs.winehq.org/show_bug.cgi?id=37689
zippy gohabsgo60@yahoo.ca changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |gohabsgo60@yahoo.ca
https://bugs.winehq.org/show_bug.cgi?id=37689
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download
https://bugs.winehq.org/show_bug.cgi?id=37689
Michael Müller michael@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |michael@fds-team.de
--- Comment #1 from Michael Müller michael@fds-team.de --- Hi,
I tried to debug this problem further, but it seems to be very likely just an application bug - it works with native gdiplus just by chance.
The application creates a GDI+ image from a buffer (0x108e780) which is part of the stack: ---- 0027:trace:gdiplus:GdipCreateBitmapFromScan0 32 4 128 0x26200a 0x108e780 0x108e548 [...] 0027:trace:gdiplus:GdipCreateBitmapFromScan0 <-- 0x14b5c8 ----
Now the application creates a GDI+ graphic context from this GDI+ image: ---- 0027:trace:gdiplus:GdipGetImageGraphicsContext 0x14b5c8 0x108e548 [...] 0027:trace:gdiplus:graphics_from_image <-- 0x17c920 ----
As next step the application requests a DC context, modifies it and releases it again. ---- 0027:trace:gdiplus:GdipGetDC (0x17c920, 0x108e964) 0027:trace:gdiplus:GdipGetImageBounds 0x14b5c8 0x108e8e8 0x108e864 0027:trace:gdiplus:GdipGetImageBounds returning (0.000000, 0.000000) (32.000000, 4.000000) unit type 2 0027:trace:gdiplus:GdipReleaseDC (0x17c920, 0x33007a) ----
The function GdipReleaseDC tries to sync the content between the DC context and the original image buffer by calling alpha_blend_pixels_hrgn which redirects the call to alpha_blend_bmp_pixels.
The crash occurs in alpha_blend_bmp_pixels: ---- static GpStatus alpha_blend_bmp_pixels(GpGraphics *graphics, INT dst_x, INT dst_y, const BYTE *src, INT src_width, INT src_height, INT src_stride) { GpBitmap *dst_bitmap = (GpBitmap*)graphics->image; INT x, y;
for (x=0; x<src_width; x++) { for (y=0; y<src_height; y++) { ARGB dst_color, src_color; GdipBitmapGetPixel(dst_bitmap, x+dst_x, y+dst_y, &dst_color); src_color = ((ARGB*)(src + src_stride * y))[x]; GdipBitmapSetPixel(dst_bitmap, x+dst_x, y+dst_y, color_over(dst_color, src_color)); } }
return Ok; } ----
The function reads the color from the DC image, mixes it with the destination image (i.e. the image we tracked before) and saves the final color into the destination image.
Here is an excerpt of my crash dump: ---- Unhandled exception: page fault on read access to 0x00371000 in 32-bit code (0x7ebb224d). Register dump: CS:0023 SS:002b DS:002b ES:002b FS:0063 GS:006b EIP:7ebb224d ESP:0108e810 EBP:0108e8f8 EFLAGS:00010206( R- -- I - -P- ) EAX:00371000 EBX:7ec0a000 ECX:00000000 EDX:0108ef3c ESI:00000001 EDI:00000000 ----
As you can see the ESP value 0x0108e810 and the address of the original image buffer 0x108e780 are pretty close. The difference is just 144 bytes while the image uses 512 byte (4 * 128). What happens now, is that our local variables overlap with the original image buffer and get overwritten by the image content. This causes Wine to crash since the calculated source pointer is now invalid: ---- 0x7ebb224d alpha_blend_pixels_hrgn+0x65d [/home/michael/wine/wine32/dlls/gdiplus/../../../../projects/wine/dlls/gdiplus/graphics.c:384] in gdiplus: movl 0x0(%eax),%edi 369 src_color = ((ARGB*)(src + src_stride * y))[x]; ----
The problem is most probably caused by the application since it passes a buffer which is no longer valid. If there is no special case in which this buffer is copied instead of using the original one, this is most probably an upstream bug.
The reason why the native gdiplus.dll works is that either uses less or more memory on the stack so that the internal variables of the copy function do not overlap.
Regards, Michael
https://bugs.winehq.org/show_bug.cgi?id=37689
--- Comment #2 from zippy gohabsgo60@yahoo.ca --- Michael, thanks for looking into this.
are you sure GdipCreateBitmapFromScan0 isn't supposed to make a copy of the data ?
on an unrelated note, it looks like the nested 'for' loops in alpha_blend_bmp_pixels are in the wrong order - the pixels are fetched by column which is very inefficient.
one last thing, i forgot to mention that the program is open source and available on github https://github.com/sumatrapdfreader/sumatrapdf
https://bugs.winehq.org/show_bug.cgi?id=37689
--- Comment #3 from Vincent Povirk madewokherd@gmail.com --- Yes, when given a non-NULL data pointer, the Bitmap object uses the data in place, we have tests that verify this.
I grabbed the source code from github, and it seems the only file creating Bitmap objects with a data pointer is src/mui/MuiBase.cpp, which can create a situation like Micheal described.
The object has a data array inside the GraphicsCacheEntry structure, which it uses to initialize the Bitmap* also in the struct. It creates instances of the structure like this:
GraphicsCacheEntry ce; ce.Create(); gGraphicsCache->Append(ce);
Note that ce is on the stack in this case. gGraphicsCache is a Vec<GraphicsCacheEntry>, so ce is copied into the vector. There is no copy constructor, so I guess it's effectively a memcpy. However, the Bitmap object referenced by the Bitmap* now in the vector still references the data from the ce on the stack.
What a great case study in how the automatic things C++ does can hurt you.
It would have been better to pass a NULL pointer so the data is created and owned by the Bitmap object.
That said, having an invalid data pointer in a Bitmap object is fine as long as we never try to draw to the Bitmap or read its data. In this case, that seems to be the intention. However, HtmlFormatter.cpp/TextRender.cpp at least in the 3.0 branch looks like it calls Graphics.GetDC, which is going to lead us to call alpha_blend_pixels on ReleaseDC (and I see these are in the backtrace), which will access the data EVEN IF NOTHING WAS DRAWN ON THE HDC. I bet native doesn't do that, and that could be the source of this bug.
https://bugs.winehq.org/show_bug.cgi?id=37689
--- Comment #4 from Vincent Povirk madewokherd@gmail.com --- Please retest with master or 1.7.39 when it's released, I'm hoping 08c1e6cd96064a2025f62dbc046b888b63b73b62 will fix this.
https://bugs.winehq.org/show_bug.cgi?id=37689
zippy gohabsgo60@yahoo.ca changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED
--- Comment #5 from zippy gohabsgo60@yahoo.ca --- fixed in 1.7.39
thanks Vincent !
changing status to resolved fixed.
https://bugs.winehq.org/show_bug.cgi?id=37689
Bruno Jesus 00cpxxx@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |08c1e6cd96064a2025f62dbc046 | |b888b63b73b62
https://bugs.winehq.org/show_bug.cgi?id=37689
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #6 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 1.7.41.
-
wine-bugs＠winehq.org