wxImage fails to load certain bitmaps (Issue #22499)
Adrian Lopez
Using wxWidgets 3.1.7, wxImage::LoadFile() fails to load some bitmaps that used to load without any problems under wxWidgets 3.0.4. I am attaching a program and sample images below. The output under wxWidgets 3.0.4 is as follows:
test1.bmp ok
test2.bmp ok
test3.bmp ok
test4.bmp ok
test5.bmp ok
test6.bmp ok
test7.bmp ok
test8.bmp ok
The output under wxWidgets 3.1.7 is as follows:
test1.bmp ok
test2.bmp ok
test3.bmp ok
test4.bmp ok
test5.bmp failed
test6.bmp failed
test7.bmp failed
test8.bmp failed
Environment:
Linux Mint 20
wxWidgets 3.1.7
wxGTK
GTK 3.24.20
Attachments: wxImageLoadTest.zip
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
I opened up test5.bmp in 4 different non-wxWidgets apps, and none of them reported a problem. I opened it up with an app built on wxWidgets 3.1.5, and it failed as did another app built on 3.1.7, but an app built on wxWidgets 3.0 (I know for certain it uses wxWidgets to load bitmaps) loaded the bitmap without a problem. This was all done on Windows, so it doesn't appear to be on OS specific issue, or specific to the app adrianlopezroche is using.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Eric Jensen
Wednesday, June 8, 2022, 12:14:42 AM, you wrote:
AL> Using wxWidgets 3.1.7, wxImage::LoadFile() fails to load some
AL> bitmaps that used to load without any problems under wxWidgets
AL> 3.0.4. I am attaching a program and sample images below. The
AL> output under wxWidgets 3.0.4 is as follows:
AL> --
AL> Reply to this email directly or view it on GitHub:
AL> https://github.com/wxWidgets/wxWidgets/issues/22499
AL> You are receiving this because you are subscribed to this thread.
AL> Message ID: <wxWidgets/wxWidgets/issues/22...@github.com>
This was indirectly caused by my commit:
https://github.com/wxWidgets/wxWidgets/commit/cccda9ef6bc7f6ee0ff47e4e9da634e762ccabb3
The code moves the file pointer after the header, to the beginning of
the image data (which is correct by itself).
Unfortunately there is a code path when BI_BITFIELDS is used, that
assumes the file pointer is still inside the header and reads another
12 bytes:
https://github.com/wxWidgets/wxWidgets/blob/dc4d1b66e4903edfcb66c3b41a36bca8806c98e5/src/common/imagbmp.cpp#L640
Not only does this read data from the wrong position, but also
"consumes" some image data, which is then missing at the end when
loading the image data, causing the error.
I don't have time to look deeper into this at the moment, because of
the different code paths and potential side effects, it seems not to
be trivial at first sight.
Hth
Eric
Maarten
caused by cccda9e but I know nothing about bitmap headers so I can't fix this myself.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Oh, Eric replied in the mailing list as well, but that is not visible here: https://groups.google.com/g/wx-dev/c/oHiaF1YpoVs/m/0rRRrQZLAQAJ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks for reporting this, it definitely needs to be fixed. In the worst case we can always revert cccda9e (we'd need to revert bcb0d42 and ca065c5 too in this case), but perhaps we could instead just make it conditional on comp != BI_BITFIELDS? As the palette is not used in this case, it shouldn't be necessary to do this SeekI(), should it? Eric, what do you think?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Eric Jensen
Wednesday, June 8, 2022, 2:26:32 PM, you wrote:
V> Thanks for reporting this, it definitely needs to be fixed. In the
V> worst case we can always revert
V> cccda9ef6bc7f6ee0ff47e4e9da634e762ccabb3 (we'd need to revert
V> bcb0d42d7a2a9a76c2d52fa75f1f57447f2aab3c and
V> ca065c500436679fc4c15d2170716df24dcfd1ab too in this case), but
V> perhaps we could instead just make it conditional on `comp !=
V> BI_BITFIELDS`? As the palette is not used in this case, it
V> shouldn't be necessary to do this `SeekI()`, should it? Eric, what do you think?
It would probably work as a quick fix, but it doesn't sound like a
clean solution to me.
If would be nice if the DoLoadDib method could assume that the
filepointer points to the actual image data. But if some code needs
additional information from the heads, that's not going to work.
There is another seek later inside DoLoadDib...
https://github.com/wxWidgets/wxWidgets/blob/dc4d1b66e4903edfcb66c3b41a36bca8806c98e5/src/common/imagbmp.cpp#L702
I was wondering if this could be combined with the Seek i added.
But as already mentioned, currently i don't have time to take a deeper
look and run tests with all those different BMP variations.
Eric
--
Vadim Zeitlin
EJ> Wednesday, June 8, 2022, 2:26:32 PM, you wrote:
EJ>
EJ> V> Thanks for reporting this, it definitely needs to be fixed. In the
EJ> V> worst case we can always revert
EJ> V> cccda9ef6bc7f6ee0ff47e4e9da634e762ccabb3 (we'd need to revert
EJ> V> bcb0d42d7a2a9a76c2d52fa75f1f57447f2aab3c and
EJ> V> ca065c500436679fc4c15d2170716df24dcfd1ab too in this case), but
EJ> V> perhaps we could instead just make it conditional on `comp !=
EJ> V> BI_BITFIELDS`? As the palette is not used in this case, it
EJ> V> shouldn't be necessary to do this `SeekI()`, should it? Eric, what do you think?
EJ>
EJ> It would probably work as a quick fix, but it doesn't sound like a
EJ> clean solution to me.
No, but right now I'd like to fix the regression, preferably without
breaking palette handling that you had fixed in that commit.
EJ> If would be nice if the DoLoadDib method could assume that the
EJ> filepointer points to the actual image data. But if some code needs
EJ> additional information from the heads, that's not going to work.
Yes, it would be definitely nice to restructure this code and pass all the
information from the header to DoLoadDib() instead of passing it some parts
of it (using no less than 12 parameters!) and letting it get some of it.
But I'm not going to do it right now.
EJ> There is another seek later inside DoLoadDib...
EJ> https://github.com/wxWidgets/wxWidgets/blob/dc4d1b66e4903edfcb66c3b41a36bca8806c98e5/src/common/imagbmp.cpp#L702
Yes, this doesn't look great neither :-(
EJ> But as already mentioned, currently i don't have time to take a deeper
EJ> look and run tests with all those different BMP variations.
I'll add my hack and a test with one of the BI_BITFIELDS bitmaps then to
at least make them work again for 3.2.0.
Thanks,
VZ
VZ
Please test the linked PR #22504 if you can, it fixes the issue with reading BI_BITFIELDS bitmaps, but also changes quite a few things, so please let me know if anybody sees any new regressions with it. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vadim Zeitlin
EJ> Wednesday, June 8, 2022, 2:26:32 PM, you wrote:
EJ>
EJ> V> Thanks for reporting this, it definitely needs to be fixed. In the
EJ> V> worst case we can always revert
EJ> V> cccda9ef6bc7f6ee0ff47e4e9da634e762ccabb3 (we'd need to revert
EJ> V> bcb0d42d7a2a9a76c2d52fa75f1f57447f2aab3c and
EJ> V> ca065c500436679fc4c15d2170716df24dcfd1ab too in this case), but
EJ> V> perhaps we could instead just make it conditional on `comp !=
EJ> V> BI_BITFIELDS`? As the palette is not used in this case, it
EJ> V> shouldn't be necessary to do this `SeekI()`, should it? Eric, what do you think?
EJ>
EJ> It would probably work as a quick fix, but it doesn't sound like a
EJ> clean solution to me.
https://github.com/wxWidgets/wxWidgets/pull/22504
This wasn't as difficult as I feared, but does change quite a few things
and even though I've tried to do it in small, easy to review steps, it's
still possible that I've broken something, so please test this PR if you
can and let me know about any breakage.
TIA!
VZ
Randalph
Eric Jensen
Thursday, June 9, 2022, 1:03:57 AM, you wrote:
V> Please test the linked PR #22504 if you can, it fixes the issue
V> with reading `BI_BITFIELDS` bitmaps, but also changes quite a few
V> things, so please let me know if anybody sees any new regressions with it. TIA!
V> --
V> Reply to this email directly or view it on GitHub:
V> https://github.com/wxWidgets/wxWidgets/issues/22499#issuecomment-1150499301
V> You are receiving this because you are subscribed to this thread.
V> Message ID: <wxWidgets/wxWidgets/issues/22499/11504...@github.com>
Very nice cleanup, thanks!
I tested the new code with all bmps i could find on my system (~4000),
and they mostly worked fine.
Only two were rejected with the following error:
"BMP: header has biClrUsed=257 when biBitCount=8"
The error is justified, however most other apps choose to ignore it
and load the file just fine.
Not a regression, but it might be worth considering to ignore that
error, too.
Regards,
Eric
--
Vadim Zeitlin
EJ> I tested the new code with all bmps i could find on my system (~4000),
EJ> and they mostly worked fine.
Thanks to you and Randalph for testing this!
EJ> Only two were rejected with the following error:
EJ> "BMP: header has biClrUsed=257 when biBitCount=8"
EJ>
EJ> The error is justified, however most other apps choose to ignore it
EJ> and load the file just fine.
EJ>
EJ> Not a regression, but it might be worth considering to ignore that
EJ> error, too.
We can change this later, but not loading them is better than crashing
(see https://github.com/wxWidgets/wxWidgets/issues/19295) and it's not
clear what should we do here (just ignore the extra colours, I guess, but
this is weird), so I won't change this for now, but we could change this
later, including in 3.2.x.
BTW, if anyone is interested in further improving this code, it would be
really great to add support for fuzzing it on oss-fuzz (see tests/fuzz), as
I'm -- unfortunately -- pretty sure that this will find more bugs in it.
Thanks again for testing and I'll merge this soon if nobody else finds any
problems with the changes,
VZ
Adrian Lopez
PR #22504 fixes the problem for me.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #22499 as completed via 3ffc739.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.