Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

content/browser/renderer_host/media/midi_host.cc

Index: content/browser/renderer_host/media/midi_host.cc
diff --git a/content/browser/renderer_host/media/midi_host.cc b/content/browser/renderer_host/media/midi_host.cc
index 934d5a21f1839fa97a5740fc60d28bba1936a05a..a044a2b2d8700126f6f6bef542d8db01931b8f63 100644
--- a/content/browser/renderer_host/media/midi_host.cc
+++ b/content/browser/renderer_host/media/midi_host.cc
@@ -16,6 +16,7 @@
 #include "content/public/browser/media_observer.h"
 #include "content/public/browser/user_metrics.h"
 #include "media/midi/midi_manager.h"
+#include "media/midi/midi_message_util.h"
 
 using media::MIDIManager;
 using media::MIDIPortInfoList;
@@ -36,6 +37,7 @@ namespace content {
 
 MIDIHost::MIDIHost(int renderer_process_id, media::MIDIManager* midi_manager)
     : renderer_process_id_(renderer_process_id),
+      sysex_allowed_(false),

[Takashi Toyoshima, 2013/11/20 10:04:58]

I like this new approach.

[yukawa, 2013/11/20 11:06:08]

Thanks. And I renamed |sysex_allowed_| with |has_sys_ex_permission_| so that we
can share similar terminology between MIDIDispatcherHost and MIDIHost.

       midi_manager_(midi_manager),
       sent_bytes_in_flight_(0),
       bytes_sent_since_last_acknowledgement_(0) {
@@ -75,6 +77,12 @@ void MIDIHost::OnStartSession(int client_id) {
     if (success) {
       input_ports = midi_manager_->input_ports();
       output_ports = midi_manager_->output_ports();
+      received_messages_queues_.clear();
+      received_messages_queues_.resize(input_ports.size());
+      // ChildSecurityPolicy is set just before OnStartSession. So we can safely
+      // cache the policy.
+      sysex_allowed_ = ChildProcessSecurityPolicyImpl::GetInstance()->

[Takashi Toyoshima, 2013/11/20 10:04:58]

FYI, midi_dispatcher_host.cc is the exact place to store the permission.

[yukawa, 2013/11/20 11:06:08]

Thank you for the info!  Slightly revised the comment.

+          CanSendMIDISysExMessage(renderer_process_id_);
     }
   }
 
@@ -94,32 +102,24 @@ void MIDIHost::OnSendData(uint32 port,
   if (data.empty())
     return;
 
-  base::AutoLock auto_lock(in_flight_lock_);
-
-  // Sanity check that we won't send too much.
-  if (sent_bytes_in_flight_ > kMaxInFlightBytes ||
-      data.size() > kMaxInFlightBytes ||
-      data.size() + sent_bytes_in_flight_ > kMaxInFlightBytes)
+  // Blink running in a renderer checks permission to raise a SecurityError
+  // in JavaScript. The actual permission check for security purposes
+  // happens here in the browser process.
+  if (!sysex_allowed_ &&
+      (std::find(data.begin(), data.end(), kSysExMessage) != data.end())) {
+    RecordAction(UserMetricsAction("BadMessageTerminate_MIDI"));
+    BadMessageReceived();
     return;
-
-  if (data[0] >= kSysExMessage) {
-    // Blink running in a renderer checks permission to raise a SecurityError in
-    // JavaScript. The actual permission check for security perposes happens
-    // here in the browser process.
-    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanSendMIDISysExMessage(
-        renderer_process_id_)) {
-      RecordAction(UserMetricsAction("BadMessageTerminate_MIDI"));
-      BadMessageReceived();
-      return;
-    }
   }
 
-  midi_manager_->DispatchSendMIDIData(
-      this,
-      port,
-      data,
-      timestamp);
+  if (!media::IsValidMIDIMessagesForWebMIDI(data))
+    return;
 
+  base::AutoLock auto_lock(in_flight_lock_);
+  // Sanity check that we won't send too much data.
+  if (data.size() + sent_bytes_in_flight_ > kMaxInFlightBytes)

[Takashi Toyoshima, 2013/11/20 10:04:58]

Can you add TODO here?
We should handle internal buffer overflow later, and may want to throw some
error in JavaScript. We'll discuss it in W3C.

[yukawa, 2013/11/20 11:06:08]

Done.

+    return;
+  midi_manager_->DispatchSendMIDIData(this, port, data, timestamp);
   sent_bytes_in_flight_ += data.size();
 }
 
@@ -130,20 +130,29 @@ void MIDIHost::ReceiveMIDIData(
     double timestamp) {
   TRACE_EVENT0("midi", "MIDIHost::ReceiveMIDIData");
 
-  // Check a process security policy to receive a system exclusive message.
-  if (length > 0 && data[0] >= kSysExMessage) {
-    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanSendMIDISysExMessage(
-        renderer_process_id_)) {
-      // MIDI devices may send a system exclusive messages even if the renderer
-      // doesn't have a permission to receive it. Don't kill the renderer as
-      // OnSendData() does.
-      return;
-    }
-  }
+  if (received_messages_queues_.size() <= port)
+    return;
 
-  // Send to the renderer.
-  std::vector<uint8> v(data, data + length);
-  Send(new MIDIMsg_DataReceived(port, v, timestamp));
+  // Lazy initialization
+  if (received_messages_queues_[port] == NULL)
+    received_messages_queues_[port] = new media::MIDIMessageQueue(true);
+
+  received_messages_queues_[port]->Add(data, length);
+  std::vector<uint8> message;
+  while (true) {
+    received_messages_queues_[port]->Get(&message);
+    if (message.empty())
+      break;
+
+    // MIDI devices may send a system exclusive messages even if the renderer
+    // doesn't have a permission to receive it. Don't kill the renderer as
+    // OnSendData() does.
+    if (message[0] == kSysExMessage && !sysex_allowed_)
+      continue;
+
+    // Send to the renderer.
+    Send(new MIDIMsg_DataReceived(port, message, timestamp));
+  }
 }
 
 void MIDIHost::AccumulateMIDIBytesSent(size_t n) {