Bug 1742559 - Adjust MIDI message validation and add relevant tests r=padenot

Differential Revision: https://phabricator.services.mozilla.com/D134149

dom/midi/MIDIOutput.cpp

@@ -71,9 +71,12 @@ void MIDIOutput::Send(const Sequence<uint8_t>& aData,
   }
 
   nsTArray<MIDIMessage> msgArray;
-  MIDIUtils::ParseMessages(aData, timestamp, msgArray);
-  // Our translation of the spec is that invalid messages in a multi-message
-  // sequence will be thrown out, but that valid messages will still be used.
+  bool ret = MIDIUtils::ParseMessages(aData, timestamp, msgArray);
+  if (!ret) {
+    aRv.ThrowTypeError("Invalid MIDI message");
+    return;
+  }
+
   if (msgArray.IsEmpty()) {
     aRv.ThrowTypeError("Empty message array");
     return;

dom/midi/MIDIUtils.cpp

@@ -21,33 +21,65 @@ static const uint8_t kCommandLengths[] = {3, 3, 3, 3, 2, 2, 3};
 // messages is variable, so we just put zero since it won't be checked anyways.
 // Taken from MIDI Spec v1.0, Pg. 105, Table 5
 static const uint8_t kSystemLengths[] = {0, 2, 3, 2, 1, 1, 1, 1};
+static const uint8_t kReservedStatuses[] = {0xf4, 0xf5, 0xf9, 0xfd};
 
 namespace mozilla::dom::MIDIUtils {
 
+static bool IsSystemRealtimeMessage(uint8_t aByte) {
+  return (aByte & kSystemRealtimeMessage) == kSystemRealtimeMessage;
+}
+
+static bool IsCommandByte(uint8_t aByte) {
+  return (aByte & kCommandByte) == kCommandByte;
+}
+
+static bool IsReservedStatus(uint8_t aByte) {
+  for (const auto& msg : kReservedStatuses) {
+    if (aByte == msg) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 // Checks validity of MIDIMessage passed to it. Throws debug warnings and
 // returns false if message is not valid.
 bool IsValidMessage(const MIDIMessage* aMsg) {
-  if (NS_WARN_IF(!aMsg)) {
+  if (aMsg->data().Length() == 0) {
     return false;
   }
-  // Assert on parser problems
-  MOZ_ASSERT(aMsg->data().Length() > 0,
-             "Created a MIDI Message of Length 0. This should never happen!");
+
   uint8_t cmd = aMsg->data()[0];
   // If first byte isn't a command, something is definitely wrong.
-  MOZ_ASSERT((cmd & kCommandByte) == kCommandByte,
-             "Constructed a MIDI packet where first byte is not command!");
+  if (!IsCommandByte(cmd)) {
+    NS_WARNING("Constructed a MIDI packet where first byte is not command!");
+    return false;
+  }
+
+  if (IsReservedStatus(cmd)) {
+    NS_WARNING("Using a reserved message");
+    return false;
+  }
+
   if (cmd == kSysexMessageStart) {
-    // All we can do with sysex is make sure it starts and  ends with the
-    // correct command bytes.
+    // All we can do with sysex is make sure it starts and ends with the
+    // correct command bytes and that it does not contain other command bytes.
     if (aMsg->data()[aMsg->data().Length() - 1] != kSysexMessageEnd) {
       NS_WARNING("Last byte of Sysex Message not 0xF7!");
       return false;
     }
+
+    for (size_t i = 1; i < aMsg->data().Length() - 2; i++) {
+      if (IsCommandByte(aMsg->data()[i])) {
+        return false;
+      }
+    }
+
     return true;
   }
   // For system realtime messages, the length should always be 1.
-  if ((cmd & kSystemRealtimeMessage) == kSystemRealtimeMessage) {
+  if (IsSystemRealtimeMessage(cmd)) {
     return aMsg->data().Length() == 1;
   }
   // Otherwise, just use the correct array for testing lengths. We can't tell
@@ -72,44 +104,66 @@ bool IsValidMessage(const MIDIMessage* aMsg) {
   return aMsg->data().Length() == kCommandLengths[cmdIndex];
 }
 
-uint32_t ParseMessages(const nsTArray<uint8_t>& aByteBuffer,
-                       const TimeStamp& aTimestamp,
-                       nsTArray<MIDIMessage>& aMsgArray) {
+bool ParseMessages(const nsTArray<uint8_t>& aByteBuffer,
+                   const TimeStamp& aTimestamp,
+                   nsTArray<MIDIMessage>& aMsgArray) {
   uint32_t bytesRead = 0;
   bool inSysexMessage = false;
-  UniquePtr<MIDIMessage> currentMsg;
+  UniquePtr<MIDIMessage> currentMsg = nullptr;
   for (const auto& byte : aByteBuffer) {
     bytesRead++;
-    if ((byte & kSystemRealtimeMessage) == kSystemRealtimeMessage) {
+
+    if (IsSystemRealtimeMessage(byte)) {
       MIDIMessage rt_msg;
       rt_msg.data().AppendElement(byte);
       rt_msg.timestamp() = aTimestamp;
+      if (!IsValidMessage(&rt_msg)) {
+        return false;
+      }
       aMsgArray.AppendElement(rt_msg);
       continue;
     }
+
     if (byte == kSysexMessageEnd) {
       if (!inSysexMessage) {
-        MOZ_ASSERT(inSysexMessage);
         NS_WARNING(
             "Got sysex message end with no sysex message being processed!");
+        return false;
       }
       inSysexMessage = false;
-    } else if (byte & kCommandByte) {
-      if (currentMsg && IsValidMessage(currentMsg.get())) {
+    } else if (IsCommandByte(byte)) {
+      if (currentMsg) {
+        if (!IsValidMessage(currentMsg.get())) {
+          return false;
+        }
+
         aMsgArray.AppendElement(*currentMsg);
       }
+
       currentMsg = MakeUnique<MIDIMessage>();
       currentMsg->timestamp() = aTimestamp;
     }
+
+    if (!currentMsg) {
+      NS_WARNING("No command byte has been encountered yet!");
+      return false;
+    }
+
     currentMsg->data().AppendElement(byte);
+
     if (byte == kSysexMessageStart) {
       inSysexMessage = true;
     }
   }
-  if (currentMsg && IsValidMessage(currentMsg.get())) {
+
+  if (currentMsg) {
+    if (!IsValidMessage(currentMsg.get())) {
+      return false;
+    }
     aMsgArray.AppendElement(*currentMsg);
   }
-  return bytesRead;
+
+  return true;
 }
 
 bool IsSysexMessage(const MIDIMessage& aMsg) {

dom/midi/MIDIUtils.h

@@ -17,10 +17,10 @@ class MIDIMessage;
 namespace MIDIUtils {
 
 // Takes a nsTArray of bytes and parses it into zero or more MIDI messages.
-// Returns number of bytes parsed.
-uint32_t ParseMessages(const nsTArray<uint8_t>& aByteBuffer,
-                       const TimeStamp& aTimestamp,
-                       nsTArray<MIDIMessage>& aMsgArray);
+// Returns true if no errors were encountered, false otherwise.
+bool ParseMessages(const nsTArray<uint8_t>& aByteBuffer,
+                   const TimeStamp& aTimestamp,
+                   nsTArray<MIDIMessage>& aMsgArray);
 // Returns true if a message is a sysex message.
 bool IsSysexMessage(const MIDIMessage& a);
 }  // namespace MIDIUtils

dom/midi/TestMIDIPlatformService.cpp

@@ -107,6 +107,7 @@ void TestMIDIPlatformService::Init() {
   MIDIPlatformService::Get()->AddPortInfo(mControlInputPort);
   MIDIPlatformService::Get()->AddPortInfo(mControlOutputPort);
   MIDIPlatformService::Get()->AddPortInfo(mAlwaysClosedTestOutputPort);
+  MIDIPlatformService::Get()->AddPortInfo(mStateTestOutputPort);
   nsCOMPtr<nsIRunnable> r(new SendPortListRunnable());
 
   // Start the IO Thread.
@@ -220,8 +221,8 @@ void TestMIDIPlatformService::ProcessMessages(const nsAString& aPortId) {
             // messages.
             case 0x01: {
               nsTArray<uint8_t> msgs;
-              const uint8_t msg[] = {0xF0, 0x01, 0xF8, 0x02, 0x03,
-                                     0x04, 0xF9, 0x05, 0xF7};
+              const uint8_t msg[] = {0xF0, 0x01, 0xFA, 0x02, 0x03,
+                                     0x04, 0xF8, 0x05, 0xF7};
               // Can't use AppendElements on an array here, so just do range
               // based loading.
               for (const auto& s : msg) {

dom/midi/tests/mochitest.ini

@@ -19,3 +19,4 @@ skip-if = os == 'android' || os == 'linux' || os == 'mac' #Bug 1747637
 disabled = Bug 1437204
 [test_midi_device_pending.html]
 disabled = Bug 1437204
+[test_midi_send_messages.html]

dom/midi/tests/test_midi_device_enumeration.html

@@ -31,7 +31,7 @@
        var inputs = access.inputs;
        var outputs = access.outputs;
        is(inputs.size, 1, "Should have one input");
-       is(outputs.size, 2, "Should have two outputs");
+       is(outputs.size, 3, "Should have three outputs");
        ok(inputs.has(input_id), "input list should contain input id");
        ok(outputs.has(output_id), "output list should contain output id");
        var input = access.inputs.get(input_id);

dom/midi/tests/test_midi_device_system_rt.html

@@ -18,9 +18,9 @@
        function checkReturn(msg) {
          checkCount++;
          if (checkCount == 1) {
-           MIDITestUtils.checkPacket(msg.data, [0xF8]);
+           MIDITestUtils.checkPacket(msg.data, [0xFA]);
          } else if (checkCount == 2) {
-           MIDITestUtils.checkPacket(msg.data, [0xF9]);
+           MIDITestUtils.checkPacket(msg.data, [0xF8]);
          } else if (checkCount == 3) {
            MIDITestUtils.checkPacket(msg.data, [0xF0, 0x01, 0x02, 0x03, 0x04, 0x05, 0xF7]);
            SimpleTest.finish();

dom/midi/tests/test_midi_send_messages.html

@@ -0,0 +1,114 @@
+<html>
+
+<head>
+  <title>WebMIDI Send Test</title>
+  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <script type="application/javascript" src="MIDITestUtils.js"></script>
+</head>
+
+<body onload="runTests()">
+  <script class="testbody" type="application/javascript">
+    SimpleTest.waitForExplicitFinish();
+
+    async function runTests() {
+      await MIDITestUtils.permissionSetup(true);
+      const access = await navigator.requestMIDIAccess({ sysex: true });
+      const output = access.outputs.get(MIDITestUtils.stateTestOutputInfo.id);
+
+      dump("output = " + JSON.stringify(output, null, "  ") + "\n");
+
+      // Note on(off).
+      output.send([0xff, 0x90, 0x00, 0x00, 0x90, 0x07, 0x00]);
+
+      try {
+        output.send([0x00, 0x01])
+      } catch (ex) {
+        dump("Caught exception");
+        ok(true, "Caught exception");
+      }
+
+      // Running status is not allowed in Web MIDI API.
+      SimpleTest.doesThrow(() => output.send([0x00, 0x01]), "Running status is not allowed in Web MIDI API.");
+
+      // Unexpected End of Sysex.
+      SimpleTest.doesThrow(() => output.send([0xf7]), "Unexpected End of Sysex.");
+
+      // Unexpected reserved status bytes.
+      SimpleTest.doesThrow(() => output.send([0xf4]), "Unexpected reserved status byte 0xf4.");
+      SimpleTest.doesThrow(() => output.send([0xf5]), "Unexpected reserved status byte 0xf5.");
+      SimpleTest.doesThrow(() => output.send([0xf9]), "Unexpected reserved status byte 0xf9.");
+      SimpleTest.doesThrow(() => output.send([0xfd]), "Unexpected reserved status byte 0xfd.");
+
+      // Incomplete channel messages.
+      SimpleTest.doesThrow(() => output.send([0x80]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0x80, 0x00]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0x90]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0x90, 0x00]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xa0]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xa0, 0x00]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xb0]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xb0, 0x00]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xc0]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xd0]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xe0]), "Incomplete channel message.");
+      SimpleTest.doesThrow(() => output.send([0xe0, 0x00]), "Incomplete channel message.");
+
+      // Incomplete system messages.
+      SimpleTest.doesThrow(() => output.send([0xf1]), "Incomplete system message.");
+      SimpleTest.doesThrow(() => output.send([0xf2]), "Incomplete system message.");
+      SimpleTest.doesThrow(() => output.send([0xf2, 0x00]), "Incomplete system message.");
+      SimpleTest.doesThrow(() => output.send([0xf3]), "Incomplete system message.");
+
+      // Invalid data bytes.
+      SimpleTest.doesThrow(() => output.send([0x80, 0x80, 0x00]), "Incomplete system message.");
+      SimpleTest.doesThrow(() => output.send([0x80, 0x00, 0x80]), "Incomplete system message.");
+
+      // Complete messages.
+      output.send([0x80, 0x00, 0x00]);
+      output.send([0x90, 0x00, 0x00]);
+      output.send([0xa0, 0x00, 0x00]);
+      output.send([0xb0, 0x00, 0x00]);
+      output.send([0xc0, 0x00]);
+      output.send([0xd0, 0x00]);
+      output.send([0xe0, 0x00, 0x00]);
+
+      // Real-Time messages.
+      output.send([0xf8]);
+      output.send([0xfa]);
+      output.send([0xfb]);
+      output.send([0xfc]);
+      output.send([0xfe]);
+      output.send([0xff]);
+
+      // Valid messages with Real-Time messages.
+      output.send([0x90, 0xff, 0xff, 0x00, 0xff, 0x01, 0xff, 0x80, 0xff, 0x00,
+        0xff, 0xff, 0x00, 0xff, 0xff]);
+
+      // Sysex messages.
+      output.send([0xf0, 0x00, 0x01, 0x02, 0x03, 0xf7]);
+      output.send([0xf0, 0xf8, 0xf7, 0xff]);
+      SimpleTest.doesThrow(() => output.send([0xf0, 0x80, 0xf7]), "Invalid sysex message.");
+      SimpleTest.doesThrow(() => output.send([0xf0, 0xf0, 0xf7]), "Double begin sysex message.");
+      SimpleTest.doesThrow(() => output.send([0xf0, 0xff, 0xf7, 0xf7]), "Double end sysex message.");
+
+      // Reserved status bytes.
+      SimpleTest.doesThrow(() => output.send([0xf4, 0x80, 0x00, 0x00]), "Reserved status byte.");
+      SimpleTest.doesThrow(() => output.send([0x80, 0xf4, 0x00, 0x00]), "Reserved status byte.");
+      SimpleTest.doesThrow(() => output.send([0x80, 0x00, 0xf4, 0x00]), "Reserved status byte.");
+      SimpleTest.doesThrow(() => output.send([0x80, 0x00, 0x00, 0xf4]), "Reserved status byte.");
+      SimpleTest.doesThrow(() => output.send([0xf0, 0xff, 0xf4, 0xf7]), "Reserved status byte.");
+
+      // Invalid timestamps.
+      SimpleTest.doesThrow(() => output.send([], NaN), "NaN timestamp.");
+      SimpleTest.doesThrow(() => output.send([], Infinity), "Infinity timestamp.");
+      SimpleTest.doesThrow(() => output.send(new Uint8Array(), NaN), "NaN timestamp.");
+      SimpleTest.doesThrow(() => output.send(new Uint8Array(), Infinity), "Infinity timestamp.");
+
+      SimpleTest.finish();
+    }
+  </script>
+</body>
+
+</html>