Exiv2

Auke

Very nice photo.

I'm not sure I've understood the issue here. When I run your file through exiv2 -pt \Baby.jpg, I get three warnings:

C:\Users\rmills\Pictures>exiv2 -pt \Baby.jpg | wc
Warning: Directory Photo, entry 0x927c has unknown Exif (TIFF) type 39; setting type size 1.
Warning: Directory Photo, entry 0xa003 has unknown Exif (TIFF) type 67; setting type size 1.
Warning: Directory Photo, entry 0xa408 has unknown Exif (TIFF) type 35; setting type size 1.
    168   34441  128691

As you can see, exiv2 didn't crash and found 168 tags. The warnings are comming from TiffReader::readTiffEntry(). I set a breakpoint in TiffMnEntry::doCount() and the program didn't go there.

Robin

Hi Robin,

Indeed, when running the Exiv2 executable, no errors occur.
But please try to run this short piece of code:

Image::AutoPtr srcimg=ImageFactory::open("File_281.jpg");
   srcimg->readMetadata();
   ExifData &ed=srcimg->exifData();
   Blob blob;
   ExifParser::encode(blob, littleEndian, ed);

On my system (Win7 x64, 16gb, VC2010, latest exiv2 trunk), it does crash at line 1037 of tiffcompsite.cpp.
I tried various older versions of exiv2. Releases prior to r2664 do not crash here.

I hope this helps to identify the problem.
Thanks and greetings,
Auke

Hi Robin,

A quick and dirty solution is this:

uint32_t TiffMnEntry::doCount() const
    {
        if (!mn_) {
            return TiffEntryBase::doCount();
        }
        // Count of IFD makernote in tag Exif.Photo.MakerNote is the size of the
        // Makernote in bytes
          if (TiffType()==NULL) return 0; // <= ADDED
        assert(tiffType()  ttUndefined || tiffType()  ttUnsignedByte || tiffType() == ttSignedByte);
        return mn_->size();
    }

But probably you have a more elegant way of handling this (minor) issue...

Greetings,
Auke

Auke

Ah. Now I understand. I'm not convinced by your fix - because it didn't work for me! I have TiffType() with a value of 39 on my msvc64/Win32/Debug build (using MSVC2005). The valid values are 1..13 (see tiffcomposite_int.hpp)

I think (but I am not certain) that a better fix might be:

uint32_t TiffMnEntry::doCount() const
{
    if (!mn_) {
        return TiffEntryBase::doCount();
    }
    // Count of IFD makernote in tag Exif.Photo.MakerNote is the size of the
    // Makernote in bytes
    TiffType tt = TiffType(); 
    return  (tt == ttUndefined || tt == ttUnsignedByte || tt == ttSignedByte) ? mn_->size() : 0 ;
}

Can you try this please? I'm going to do more code analysis and testing before deciding what to do. I see that the code is copying TiffComponents, however the cast below conjures up pDirEntry->tiffType_ with the value of 39.

I don't want to remove as assert without a very good reason, so I want to be quite certain about why this value is out of range.

    uint32_t TiffDirectory::writeDirEntry(IoWrapper&     ioWrapper,
                                          ByteOrder      byteOrder,
                                          int32_t        offset,
                                          TiffComponent* pTiffComponent,
                                          uint32_t       valueIdx,
                                          uint32_t       dataIdx,
                                          uint32_t&      imageIdx) const
    {
        assert(pTiffComponent);
        TiffEntryBase* pDirEntry = dynamic_cast<TiffEntryBase*>(pTiffComponent);
        assert(pDirEntry);

I hope that helps. I will appreciate your thoughts and any feedback you'd like to provide.

Robin

My fix is hopeless. It fails the test suite on the Mac. (I prefer to debug/develop on Windows and test on Mac or Unix). I'm getting the tiffType() value 39 on the Mac, so I think its coming from the file. It's very unlikely that a simple uninitialized value would be the same on Windows and the Mac.

Your fix does work with your code and file, however your fix also causes the test suite to fail.

I hadn't understood there's a TiffType() and a tiffType() function. I'll do more work on this in the next few days.

I'd appreciate any thoughts you have about this.

Robin

Hi Robin,

Me writing TiffType() iso tiffType() was a typeing mistake :(
Indeed, using the proper tiffType() function, 39 is returned, which is a unknown value...

At the moment, I also do not know how to fix this properly.
Will look into it more next weekened...

Thanks and greetings,
Auke

Auke

I've looked at this again. The failures in our test suite were my error. I modified exifprint.cpp to contain your code. I didn't remember that exifprint is used by the suite.

There are two typeType() functions.

• tiffType()
  is a return on tiffType_
  which we agree is 39 and out of bounds for your file.
• TiffType() always returns NULL
  I don't know where TiffType() is defined.
  MSVC doesn't can't find it, and the debugger won't step into it.

So, it seems to me that:

if ( TypeType() == NULL ) return 0;

is effectively:

return 0;

This will introduce a bug in the code by preventing mn_->size() from ever being called.

So, that leaves me with a proposal:

    uint32_t TiffMnEntry::doCount() const
    {
        uint32_t result = 0 ;
        if (!mn_) {
            result = TiffEntryBase::doCount();
        } else {
            int tt = tiffType();
            if ( ttMin <= tt && tt <= ttMax ) {
                // Count of IFD makernote in tag Exif.Photo.MakerNote is the size of the Makernote in bytes
                assert(tt == ttUndefined || tt == ttUnsignedByte || tt == ttSignedByte);
                result = mn_->size();
            }
        }
        return result;
    }

And you should add update tiff composite_int.hpp

    const TiffType ttMin              = 1; //!< Exif BYTE type
    const TiffType ttUnsignedByte     = 1; //!< Exif BYTE type
    ...
    const TiffType ttTiffIfd          =13; //!< TIFF IFD type
    const TiffType ttMax              =13; //!< Exif BYTE type

I'm unwilling to submit this change. We know the data is out of bounds and we're evading the assert which would catch the error.

There's nothing to prevent you from "patching" your version of the library with this, or any other fix. I you decide to patch, I'd like to ask you to do one of the following:

1 Run our test suite on your build.
or
2 Send me your patch and I'll test it for you.

If you adopt my proposal (involve ttMin and ttMax), I have successfully run our test suite using that fix.

If you're able to explore this matter further at the weekend, I'm willing to reconsider my position on this matter. Perhaps you'll discover a solid fix that preserves the assert and forgives your file with good cause. If you find such a fix, not only will I submit - I will also add your file and appropriate code to our test suite.

Robin

Hi Robin,

Thanks for your code. It looks good!

As my application can and will run into images with (partly) corrupted exif headers, and MUST continue, I would like to use this patch.
I ran my own test code using the patched exiv2 library and all went fine.

I understand that this fix cannot be submitted, as it will indeed circumvent the assert statement.
At the moment I do not see a better alternative (for my application, that is).

Which test code would you like me to run? (there are many test projects in the solution file...)

Thanks and greetings,
Auke

If you're using my code, you're good to go. To run the tests, you'll need cygwin because the suite is written in bash. In the topmost directory, run $ make tests. To test a specific msvc64 build, you'll need to use the environment string EXIV2_BIN. For example $ make tests EXIV2_BINDIR=$PWD/msvc64/bin/Win32/ReleaseDLL

Take care, Cygwin is case sensitive about path/filenames.

On Mac/Linux, it runs in about 1 minute. On Cygwin, it's about 5 minutes.

If all is good, it will produce a short output mostly saying 'all tests passed'.

I ought to put a document on the wiki about this. I'll do that next week.

Robin

Auke

I've added a Wiki page to cover the test suite:

http://dev.exiv2.org/projects/exiv2/wiki/How_do_I_run_the_test_suite_for_Exiv2

Comments about the page are welcome (on this thread).

Robin