Fix arithmetic operation overflow

[piponazo]

This change fixes the arithmetic operation overflow reported in #188.

I started to check how I could introduce the POC file into the repository and add a new test for the existing test suite (in bash). But then I noticed that I did not know how to update the file test/data/bugfixes-test.out and that it could be better to just add that test in the incoming python testing framework (#155 ).

[clanmills]

This looks like a good and simple fix. And I'm happy that it's based on unint32_t, so it should work fine on 32bit and 64bit builds. Good Work. Thank You.

Several comments:

1. We should also apply this test to length in Jp2Image::printStructure() at line 480 after we have discovered the length of the "box".
2. The code in tiffvisitor_int.cpp Replace jp2image with bmffimage #1525 is:

            if ((static_cast<uintptr_t>(baseOffset()) > std::numeric_limits<uintptr_t>::max() - static_cast<uintptr_t>(offset))
             || (static_cast<uintptr_t>(baseOffset() + offset) > std::numeric_limits<uintptr_t>::max() - reinterpret_cast<uintptr_t>(pData_)))
            {
                throw Error(59);
            }
            if (pData_ + static_cast<uintptr_t>(baseOffset()) + static_cast<uintptr_t>(offset) > pLast_) {
                throw Error(58);
            }
            pData = const_cast<byte*>(pData_) + baseOffset() + offset;

Can the code in tiffvisitor_int.cpp be simplified?

3 Is it possible to add a utility function (in error.cpp) to detect this condition and possibly throw the error.

Can you add the test file to test/data and update bugfixes-test.sh.

[D4N]

The code in tiffvisitor_int.cpp cannot be simplified because there are two additions involved and each of these can overflow, so it requires two checks. The if is a bounds check for the pointer pData which should stay too.

Concerning the utility function for this, I am going to make a PR for something like this.

[clanmills]

@D4N Thanks for the clarification concerning the code in tiffvisitor_int.cpp#1525

To add a test to bugfixes-test.sh (and most of the test suite bash scripts):
1 git add test/data/foo-file (add the test target file)
2 edit test/bugfixes-test.sh to perform the test
3 make bugfixes or (cd test && ./bugfixes-tests.sh)
... this will of course fail because your new test will defeat the reference file (test/data/bugfixes-test.out)
4 cp test/tmp/bugfixes-test.out test/data/bugfixes-test.out

Submit the files. I normally refer this as the "regression detector". You should have at least:
1 x new files (foo-file)
2 x modified files (test/data/bugfixes-test.sh and test/data/bugfix-test.out)

Usually you will also update C++ code library/sample code and when you change the test harness.

[D4N]

Can you rewrite this PR using the new overflow checked addition?

[piponazo]

Sure! I will do it as soon as I find some spare time ;)

[piponazo]

Guys, I re-rewrote yesterday the change using the new overflow functionality and I also added the reproducer to the old test suite. Let me know if the changes make sense (At least, CI is green 😃 )

[clanmills]

I've merged the code on MacOSX with the commands:

$ git clone https://github.com/exiv2/exiv2
$ cd exiv2
$ git checkout -b piponazo-bug188 master
$ git pull git://github.com/piponazo/exiv2.git bug188
--- vim started up requesting a comment (which it refused to save) ---
remote: Counting objects: 19, done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 19 (delta 14), reused 19 (delta 14), pack-reused 0
Unpacking objects: 100% (19/19), done.
From git://github.com/piponazo/exiv2
 * branch              bug188     -> FETCH_HEAD
Merge made by the 'recursive' strategy.
 src/actions.cpp                   |  64 ++++++++++++++++++++++++++++++++++++----------------------------
 src/jp2image.cpp                  |   6 ++++--
 test/bugfixes-test.sh             |   7 +++++++
 test/data/bugfixes-test.out       | Bin 1962825 -> 1962971 bytes
 test/data/poc_2017-12-12_issue188 | Bin 0 -> 84 bytes
 5 files changed, 47 insertions(+), 30 deletions(-)
 create mode 100644 test/data/poc_2017-12-12_issue188
507 rmills@rmillsmm:~/temp/exiv2/exiv2 $ mkdir build ; cd build ; cmake -DEXIV2_BUILD_UNIT_TESTS=On .. ; make ; make tests ; bin/unit_tests

It passed the tests (including g188).

513 rmills@rmillsmm:~/temp/exiv2/exiv2/build $ bin/exiv2 ../test/data/poc_2017-12-12_issue188 
std::overflow_error exception in print action for file ../test/data/poc_2017-12-12_issue188:
Overflow in addition
514 rmills@rmillsmm:~/temp/exiv2/exiv2/build $

Looking in tiffvisitor_int.cpp, I don't see any difference near in the overflow test about line 1550.