summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorA. Cody Schuffelen <schuffelen@google.com>2020-11-23 16:54:33 -0800
committerA. Cody Schuffelen <schuffelen@google.com>2020-11-24 13:37:49 -0800
commit6157fd6b7de3f9e7f53fc1d812efd8bd47791d7b (patch)
treef30799a67520f756b0d2fe5aefa11dcad5aeeda0
parentdcaf20a12f8fa5680c5c9a03eafff9e8c7fb8a83 (diff)
downloadlibwebm-6157fd6b7de3f9e7f53fc1d812efd8bd47791d7b.tar.gz
mkvmuxer: Keep Segment in a good state when frame writes fail
The problem in WriteFramesAll manifests as a null pointer dereference, while the problem in WriteFramesLessThan manifests as a use-after-free. When multiple audio frames are queued in the Segment and a call to WriteFramesAll or WriteFramesLessThan fails partway through, there may be null frames left in the Segment frame queue. This is fixed by dropping the invalid frames and continuing so that the queue is in a good state after a call, even if it fails. Found through fuzzing mkvmuxer. Minimal reproduction case for issue with WriteFramesAll. ``` #include <stdint.h> #include "mkvmuxer/mkvmuxer.h" #include "mkvmuxer/mkvwriter.h" int main() { mkvmuxer::MkvWriter writer; writer.Open("file.mkv"); mkvmuxer::Segment segment; segment.Init(&writer); uint64_t video_track = segment.AddVideoTrack(1, 1, 0); uint64_t audio_track = segment.AddAudioTrack(1, 1, 0); uint8_t data[] = {0}; segment.AddFrame(data, 1, audio_track, 395136991333ULL, false); segment.AddFrame(data, 1, audio_track, 18374686479674129407ULL, false); segment.AddFrame(data, 1, video_track, 18446462628926195463ULL, false); segment.Finalize(); } ``` The second audio frame has an invalid timestamp that is only caught in WriteFrame from mkvmuxerutil.cc , causing WriteFramesAll to fail on writing the second audio frame when the first video frame forces a new cluster. WriteFramesLessThan has a similar pattern, where failing partway through a write can leave the Segment in a bad state. Bug: b/174070314 Test: clang++ *.cc repro.cpp -I.. -o repro -fsanitize=address -g && ./repro Change-Id: I05c707477e454752776c7571aff9f24f22195d55 (cherry picked from commit 97f8074dc9cc5ca0263325c16340ff0f4fcfc212)
-rw-r--r--mkvmuxer/mkvmuxer.cc24
1 files changed, 16 insertions, 8 deletions
diff --git a/mkvmuxer/mkvmuxer.cc b/mkvmuxer/mkvmuxer.cc
index 5120312..5926a15 100644
--- a/mkvmuxer/mkvmuxer.cc
+++ b/mkvmuxer/mkvmuxer.cc
@@ -4105,12 +4105,16 @@ int Segment::WriteFramesAll() {
// places where |doc_type_version_| needs to be updated.
if (frame->discard_padding() != 0)
doc_type_version_ = 4;
- if (!cluster->AddFrame(frame))
- return -1;
+ if (!cluster->AddFrame(frame)) {
+ delete frame;
+ continue;
+ }
if (new_cuepoint_ && cues_track_ == frame->track_number()) {
- if (!AddCuePoint(frame->timestamp(), cues_track_))
- return -1;
+ if (!AddCuePoint(frame->timestamp(), cues_track_)) {
+ delete frame;
+ continue;
+ }
}
if (frame->timestamp() > last_timestamp_) {
@@ -4153,12 +4157,16 @@ bool Segment::WriteFramesLessThan(uint64_t timestamp) {
const Frame* const frame_prev = frames_[i - 1];
if (frame_prev->discard_padding() != 0)
doc_type_version_ = 4;
- if (!cluster->AddFrame(frame_prev))
- return false;
+ if (!cluster->AddFrame(frame_prev)) {
+ delete frame_prev;
+ continue;
+ }
if (new_cuepoint_ && cues_track_ == frame_prev->track_number()) {
- if (!AddCuePoint(frame_prev->timestamp(), cues_track_))
- return false;
+ if (!AddCuePoint(frame_prev->timestamp(), cues_track_)) {
+ delete frame_prev;
+ continue;
+ }
}
++shift_left;