Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#22 manipulating thumbnail in Canon image causes crash

closed-works-for-me
libexif (61)
5
2005-03-23
2004-10-26
Ben Liblit
No

When exif is asked to insert or remove a thumbnail in a Canon image, it may fail to load MakerNote data. Subsequently saving the modified image file causes a crash as exif tries to use MakerNote data which was never properly loaded. We are seeing this crash using exif-0.6.9 with libexif-0.6.11.

This bug was uncovered as part of the Cooperative Bug Isolation Project <http://sampler.cs.berkeley.edu/>, which attempts to find bugs by discovering statistically significant differences in the behavior of good and bad runs. This bug, along with several others, was discovered by running exif with random command line flags on random elements of a large corpus of JPEG images. We observed a 7% crash rate across the entire experiment. The bug we describe here accounts for 1% of those failures (0.07% overall). That rate could easily be higher, of course, if Canon images formed a larger proportion of the image corpus.

["We" here refers to the members of the Cooperative Bug Isolation research team at Stanford University and the University of California, Berkeley. Though just one of us is submitting this report, it is the product of a joint research effort to which all members made invaluable contributions.]

We are attaching a test file, "image3.jpg", to this report. The command "exif -r image3.jpg" causes a segmentation fault. The (abstract) stack trace looks as follows:

main
exif_data_save_data
exif_data_save_data_content
exif_data_save_data_content
exif_data_save_data_entry
exif_mnote_data_save
exif_mnote_data_canon_save
memcpy

The stack trace alone doesn't help the programmer much in determining the cause of the bug. Our Cooperative Bug Isolation analysis tools direct the programmer's attention to the following predicate:

"o + s > buf_size" in function exif_mnote_data_canon_load in file libexif-0.6.10/libexif/canon/exif-mnote-data-canon.c

This predicate is highly predictive of failure. That is, when the above predicate is true, exif is much more likely to crash.

Knowing that exif_mnote_data_canon_load is implicated in the bug lets us extend the stack trace to the following sequence of calls:

main
exif_loader_get_data
exif_data_load_data
exif_mnote_data_canon_load
exif_data_save_data
exif_data_save_data_content
exif_data_save_data_content
exif_data_save_data_entry
exif_mnote_data_save
exif_mnote_data_canon_save
memcpy

The programmer can easily infer that exif_mnote_data_canon_load and exif_mnote_data_canon_save are "complementary": an early exit from the former can affect the latter.

The code of exif_mnote_data_canon_load looks as follows:

exif_mnote_data_canon_load(...)
{
...
for (i = 0; i < c; i++) {
...
n->count = i + 1;
...
if (o + s > buf_size) return;
n->entries[i].data = malloc (sizeof (char) * s);
...
}
...
}

Notice that whenever the predicate "o + s > buf_size" is true, n->entries[i].data is not malloc'ed.

The code of exif_mnote_data_canon_save looks as follows:

for (i = 0; i < n->count; i++) {
...
memcpy (*buf + doff, n->entries[i].data, s);
...
}

Now, it is easy to see that:

1. n->entries[i]->data must be NULL for some i (specifically, the i corresponding to the last iteration of the for-loop in exif_mnote_data_canon_load), and

2. this NULL pointer must be dereferenced in the 2nd argument of the call to memcpy in the corresponding iteration of the for-loop in exif_mnote_data_canon_save, thereby causing memcpy to crash.

The fix seems to be for exif_mnote_data_canon_load to return an error code when the predicate "o + s > buf_size" is true instead of returning silently, and to check for this error code in callers of exif_mnote_data_canon_load.

Discussion

  • Ben Liblit
    Ben Liblit
    2004-10-26

    test image demonstrating the bug

     
    Attachments
  • Ben Liblit
    Ben Liblit
    2004-10-26

    Logged In: YES
    user_id=2116

    Unfortunately it looks like SourceForge's bug system ate up the
    leading whitespace in our abstract stack traces. Lack of indentation
    makes them hard to read. Using underscores instead of spaces,
    the initial stack trace at the point of failure should look like
    this:

    __ main
    ____ exif_data_save_data
    ______ exif_data_save_data_content
    ________ exif_data_save_data_content
    __________ exif_data_save_data_entry
    ____________ exif_mnote_data_save
    ______________ exif_mnote_data_canon_save
    ________________ memcpy

    Once we identify a strong crash predictor in exif_mnote_data_canon_load we can extend that stack trace to the following sequence of calls leading up to the failure:

    __ main
    ____ exif_loader_get_data
    ______ exif_data_load_data
    ________ exif_mnote_data_canon_load
    ____ exif_data_save_data
    ______ exif_data_save_data_content
    ________ exif_data_save_data_content
    __________ exif_data_save_data_entry
    ____________ exif_mnote_data_save
    ______________ exif_mnote_data_canon_save
    ________________ memcpy

    Hopefully that's more readable. Sorry about the confusion!

     
    • labels: --> 722605
     
    • labels: 722605 --> libexif
     
  • Logged In: YES
    user_id=59853

    Thanks for the report.

    It looks like this problem has been fixed since.

    At least I cannot reproduce it with libexif 0.6.13 CVS HEAD
    and exif 0.6.11 CVS HEAD.

    Test case added to test suite.

     
    • assigned_to: nobody --> hun
    • status: open --> pending-fixed
     
  • Ben Liblit
    Ben Liblit
    2005-03-21

    Logged In: YES
    user_id=2116

    I confirm that the bug no longer occurs as originally
    described using the latest CVS libexif sources. Stepping
    through exif_mnote_data_canon_load(), I see that the "if (o
    + s > buf_size) return;" statement no longer causes an early
    return. Nothing before this point has changed in
    exif_mnote_data_canon_load(), though, so presumably other
    changes elsewhere have caused the "(o + s > buf_size)" test
    to no longer be true here.

    exif_mnote_data_canon_load() still has no provision for
    returning an error code, which makes me worry that this bug
    could easily return if some other image arose which caused
    the "(o + s > buf_size)" test to be true again.

    Until such an image appears, I suppose we can optimistically
    consider this bug fixed, and this report closed. Or perhaps
    you'd rather use "Works For Me" as the resolution? Well,
    I'll leave it "Fixed" for now and let hun or some other
    developer change it if they prefer.

     
  • Ben Liblit
    Ben Liblit
    2005-03-21

    • status: pending-fixed --> closed-fixed
     
    • status: closed-fixed --> open-works-for-me
     
  • Logged In: YES
    user_id=59853

    OK, then I'll reopen this bug with solution "worksforme"
    just to have something to track the problem in
    exif_mnote_data_canon_load().

     
    • status: open-works-for-me --> closed-works-for-me