Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix long-running loop in QuickTimeVideo::sampleDesc #2424

Merged
merged 3 commits into from
Nov 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/exiv2/quicktimevideo.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,13 @@ class QuickTimeVideo : public Image {

private:
//! Variable which stores Time Scale unit, used to calculate time.
uint64_t timeScale_;
uint64_t timeScale_ = 0;
//! Variable which stores current stream being processsed.
int currentStream_;
int currentStream_ = 0;
//! Variable to check the end of metadata traversing.
bool continueTraversing_;
bool continueTraversing_ = 0;
//! Variable to store height and width of a video frame.
uint64_t height_, width_;
uint64_t height_ = 0, width_ = 0;

}; // QuickTimeVideo End

Expand Down
30 changes: 16 additions & 14 deletions src/quicktimevideo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "error.hpp"
#include "futils.hpp"
#include "quicktimevideo.hpp"
#include "safe_op.hpp"
#include "tags.hpp"
#include "tags_int.hpp"
// + standard includes
Expand Down Expand Up @@ -496,7 +497,8 @@ namespace Exiv2 {

using namespace Exiv2::Internal;

QuickTimeVideo::QuickTimeVideo(BasicIo::UniquePtr io) : Image(ImageType::qtime, mdNone, std::move(io)), timeScale_(1) {
QuickTimeVideo::QuickTimeVideo(BasicIo::UniquePtr io) :
Image(ImageType::qtime, mdNone, std::move(io)), timeScale_(1), currentStream_(Null) {
} // QuickTimeVideo::QuickTimeVideo

std::string QuickTimeVideo::mimeType() const {
Expand Down Expand Up @@ -860,8 +862,8 @@ void QuickTimeVideo::userDataDecoder(size_t size_external) {
void QuickTimeVideo::NikonTagsDecoder(size_t size_external) {
size_t cur_pos = io_->tell();
DataBuf buf(200), buf2(4 + 1);
unsigned long TagID = 0;
unsigned short dataLength = 0, dataType = 2;
uint32_t TagID = 0;
uint16_t dataLength = 0, dataType = 2;
const TagDetails *td, *td2;

for (int i = 0; i < 100; i++) {
Expand Down Expand Up @@ -1094,16 +1096,15 @@ void QuickTimeVideo::timeToSampleDecoder() {
DataBuf buf(4 + 1);
io_->readOrThrow(buf.data(), 4);
io_->readOrThrow(buf.data(), 4);
size_t noOfEntries, totalframes = 0, timeOfFrames = 0;
noOfEntries = buf.read_uint32(0, bigEndian);
size_t temp;
uint64_t totalframes = 0, timeOfFrames = 0;
const uint32_t noOfEntries = buf.read_uint32(0, bigEndian);

for (unsigned long i = 1; i <= noOfEntries; i++) {
for (uint32_t i = 0; i < noOfEntries; i++) {
io_->readOrThrow(buf.data(), 4);
temp = buf.read_uint32(0, bigEndian);
totalframes += temp;
const uint64_t temp = buf.read_uint32(0, bigEndian);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that because of the multiplication two lines later:

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

totalframes = Safe::add(totalframes, temp);
io_->readOrThrow(buf.data(), 4);
timeOfFrames += temp * buf.read_uint32(0, bigEndian);
timeOfFrames = Safe::add(timeOfFrames, temp * buf.read_uint32(0, bigEndian));
}
if (currentStream_ == Video)
xmpData_["Xmp.video.FrameRate"] = (double)totalframes * (double)timeScale_ / (double)timeOfFrames;
Expand All @@ -1114,16 +1115,17 @@ void QuickTimeVideo::sampleDesc(size_t size) {
size_t cur_pos = io_->tell();
io_->readOrThrow(buf.data(), 4);
io_->readOrThrow(buf.data(), 4);
size_t noOfEntries;
noOfEntries = buf.read_uint32(0, bigEndian);
const uint32_t noOfEntries = buf.read_uint32(0, bigEndian);

for (unsigned long i = 1; i <= noOfEntries; i++) {
for (uint32_t i = 0; i < noOfEntries; i++) {
if (currentStream_ == Video)
imageDescDecoder();
else if (currentStream_ == Audio)
audioDescDecoder();
else
break;
}
io_->seek(cur_pos + size, BasicIo::beg);
io_->seek(Safe::add(cur_pos, size), BasicIo::beg);
} // QuickTimeVideo::sampleDesc

void QuickTimeVideo::audioDescDecoder() {
Expand Down
Binary file added test/data/issue_2423_poc.mp4
Binary file not shown.
13 changes: 13 additions & 0 deletions tests/bugfixes/github/test_issue_2423.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# -*- coding: utf-8 -*-

from system_tests import CaseMeta, check_no_ASAN_UBSAN_errors

class issue_2423_QuickTimeVideo_sampleDesc_long_running(metaclass=CaseMeta):
url = "https://github.com/Exiv2/exiv2/issues/2423"
filename = "$data_path/issue_2423_poc.mp4"
commands = ["$exiv2 $filename"]
retval = [1]
stderr = ["""$exiv2_exception_message $filename:
$kerCorruptedMetadata
"""]
stdout = [""]
1 change: 1 addition & 0 deletions tests/regression_tests/test_regression_allfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ def get_valid_files(data_dir):
"issue_2377_poc.mp4",
"issue_2383_poc.mp4",
"issue_2393_poc.mp4",
"issue_2423_poc.mp4",
"2018-01-09-exiv2-crash-001.tiff",
"cve_2017_1000126_stack-oob-read.webp",
"exiv2-bug1247.jpg",
Expand Down