STR #2284: Bad return value handling from "getc" in Fl_BMP_Image c'tor

STR #2284

Application:	FLTK Library
Status:	5 - New
Priority:	3 - Moderate, e.g. unable to compile the software
Scope:	3 - Applies to all machines and operating systems
Subsystem:	Image Support
Summary:	Bad return value handling from "getc" in Fl_BMP_Image c'tor
Version:	1.4-feature
Created By:	conveycj
Assigned To:	Unassigned
Fix Version:	Unassigned
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

Post File

No files

Trouble Report Comments:

Post Text

#1	conveycj 13:11 Nov 06, 2009

In FLTK 1.1.10rc3, in Fl_BMP_Image.cxx line 375: There are two variables of type "uchar" that receive the return value of a "getc" function call. This could be a problem. "getc" returns -1 to endicate EOF. When storing -1 into a uchar, it becomes 255. This is probably bad for a variety of reasons. A decent writeup is here: https://www.securecoding.cert.org/confluence/display/seccode/FIO34-C.+Use+int+to+capture+the+return+value+of+character+IO+functions

#2	greg.ercolano 00:35 Nov 07, 2009

Indeed the code should be rewritten to check for errors during file reading and fail more gracefully. In fact, there are many places in the code where getc() is used badly.. just run in the fltk/src directory grep 'getc(' *.cxx ..and indeed you'll see the problem in much of the code. I'd suggest using fread() instead which has very clear separation of data and status info.

#3	matt 12:54 Nov 07, 2009

Yes, this is a known issue. None of the image functions does much error checking is any at all. Actually, there is not even a mechanism to reliably test if an image was actually loaded. This is nothing we can fix in 1.1, so I promoted this to 1.3

#4	matt 12:49 Feb 26, 2010

I am upping this to 1.4 until we have decided on some common error handling for all image formats.

#5	matt 16:13 Jan 14, 2023

As of 1.4, we no longer use getc to read BMP images, but have our own reader class. The code still doesn't;t check for all possible errors, and not for EOF with every call either, but at least we now return 0 instead of -1 (255), which is probably less harmful. See https://github.com/fltk/fltk/pull/228 for general BMP error checking.