Add media file validation to utility process

media/base/media_file_checker.cc

Index: media/base/media_file_checker.cc
diff --git a/media/base/media_file_checker.cc b/media/base/media_file_checker.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bd5df71dd2d6e1ee3b85e93cc0dc8bb7d9fa1806
--- /dev/null
+++ b/media/base/media_file_checker.cc
@@ -0,0 +1,116 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/media_file_checker.h"
+
+#include <map>
+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "base/time/time.h"
+#include "media/ffmpeg/ffmpeg_common.h"
+#include "media/filters/blocking_url_protocol.h"
+#include "media/filters/ffmpeg_glue.h"
+#include "media/filters/file_data_source.h"
+
+namespace media {
+
+namespace {
+
+void ClearBoolCallback(bool* called) {

[DaleCurtis, 2013/08/07 18:27:07]

Name is pretty meaningless.  How about OnError() instead.  Also make it static.

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Done.

+  *called = false;
+}
+
+struct StreamContext {
+  StreamContext()
+      : codec_context(NULL),
+        codec(NULL) {
+  }
+  AVCodecContext* codec_context;
+  AVCodec* codec;
+};
+
+}  // namespace
+
+
+MediaFileChecker::MediaFileChecker(base::PlatformFile file)
+    : file_(file),
+      file_closer_(&file_) {
+}
+
+MediaFileChecker::~MediaFileChecker() {
+}
+
+bool MediaFileChecker::Start(const base::TimeDelta& check_time) {

[DaleCurtis, 2013/08/07 18:27:07]

Sanity check check_time to be positive and less than some maximum?

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Done.

+  media::FileDataSource source;
+  bool read_ok = true;
+  media::BlockingUrlProtocol protocol(&source,
+                                      base::Bind(&ClearBoolCallback,
+                                                 base::Unretained(&read_ok)));

[DaleCurtis, 2013/08/07 18:27:07]

Is unretained necessary here? 

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Done.

+  media::FFmpegGlue glue(&protocol);
+  source.InitializeFromPlatformFile(file_);
+  AVFormatContext* format_context = glue.format_context();
+
+  if (!glue.OpenContext())
+    return false;
+
+  if (avformat_find_stream_info(format_context, NULL) < 0)
+    return false;
+
+  // Remember the codec for any decodable audio or video streams.
+  std::map<int, StreamContext> stream_contexts;
+  for (size_t i = 0; i < format_context->nb_streams; ++i) {

[DaleCurtis, 2013/08/07 18:27:07]

Just FYI, the way you're doing this doesn't verify that the file is what it says
it is, just that we can decode it.  Also, it ignores streams we can't decode so
long as there is a single valid stream -- through which it might be possible to
create some crazy polyglot attack, a la something like:
https://code.google.com/p/corkami/downloads/detail?name=CorkaMIX.zip

Which is why I suggested previously that attempting to decode the file probably
doesn't get you anything besides wasted cycles :)

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Not what it says it is in what sense?  It doesn't check that the file extension
is correct, right.  It does check that it is a decodable media file.

My thinking on ignoring other streams is that in videos there may be audio
streams in formats that we don't support.

Agreed, this doesn't prevent polyglot attacks, but if it doesn't cause the
decoder to barf, it does require an attacker to try harder.  The code that uses
this sets check_time to 250ms. I agree that it doesn't provide a lot more
assurance, but it does raise the bar somewhat.

+    AVCodecContext* c = format_context->streams[i]->codec;
+    if (c->codec_type == AVMEDIA_TYPE_AUDIO ||
+        c->codec_type == AVMEDIA_TYPE_VIDEO) {
+      AVCodec* codec = avcodec_find_decoder(c->codec_id);
+      if (codec && avcodec_open2(c, codec, NULL) >= 0) {
+        stream_contexts[i].codec_context = c;
+        stream_contexts[i].codec = codec;
+      }
+    }
+  }
+
+  if (stream_contexts.size() == 0)
+    return false;
+
+

[DaleCurtis, 2013/08/07 18:27:07]

Spacing?

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Done.  Don't know how that got in there... :-/

+
+  AVPacket packet;
+  scoped_ptr_malloc<AVFrame, media::ScopedPtrAVFree> frame(
+      avcodec_alloc_frame());
+  int result_code = 0;
+  base::Time deadline = base::Time::Now() + check_time;
+
+  do {

[DaleCurtis, 2013/08/07 18:27:07]

Not quite correct, you may need to call this multiple times for a single packet.
AudioFileReader has a nested while/for loop covering this. Failure to do so may
ignore packet data which is actually invalid,

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

I don't really understand the API, but I've copied what's in AudioFileReader.

[DaleCurtis, 2013/08/07 22:56:20]

This is not what's in AudioFileReader, note that AudioFileReader has a
do{}while(packet_size >0) nested inside of a while(av_read_frame()) loop.

+    result_code = av_read_frame(glue.format_context(), &packet);
+    if (result_code < 0)
+      break;
+
+    if (!ContainsKey(stream_contexts, packet.stream_index))
+      continue;
+
+    avcodec_get_frame_defaults(frame.get());
+    int frame_decoded = 0;
+    AVMediaType type =
+        stream_contexts[packet.stream_index].codec_context->codec_type;
+    if (type == AVMEDIA_TYPE_AUDIO) {
+      result_code = avcodec_decode_audio4(
+          stream_contexts[packet.stream_index].codec_context, frame.get(),
+          &frame_decoded, &packet);
+    } else if (type == AVMEDIA_TYPE_VIDEO) {
+      result_code = avcodec_decode_video2(
+          stream_contexts[packet.stream_index].codec_context, frame.get(),
+          &frame_decoded, &packet);
+    }
+  } while (base::Time::Now() < deadline && result_code >= 0);

[DaleCurtis, 2013/08/07 18:27:07]

&& read_ok

[vandebo (ex-Chrome), 2013/08/07 21:00:43]

Done.

+
+  av_free_packet(&packet);
+
+  if (result_code == AVERROR_EOF)
+    result_code = 0;
+  return read_ok && result_code >= 0;
+}
+
+}  // namespace media