Fl_Image_Reader::read_byte() does not return EOF with file reads #271
Comments
|
Good catch. Well, while read_byte() is a public method, the .h file is private (.h? Shouldn't it be .H?), so I guess there's at least two ways to solve:
-or-
Not sure which is best. |
|
Hm, @@ -74,7 +74,9 @@ Fl_Image_Reader::~Fl_Image_Reader() {
// Read a single byte from memory or a file
uchar Fl_Image_Reader::read_byte() {
if (pIsFile) {
- return getc(pFile);
+ int c = getc(pFile);
+ printf("getc: %d ferror: %d feof: %d\n", c, ferror(pFile), feof(pFile));
+ return c;
} else if (pIsData) {
return *pData++;
} else {
gives:
|
|
Good info; if we go the (2) route we should make mention of that. The docs for getc(3) do say it returns -1 on EOF /or/ error, and I suppose one is left to test either feof() or ferror() to determine what actually happened. |
|
Historically the code in question has been directly in the image loading functions, w/o the Fl_Image_Reader class. IIRC that old code didn't check for any errors. The prerequisite for loading a particular image was that it is valid data. Nevertheless this 'GH Issue' is a valid issue, but it's not only the lacks a At this point in time I'm not sure whether going the (1) or (2) route would be the better choice. Probably the most simple "fix" would be to use the |
Yep, the POSIX man page says this explicitly:
Note: the POSIX man page of getc() refers to fgetc(), hence the link above to fgetc(). |
No, not according to the CMP. |
FLTK 1.3 has the same issue with the NEXTBYTE macro: Line 155 in 4aebde8 |
|
There could be another way when reading from a file:
We should think about not reading any further bytes if a previous read operation yielded EOF or ERROR to stay consistent if the caller doesn't check for EOF or ERROR immediately. If we went that route I'd recommend to return 0 rather than 255 ( |
|
Reading from memory could be enhanced as follows: we add another constructor with an additional size argument to The drawback is that we can't easily remove the old constructor(s) which should then be deprecated and necessarily have to assume "infinite memory/size". These existing constructors do already document explicitly that buffer overruns are not checked. |
|
Wouldn't option (3) just be duplicating the separate error/eof flags in the FILE struct? Apparently that's how ferror()/feof() do what they do. If we do want our own ::eof() and ::error() detection, we can just wrap feof() and ferror() on the FILE* I'd think, no need for separate flags. But yes, any EOF detected from any of the read functions should immediately return the indication and wind back out of the image reader "gracefully" on an incomplete read. Not sure if that means returning a black image, or deallocating the image and returning a NULL image with zero for the x()/y() sizes. In my film production days, we'd often code the image reader library to show an "X" frame (black image with a white X to all 4 corners) as a visual indication of an image that didn't load completely. |
|
I have tested `bug.gif' with this change: diff --git a/src/Fl_GIF_Image.cxx b/src/Fl_GIF_Image.cxx
index d747ac480..188665758 100644
--- a/src/Fl_GIF_Image.cxx
+++ b/src/Fl_GIF_Image.cxx
@@ -192,7 +192,7 @@ void Fl_GIF_Image::load_gif_(Fl_Image_Reader &rdr)
for (;;) {
int i = rdr.read_byte();
- if (i<0) {
+ if (rdr.error()) {
Fl::error("Fl_GIF_Image: %s - unexpected EOF", rdr.name());
w(0); h(0); d(0); ld(ERR_FORMAT);
return;
@@ -242,11 +242,23 @@ void Fl_GIF_Image::load_gif_(Fl_Image_Reader &rdr)
break; // okay, this is the image we want
} else {
Fl::warning("%s: unknown gif code 0x%02x", rdr.name(), i);
- blocklen = 0;
+ // though this an unknown block id, treat it like a GIF block
+ // in order to skip it correctly
+ ch = rdr.read_byte();
+ if (rdr.error()) blocklen = 0;
+ else {
+ ch = rdr.read_byte();
+ blocklen = rdr.error() ? 0 : ch;
+ }
}
// skip the data:
- while (blocklen>0) {while (blocklen--) {ch = rdr.read_byte();} blocklen = rdr.read_byte();}
+ while (!rdr.error() && blocklen>0) {
+ while (!rdr.error() && blocklen--) {
+ ch = rdr.read_byte();
+ }
+ blocklen = rdr.read_byte();
+ }
}
if (BitsPerPixel >= CodeSize)
diff --git a/src/Fl_Image_Reader.h b/src/Fl_Image_Reader.h
index 93f5edc0d..a8f0f5396 100644
--- a/src/Fl_Image_Reader.h
+++ b/src/Fl_Image_Reader.h
@@ -71,6 +71,7 @@ public:
// return the name or filename for this reader
const char *name() { return pName; }
+ int error() const { return pFile ? (feof(pFile) || ferror(pFile)) ? 1 : 0 : 0; }
private:
// open() sets this if we read from a file
I have also changed the handling of unknown block id's, otherwise the file would choke on every character read in with the |
Yes if we're reading from a file, but no if we're reading from memory. The advantage is that we can make it consistent for both cases. @wcout Great job! |
This is part 1 and a prerequisite for the fix of issue #271. It enables the user of this internal class (Fl_{BMP|GIF}_Image) to test for read errors and EOF (end of file) while reading. The method used to read data from memory got an optional third argument 'const long datasize = -1)' to limit the size of the memory block of data provided to the image reader. Default is -1 which means "unlimited" (backwards compatibility). Using only two arguments (w/o size limit) is deprecated and should only be done if the data size is not available.
Fix: Fl_Image_Reader::seek() would not clear the error flag when reading from memory.
|
The two commits mentioned above add error detection to Fl_Image_Reader as discussed above. I also added Fl_Image_Reader::tell() which can be used in error cases to help debugging (output the current memory/file offset in error messages). Still to do: use the new feature(s) in Fl_GIF_Image and Fl_BMP_Image. This is work in progress: I'm almost done with it but this needs some cleanup. I'm also still testing some special images that are either broken or can't be decoded by our code. FWIW: this image file (resize.gif) appears to be displayed with random colors or crashes the current (FLTK) GIF decoder. Here is an upscaled image as displayed by gimp. If someone feels inclined to find the bug... Review of the new Fl_Image_Reader code would be appreciated. |
|
FTR: I found the culprit in the image mentioned above (resize.gif) and our FLTK decoder. The problem is that Here is an image of the resultant display in FLTK with:
Next step: clean up code and commit... |
Albrecht-S
added
fixed
|
Note: the suggested modification in #271 (comment) in lines 242++ has not been applied. These lines of code concern data that is not necessarily followed by "data blocks" that can be skipped this way. The original file Hint: interpreting EDIT: ... except maybe terminating the decoding immediately rather than reading further... |
Well, that's debatable, but not really important. The main drawback is a (possible) thousands of error lines in the console. Terminating though may be not such a good idea, because it could break some special GIF's - who knows... |
|
Ah, and FWIW Line 83 in e0d630e |
This could happen if a read error or end of file was encountered.
Thanks to @wcout for finding this.
|
Referring to this comment: I agree it's debatable, but the error message "unknown GIF code 0x..." is in a context (the main read loop) where only a few different codes are to be expected. I read the "grammar" in the GIF spec (Appendix B) and I believe that finding other codes in this place is a serious error, i.e. a broken GIF file. I'll double check this though. In other places there's the notion of data (sub)blocks with a BTW, we don't (yet) parse "empty" GIF files correctly. This one is from the pygif test suite mentioned here:
The point here is that
Since we don't read "subsequent Data Streams" this is a moot point and we should exit gracefully - but I don't know yet what to do with the image object. Mark it as invalid ( |
After reading up now...you are right, this is not the place for block skipping. Thanks for your explanations!
The GIF viewers I tested with, treat it as an invalid GIF. This seems to me also the best solution in order to discern this case from image descriptor with zero width/height. |
|
I have tested all files on my hard drive with the new version and it works perfectly 👍 Hope you don't mind some nitpicking: As you certainly know this line is technically not necessary: Line 83 in 66c0bf4 Out of curiosity: |
|
Great, thanks for confirmation.
I usually use it anyway (but not always) to emphasize that it's only done when an image was allocated, but you're right, I know that.
I don't know either. It's not the official FLTK coding style, but sometimes contributed code doesn't conform to it. Private member variables should have a trailing underscore instead. Personally I think the 'p' is a bad choice, it could mean private, public, protected, pointer, ..., maybe more. I chose to keep the existing style and added the new variables with a 'p' prefix as well to avoid changing too much unrelated code but I'm not happy with this. BTW: the CamelCase variable names in Fl_GIF_Image are also not FLTK style. |
|
I'm leaving this issue open because I intend to work on the remaining open points:
|
Hope not pointer, for then |
BTW: I came across a file (it maybe was posted in one the FLTK groups previously), that has this error, but still loads ok then: |
IMHO it doesn't "load ok" in the FLTK viewer (fl_pixmap_browser): it displays an image, but that's not "ok". As you can see in the error messages, the first 24 lines seem to show the contents of what should be a global color table (8 gray values). The last line says "black_white.gif does not have a color table, using default" (which could be considered a "feature"). However, such a broken image could eventually lead to anything, including breaking an otherwise correct FLTK program (if it crashes later for whatever reason). So the only safe solution is IMHO to terminate decoding and ... All the other "image viewers" I tested (including Firefox and Chrome) don't display an image, some of them issue an error message, others don't. FTR: the broken image has only one bit wrong: the "Global Color Table Flag" is 0 (must be 1). I fixed it with a hex editor ( Image attached: |
|
The two remaining issues have been fixed in commit cc82b74:
This concludes the work on this issue and related code. I'm closing this issue now. Thanks for all contributions. |
...because the value of getc(pFile) is cast to unsigned char on return, so EOF (-1) becomes 255 (0xff) and the line in Fl_GIF_Image::load_ gif()
https://github.com/fltk/fltk/blob/master/src/Fl_GIF_Image.cxx#L195
will not detect the EOF condition.
A hand-crafted fake GIF shows the effect: endless loop.
bug.zip
The text was updated successfully, but these errors were encountered: