Fix integer overflow

[kevinbackhouse]

Fixes #1530.

The problem is a negative integer overflow in cc->size() - 8 (line 1193). In the test case, the value of cc->size() is 5.

I'll add a regression test to this PR shortly.

[clanmills]

I think your fix is more-or-less correct, so I will approve this. I'm happy if you wish to modify the PR after approval.

I've never looked at this code before. It's horrible. I suspect that if cc->size() is less than 28, we should parachute out of there. Something like:

if ((edX != edEnd || edY != edEnd || edO != edEnd) && cc->size >= 28 ) {
...
} else {
... remove some mysterious data ...
}

[kevinbackhouse]

@clanmills: None of the existing tests fail if we disallow cc->size() < 28, so that seems like a good choice. It makes the code simpler too.

[clanmills]

I'm going to open an issue for v1.00 to "properly" fix our 32/64 bit support.

We announced in v0.27 (and the the five dots), that the API for v0.28 (now to be called v1.00) will change. There's a horrible mix of size_t, uint32_t, long, int, uint64_t in the API. Now is the time to fix that. I suspect the correct way forward is to use uint64_t everywhere. We don't have to worry much about performance because Exiv2 is very fast. We can open and read the metadata from about 100+ files/second on my 6 year old MacMini.

[kevinbackhouse]

I made a few changes:

• Added a regression test
• Throw an exception if cc->size() < 28.
• Fixed a separate bug found by running make tests with a release ASAN build: there was a delete that should have been a delete[].

[clanmills]

@kevinbackhouse Two tiny comments:

python test symbol_name

In your python test code, we normally use $symbol_name for error messages and file names. This enables us to change the text of an error message without modify any test scripts. This is achieved via tests/suite_conf. On Monday, I added test_issue_1522.py for the first of the three CVEs. You'll see the python defines stderr as:

"""$exiv2_exception_message $filename:
$kerCorruptedMetadata
"""

Dan's test suite (in tests/system_tests.py), expands the three $symbol_names to match:

640 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/tests/bugfixes/github $ exiv2 -pS ../../../test/data/poc_1522.jp2  >/dev/null
Exiv2 exception in print action for file ../../../test/data/poc_1522.jp2:
corrupted image metadata
641 rmills@rmillsm1:~/gnu/github/exiv2/0.27-maintenance/tests/bugfixes/github $ 

Working on exiv2/exiv2
I find it easier to review PRs when they are branches in the Exiv2 repos. And, I can edit them (although I seldom do that).

[kevinbackhouse]

@clanmills: sorry, I forgot about the $symbol_name feature. I'll fix that now.

Regarding the branches, I don't have permission to create branches in the https://github.com/Exiv2/exiv2 repo. Or am I misunderstanding?

[kevinbackhouse]

I replaced the hard-coded string with $kerCorruptedMetadata. I wasn't sure what to do about "Could not write metadata to file" though. It's hard-coded in the source code here. Should that be replaced with a symbolic name?

[clanmills]

BTW, before I forget. Well done to spot that delete [] error. That's a common error that's hard to spot with the eye-ball. Did you find that with valgrind or something?

I wouldn't bother with that string "Could not write metadata to file". It's an odd-ball/one-off kind of thing. The super important ones are $exiv2 $filename $kerMumbleJumble

You're not a member! I've just given you write permission. I think you're good for the future. If you submit a new PR on exiv2/exiv2, I'll approve and merge the result.

I suspect the security guys are going to drop another CVE on us early next week. When I'm certain that both your code and @pydera's code are merged, I'll cut v0.27.4 RC2. Great end to a very successful week.

[kevinbackhouse]

The delete [] bug was found by ASAN on a release build. I was checking that my regression test would trigger the ASAN failure, but instead a different test failed.

[kevinbackhouse]

I'll close this PR and reopen it from a branch on the exiv2 repo.

[clanmills]

This testing stuff really is a good idea. I don't know why it isn't more popular! Much the same with the seldom used team-work!