Crashing on file with multiple Exif.Image.Orientation fields, one empty

A user of our project (Shotwell) reported a crasher when importing a certain file. I've been able to determine exactly where it's crashing, and I think I know why, but I'm fuzzy on a couple of things.

With exiv2 I can see that this file has two Exif.Image.Orientation fields, one set to zero (an invalid number), the other 1 (Top-Left). This is the code in our GObject wrapper library that's causing the crash:

Exiv2::ExifKey std_key ("Exif.Image.Orientation");
        it = exif_data.findKey (std_key);
        if (it != exif_data.end ()) {
            GExiv2Orientation orientation = (GExiv2Orientation) it->toLong ();
            // ...
        }

The crash specifically happens at it->toLong(). gdb reports:

Program received signal SIGSEGV, Segmentation fault.
    [Switching to Thread 0xaea69b70 (LWP 3795)]
    0x0027d7b1 in Exiv2::ValueType&lt;unsigned short&gt;::toLong(long) const ()
       from /usr/local/lib/libexiv2.so.9

I inserted some debug code and it appears the Exifdatum object being returned by the iterator has a count() of zero. As a test I added code to keep iterating until an Exifdatum with a count > 1 is found or the end of the list is reached. With this code in place, all works fine.

Is this normal? Should I be checking the count of all Metadatum fields before accessing them? Or is this a bug?

Sorry for the long-winded question, still working through the problem.

Replies (4)

RE: Crashing on file with multiple Exif.Image.Orientation fields, one empty - Added by Andreas Huggel about 11 years ago

The application needs to ensure that there is a value before it calls toLong(). As you said, the usual way to do this is by adding count() > 0 to the test.

Andreas

RE: Crashing on file with multiple Exif.Image.Orientation fields, one empty - Added by Jim Nelson about 11 years ago

Could this be added to the documentation? I didn't see it anywhere. It's a pretty vital thing to know.

RE: Crashing on file with multiple Exif.Image.Orientation fields, one empty - Added by Andreas Huggel about 11 years ago

The documentation of Exifdatum::toLong() says the behaviour is undefined if the value doesn't have the component to convert. I'd be very happy to make this more user-friendly, you're not the first who's wasting time on this, but I don't see a better approach. What should toLong() return if there is no value to convert to a long in the first place?

I just noticed the doc also says it returns -1 if the value is not set at all. That looks like a bug, it should probably also crash instead. Otherwise, when the client gets -1 it has to figure out what that means. Even more so as this is really just a programming issue and doesn't depend on the metadata, unlike the case above.

The best I can think of is to add assertions and comments in this code to reduce the time needed for debugging.

Andreas

RE: Crashing on file with multiple Exif.Image.Orientation fields, one empty - Added by Jim Nelson about 11 years ago

My apologies! I wasn't looking closely enough at the text. You are correct, that is in there. The -1 return value might throw someone off if they don't know the difference between an undefined value and a non-existent one.

In either case, a segfault doesn't seem right to me. I would encourage you to use an assertion of some kind.

(1-4/4)

Project

General

Profile

Exiv2