commit 97f8074dc9cc5ca0263325c16340ff0f4fcfc212
author A. Cody Schuffelen <schuffelen@google.com> Mon Nov 23 16:54:33 2020 -0800
committer A. Cody Schuffelen <schuffelen@google.com> Mon Nov 23 18:38:09 2020 -0800
tree d50517a1ea6281b414da40bad6ab66c9f8c21bb9
parent 11cae244cc06c1295bffa9861c610dcde3b9da18

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

mkvmuxer/mkvmuxer.cc

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 @@
     // 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 @@
       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;