Fix BMP loading bug; fix problem accepting invalid image

[fire-eggs]

This is a two part fix.

Part 1: If given a text file which has the first two characters as "BM", Fl_BMP_Image will attempt to load that file as a valid bitmap file. Depending on what is in the text file, Fl_BMP_Image will crash in various ways.

The fix is to verify the existence of some required attributes of a valid Bitmap header and set the file's fail() state.

Part 2: Having fixed the above bug, I then discovered that Fl_Shared_Image did not check the fail() state, and proceeded to act as if the invalid Fl_BMP_Image was in fact valid.

The fix here is to check the fail() state: in a failure situation, delete the invalid object, and continue iterating through the image handlers.

[Albrecht-S]

@fire-eggs Kevin, this is a tricky one. I can't find the commit 4394942 in your fork anymore although the link to the commit still displays the comparison. The "Files changed" tab shows four parts:

1. examples/table-sort.cxx: unrelated, obsoleted by a previous commit (PR failure to call pclose #240)
2. "validate reserved [should be zero]": see comment below
3. check ... if (info_size != 40) { ... // Another image size is not valid: see comment below
4. check "We might have an object, but it might not be valid.": looks plausible but needs further investigation (see below)

[fire-eggs]

Yes, part 1 you mention is irrelevant. Should I submit a new pull request? Only the changes in 4394942 are related.

[Albrecht-S]

Not yet, please wait a moment. I'm going to post my comments to 2,3, and 4 shortly. Then we can decide how to proceed.

[Albrecht-S]

2. "validate reserved [should be zero]"

According to MS docs this is correct. They say:

{ bfReserved1, bfReserved2 } : Reserved; must be zero.

However, Wikipedia docs on BMP claim:

Reserved; actual value depends on the application that creates the image, if created manually can be 0

If this is correct the patch would exclude such - otherwise valid - images, i.e. if one of the reserved words is not 0. I tried to find such images but succeeded only with a few images that didn't have values other than 0 in these reserved words. After hex-editing one of the images (replacing the contents of the reserved words with arbitrary values) the modified image could not be loaded using the patch (which is obvious, but not a valid test for anything, I know).

It would be interesting to find such non-MS-compliant images and try to open these images with different graphics applications including Microsoft's own image viewers etc..

Other than that, I believe that this check is too strict and should not be applied.

To be continued...

[Albrecht-S]

3. check ... if (info_size != 40) { ...

MS docs say:

The established bitmap file format consists of a BITMAPFILEHEADER structure followed by a BITMAPINFOHEADER, BITMAPV4HEADER, or BITMAPV5HEADER structure.

The size of BITMAPINFOHEADER is indeed 40, but BITMAPV4HEADER and BITMAPV5HEADER are larger structures.

If I'm correct then this additional check would exclude all images that include a BITMAPV4HEADER or BITMAPV5HEADER structure, respectively. The existing code ignores the parts of these larger structures (see variable repcount). Conclusion: this check is invalid as well and should not be applied. Please correct me if I missed anything.

Again, I don't have any examples. Just analyzing the code. A better option would be to test for all (three) valid structure sizes. Maybe. Comments, ideas?

To be continued...

[Albrecht-S]

Part 4: short comment: I'm not sure.

Generally the patch looks plausible (as said above), but ...

In some cases we may need an Fl_Shared_Image object although the image can't be loaded. Consider Fl_Help_View with a link to a non-existent image. I know that we render a "broken image" icon if this is true. In this case we'd have a direct link to a non-existent file (which we can't open in the first place), but what if the file exists and is broken in a way we can't load it?

This needs further investigation, I did not yet test if the patch has any consequences on this scenario or related issues. Just thinking out loud...

OK, that's it for now, I'll watch this issue, but don't expect more replies today.

@fire-eggs Thanks for all your suggestions and for considering my comments.

[Albrecht-S]

PS: comments from others appreciated as well...

[fire-eggs]

RE: parts 2 and 3. I was attempting to address an obscure edge case which I stumbled across. Breaking support for entire classes of valid BMP files is not worth it. So I'd like to withdraw the suggested changes.

RE: part 4: comments pending

[fire-eggs]

RE: part 4

I ran into this situation because I had an image which fail()-ed due to errors. I.e. Fl_Shared_Image::get() did not return NULL. Looking at the test programs, where the check is only for a NULL return, I guessed an error image would return NULL, not a fail()-ed object.

Issue A:
My first thought when I looked at the image handler loop, was what if there was a later handler which could handle the error image? Thus the change to continue the handler loop on the "fail" case.

Issue B: the possible return values from Fl_Shared_Image::get() are not documented. The documentation for various image classes (e.g. FL_BMP_Image) do document the fail() case.

Fl_Shared_Image::get() has three possible return values:

1. Returns null. The image cannot be loaded at all by any handler, or cannot be found.
2. Returns a successfully loaded image, with w / h / d values not zero.
3. Returns a Fl_Shared_Image instance with fail() returning a negative number and w / h / d set to zero. This happens if (a) the image cannot be accessed or (b) there is a load error (corrupt image).

I've not exercised 3(a) above, only 3(b), but I'm assuming the results would be the same.

Issue C: test programs using Fl_Shared_Image::get only check for NULL, not fail().

I had to find my error image and make some tests to understand the implications of Issue C above.

I've looked at most of the uses of Fl_Shared_Image::get() in the FLTK source tree. No code checks the fail() value; at best the code checks to see if the image width / height are zero.

In some cases we may need an Fl_Shared_Image object although the image can't be loaded. Consider Fl_Help_View with a link to a non-existent image. I know that we render a "broken image" icon if this is true. In this case we'd have a direct link to a non-existent file (which we can't open in the first place), but what if the file exists and is broken in a way we can't load it?

I hacked the unittests program to contain an <img> tag in the "About" text and to handling loading images. So here's what I found with Fl_Help_View::get_image:

1. If Fl_Shared_Image::get() returns NULL, the help viewer displays the "broken image" icon.
2. If the Fl_Shared_Image returned is not NULL but fail(), the viewer attempts to display the image. Here is the console output I got on Linux:

X_CreatePixmap: BadValue (integer parameter out of range for operation) 0x0
XRequest.139: BadDrawable (invalid Pixmap or Window parameter) 0x780017a
XRequest.139: RenderBadPicture (invalid Picture parameter) 0x780017b
XRequest.139: RenderBadPicture (invalid Picture parameter) 0x780017b
XRequest.139: RenderBadPicture (invalid Picture parameter) 0x780017b
XRequest.139: RenderBadPicture (invalid Picture parameter) 0x780017b

A similar set of error messages can be observed by attempting to load the "error image" using the pixmap_browser test, as it also does not check for the image height / width being zero.

On Windows there are no apparent errors or problems as a result of displaying the error image.

Issue D : I feel that FLTK should not handle a fail() image as valid. IMHO, the above errors from X11 are not a "good response" from FLTK.

Let me know if my changes to unittests would be of interest. For reference, here is my "error image":
blah.jpg.zip
[It is actually a valid JPEG file but has a CMYK profile which gives jpeglib conniptions.]

Wandered rather far afield from "attempting to check for a bad BMP file"...

Thanks for listening!

[Albrecht-S]

RE: parts 2 and 3. ... I'd like to withdraw the suggested changes.

OK.

Wandered rather far afield from "attempting to check for a bad BMP file"...
Thanks for listening!

Thank you very much for the detailed analysis and the work and time (!) you invested.

I'll need some time to reply with "real answers" and to provide my own thoughts. I'm also busy with some other stuff...

But for now some historical facts: the fail() method was IIRC added in FLTK 3.3.4 because there was no real and simple error check available before this release. The only way to determine whether an image was loaded successfully was to check w(), h(), and d() (probably all of them) to see if an image was valid. If not (valid) there was no indication of a specific error code as it is provided by fail() today (IIRC the error code is now encoded/stored in ld() but that doesn't matter here).

This is certainly the reason why the FLTK libray (Fl_Help_View) and the test and demo programs don't check for fail() in most cases and work as you describe -- i.e. they do not work correctly or at least not as expected. The above mentioned Linux/X11 errors are an example of something that must not happen ( ( although: garbage in - garbage out ? :-) ) ).

I think what we need is twofold:

1. Specify and document the corner cases, i.e. document the expected behavior
2. Modify the library and the test and demo programs to work as specified

Specifying the correct behavior for previously undocumented cases is always hard to do. We need to consider whether the new specification has the potential to break old programs that (mis)used the corner cases or "just worked" anyway.

[Albrecht-S]

Let me know if my changes to unittests would be of interest.

Yes, I think this would be helpful. Can you please post a patch/diff ?

[fire-eggs]

Let me know if my changes to unittests would be of interest.
Yes, I think this would be helpful. Can you please post a patch/diff ?

Here you go. The change to unittest_about.cxx assumes "blah.jpg" exists in the fltk/test folder.

diff ./Makefile /home/kevin/github/fltk/test/Makefile
294,296c294
< unittests$(EXEEXT): unittests.o 
< 	echo Linking $@...
< 	$(CXX) $(ARCHFLAGS) $(CXXFLAGS) $(LDFLAGS) unittests.o -o $@ $(LINKFLTKIMG) $(LDLIBS)
---
> unittests$(EXEEXT): unittests.o

diff ./unittest_about.cxx /home/kevin/github/fltk/test/unittest_about.cxx
45,47d44
< "<br>\n"
< "<img src=\"./blah.jpg\" alt=\"Bad image\">\n"
< "<br>\n"

diff ./unittests.cxx /home/kevin/github/fltk/test/unittests.cxx
33d32
< #include <FL/Fl_Shared_Image.H> // fl_register_images
175d173
<   fl_register_images(); 

[fire-eggs]

I'll need some time to reply with "real answers" and to provide my own thoughts. I'm also busy with some other stuff...

No urgency here. Thank you for prompt and regular feedback!

This is certainly the reason why the FLTK libray (Fl_Help_View) and the test and demo programs don't check for fail() in most cases and work as you describe

Ah yes, history. I wasn't aware Fl_Help_View predated the fail() change.

Modify the library and the test and demo programs to work as specified

I'm not one for touching code unnecessarily. My examination of uses of Fl_Shared_Image found that the FLTK source mostly checked for h / w / d being zero as the validity check. I think it is a valid check and doesn't need to be changed.

Fl_Help_View is an exception as it only checks for null. Here is an opportunity to use the new method.

I agree for the test and demo programs.

[Albrecht-S]

My examination of uses of Fl_Shared_Image found that the FLTK source mostly checked for h / w / d being zero as the validity check. I think it is a valid check and doesn't need to be changed.
Fl_Help_View is an exception as it only checks for null. Here is an opportunity to use the new method.

I agree, thanks for this comment as well.

[MatthiasWM]

See https://www.fltk.org/str.php?L2284 for a request on better EOF checking in BMPs.

Edit (@Albrecht-S): simplified link.