Fix long-running loop in QuickTimeVideo::sampleDesc

[kevinbackhouse]

Fixes: #2423

The most important change here is to add a break to the loop in QuickTimeVideo::sampleDesc. The second important change is to make sure that all the private fields of QuickTimeVideo are initialized. I also did some cleanup of the types of some of the local variables.

[piponazo]

Feel free to do something about the comment I have made or not, it is just a suggestion. Everything else looks good!

[piponazo]

Does it makes sense to treat the variable temp as uint64_t when we are reading here a uint32_t? Since we are talking about frames in a video, I doubt we will ever need to process such a large video to need uint64_t for the totalFrames :)

[kevinbackhouse]

I did that because of the multiplication two lines later:

exiv2/src/quicktimevideo.cpp

Line 1106 in cf15cc5

timeOfFrames += temp * buf.read_uint32(0, bigEndian);

If temp is a uint64_t then the multiplication can't overflow. (Probably not a big deal if it does overflow, but I figure we might as well avoid it.)

[piponazo]

Would it make sense to use here some of the SafeXXX operations then to throw in case of overflow?

[kevinbackhouse]

Unfortunately, we don't currently have a Safe::multiply. But I think it makes sense to replace this += with a Safe::add, which will make this code safe. I'll add a follow-up commit.

[codecov]

Codecov Report

Merging #2424 (9d044d3) into main (861625c) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main    #2424   +/-   ##
=======================================
  Coverage   64.54%   64.54%           
=======================================
  Files         119      119           
  Lines       21112    21114    +2     
  Branches    10420    10421    +1     
=======================================
+ Hits        13626    13629    +3     
  Misses       5324     5324           
+ Partials     2162     2161    -1     

Impacted Files	Coverage Δ
src/quicktimevideo.cpp	59.08% <85.71%> (+0.21%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hassec]

@kevinbackhouse looks like conan-io/conan-center-index#14259 caused the conan problem here.

removing:

exiv2/conanfile.py

Lines 36 to 37 in cf15cc5

	if self.settings.build_type == "Debug":
	self.options['gtest'].debug_postfix = ''

should do the trick

[piponazo]

@kevinbackhouse looks like conan-io/conan-center-index#14259 caused the conan problem here.

removing:

exiv2/conanfile.py

Lines 36 to 37 in cf15cc5

	if self.settings.build_type == "Debug":
	self.options['gtest'].debug_postfix = ''

should do the trick

😱 ! I'll take a look to this issue

Pull request has been modified.

[piponazo]

When you rebase this branch on top of master, the failing CI jobs should turn green (Except for the cygwin one 🙈 )

Pull request has been modified.

[kevinbackhouse]

Is this one ok to merge?