Fl_Shared_Image: use of unitialized data on invalid input

[fire-eggs]

FLTK 1.4.x, 202110409 snapshot.
Linux Mint 20.1 MATE

Running Valgrind on a program derived from pixmap_browser.cxx. Due to a bug in my code, I am occasionally passing a folder path to Fl_Shared_Image::get(). Valgrind gives me the following error [earlier stack elided]:

==2731837== Conditional jump or move depends on uninitialised value(s)
==2731837==    at 0x139B0A: Fl_Shared_Image::reload() (Fl_Shared_Image.cxx:266)
==2731837==    by 0x139685: Fl_Shared_Image::Fl_Shared_Image(char const*, Fl_Image*) (Fl_Shared_Image.cxx:145)
==2731837==    by 0x13A1A2: Fl_Shared_Image::get(char const*, int, int) (Fl_Shared_Image.cxx:462)
==2731837==    by 0x1263F3: load_file(char const*) (pixmap_browser.cxx:213)
==2731837==    by 0x1264F0: file_cb(char const*) (pixmap_browser.cxx:258)

The problem is not at the line shown above, but in this following chunk of code [lines 258-263]:

  if ((fp = fl_fopen(name_, "rb")) != NULL) {
    if (fread(header, 1, sizeof(header), fp)==0) { /* ignore */ }
    fclose(fp);
  } else {
    return;
  }

In the case of a folder path, the fread() call to populate header returns 0, and header has not been initialized. The code proceeds to use header in an invalid state.

I suggest something like this instead:

  if ((fp = fl_fopen(name_, "rb")) != NULL) {
    int count = fread(header, 1, sizeof(header), fp);
    fclose(fp);
    if (count==0) return;
  } else {
    return;
  }

I've tried the above change and Valgrind no longer complains about uninitialized values.

[Albrecht-S]

Good finding. Looking at the code below that point it seems that the count variable would be useful to check the length of the comparisons as well. In that case count would have to be declared outside the if block, of course. Shell code to (supposedly) trigger another error:

$ echo -n "#def" > x.jpg

Then run pixmap_browser with valgrind and open x.jpg.
In my quick test valgrind didn't complain though, but the comparison if (memcmp(header, "#define", 7) == 0) would (AFAICT) access uninitialized memory. EDIT: In fact it does, there must have been an issue with my first test. These tests should compare the size (count) before the actual comparison like

  if (count >= 7 && memcmp(header, "#define", 7) == 0) // XBM file

The same for all following comparisons, and the image handler calls in line 273 img = (handlers_[i])(name_, header, sizeof(header)); should supposedly be passed count rather than sizeof(header). However, I didn't follow that code path (yet) ...

[Albrecht-S]

FTR: I'm working on a fix and some more related documentations. It's almost ready and will likely be committed and pushed soon.

@fire-eggs Kevin, your proposed fix is included and extended as written above.

[Albrecht-S]

@fire-eggs Kevin, please test and confirm that commit f9e8ef0 fixes the issue.

It's a much larger fix because I had to dig deeper to find even more issues in FLTK's image check functions. I also improved documentation (see commit log).

[fire-eggs]

Definitely much improved!

• Previous version of FLTK: Valgrind issued 20 (!) "Conditional jump or move" error messages.
• Commit f9e8ef0: valgrind issues NO error messages!

When I make time, I'll look over the various changes, but as far as this particular issue is concerned, it is FIXED.

I'll have to make more bugs in my code ;-}

[Albrecht-S]

@fire-eggs Thanks for the confirmation (two months ago), I must have missed this. Closing the issue now.