Exiv2

Hi Robin,
I will look into the code asap and report back if I get stuck anywhere.

Thanks for investigating this, Abhinav. I'll build and test this after work this evening.

Thanks, Abhinav. Very good and quick work. $ make testv -> "All testcases passed" on Kubuntu 12.10/64.

The test files now live at svn://dev.exiv2.org/svn/testdata/trunk and are downloaded on demand by test/Makefile when you run $ make testv in the <exiv2-dir>. I've submitted your revised file SVN:2922 (to testdata/trunk/video/video-test.out)

Have you reviewed the buffer size changes? I'm very nervous about line#655 in quicktimevideo.cpp

    void QuickTimeVideo::decodeBlock()
    {
        const long bufMinSize = 4;
        DataBuf buf(bufMinSize);
        unsigned long size = 0;
        buf.pData_[4] = '\0' ;    // really ?  Are you sure?

        std::memset(buf.pData_, 0x0, buf.size_);

Are you confident that we don't have a buffer overflow?

Robin Mills wrote:

Abhinav. Apologies for spoiling your day. The tests are not passing on cygwin. Mac and Unix are OK. I haven't built/tested on the Windows/DevStudio builds. I attach cygwin.out.

Hi Robin,
It seems that the code in the cygwin repository is old. I have removed the lines that would have created the following error report. Please check if the test was applied on the latest updated code.

Abhinav

You're not going to believe this. I totally reran the build and test. I pulled down a fresh copy of the trunk and built him with ./configure. Same result! There's something here. So I did a cmake build and it passes!

I'm going away for the weekend, however I'll be home on Sunday afternoon. If you don't have any ideas about this, I'll investigate. I'm sure it's nothing very much.

Thanks for looking at this so quickly.

Robin

Hi Robin,
Hope you have an awesome weekend. :)

Thanks for the info.
I am sure that I have removed the lines that were mentioned in the test output report.

There is one thing that I would like to point out here as well. For some reasons, I am not able to install exiv2 by normal method.

I mean in Linux, I tried the following commands -
[1] make config
[2] ./configure
[3] make -j4
[4] sudo make install

The compilation went on fine, but then installation showed an error -

libtool: install: /usr/bin/install -c -m 644 .libs/libexiv2.12.dylib /usr/local/lib/libexiv2.12.dylib
libtool: install: (cd /usr/local/lib && { ln -s -f libexiv2.12.dylib libexiv2.dylib || { rm -f libexiv2.dylib && ln -s libexiv2.12.dylib libexiv2.dylib; }; })
libtool: install: /usr/bin/install -c -m 644 .libs/libexiv2.lai /usr/local/lib/libexiv2.la
libtool: install: /usr/bin/install -c -m 644 .libs/libexiv2.a /usr/local/lib/libexiv2.a
libtool: install: chmod 644 /usr/local/lib/libexiv2.a
libtool: install: ranlib /usr/local/lib/libexiv2.a
----------------------------------------------------------------------
Libraries have been installed in:
   /usr/local/lib

If you ever happen to want to link against installed libraries
in a given directory, LIBDIR, you must either use libtool, and
specify the full pathname of the library, or use the `-LLIBDIR'
flag during linking and do at least one of the following:
   - add LIBDIR to the `DYLD_LIBRARY_PATH' environment variable
     during execution

See any operating system documentation about shared libraries for
more information, such as the ld(1) and ld.so(8) manual pages.
----------------------------------------------------------------------
../config/mkinstalldirs /usr/local/bin
../config/mkinstalldirs /usr/local/lib/pkgconfig
/usr/bin/install -c -m 644 ../config/exiv2.pc /usr/local/lib/pkgconfig/exiv2.pc
../config/mkinstalldirs /usr/local/bin
libtool: install: /usr/bin/install -c bin/exiv2 /usr/local/bin/exiv2
install: bin/exiv2: No such file or directory
make[1]: *** [install] Error 71
make: *** [install] Error 2

This error occurs when I execute the command

sudo make install

I think there is some modification in the Makefile or something, that may be the cause.

Note - The cmake method is working fine and installation is also working perfectly.

I haven't noticed what you've described. Do you have a directory /usr/local/bin ? Maybe something's disturbed ../config/mkinstalldirs which I think is create by the autotools. I'll also have a look at that on Sunday.

Well, I hope you also have a nice week-end. I'm off camping/running with my Adobe buddies. We go camping/running this weekend every year: http://clanmills.com/2011/BigSur/Saturday/

Cool. :)

Yeah now the bug seems fixed. I have downloaded and tested the new code and it installed successfully.
Thanks. :)

Abhinav

Everything looks good. May I ask you again about possible buffer overflows:

    void QuickTimeVideo::decodeBlock()
    {
        const long bufMinSize = 4;
        DataBuf buf(bufMinSize);
        unsigned long size = 0;
        buf.pData_[4] = '\0' ;    // really ?  Are you sure?

        std::memset(buf.pData_, 0x0, buf.size_);

Are you OK with this?

Hi Robin,

const long bufMinSize = 4;
DataBuf buf(bufMinSize);
buf.pData_[4] = '\0' ;

Actually, the buffer size can't be changed and we cant include "\0" into the data buffer as well, because that would lead to conversion discrepancies, especially in case of Integer or Float.
I can't say it for sure that it will never create any buffer overflow, but it has worked fine with all the test cases that I took under consideration. I have also applied checks at places in the code to prevent it from breaking down.
So I think it would be safe to consider that it won't cause any buffer overflow kind of problem.

Would it hurt to change:

DataBuf buf(bufMinSize);

DataBuf buf(bufMinSize+1);

Robin Mills wrote:

Would it hurt to change:
[...]
to:
[...]
And review similar changes in Shawn's submission?

Yes, it will lead to a lot of misinterpretations(when the buffer is interpreted to integer or float) and may even break the code.
The data generated by interpreting the buffer would not be accurate and would also cause all of the video tests to fail.

One of the prominent reason that I could think about is -
[1] The binary data is stored either in big endian or small endian format. So the interpretation differs from case to case. So if the buffer size is increased to five (instead of four), the interpretation may be correct for little endian, but may fail in the case if big endian or vice versa. The conversion function would be expecting four byte, where it would get five bytes instead and then the result may not be well defined.

[2] The '\0' is important and can't be removed from the code either. This is because sometimes the buffer is interpreted as character string. And it can't be placed inside the buffer, cause then when a four character string is stored in the buffer, it will get overwritten, and will be useless.