[midi1/2] Catch trying to convert invalid midi 1 bytes

celtera · Oct 26, 2024 · 952f781 · 952f781
1 parent 6245d5b
commit 952f781
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 16 deletions.
diff --git a/cmake/libremidi.tests.cmake b/cmake/libremidi.tests.cmake
@@ -15,6 +15,9 @@ if(LIBREMIDI_CI)
     target_compile_definitions(libremidi ${_public} LIBREMIDI_CI)
 endif()
 
+add_executable(conversion_test tests/unit/conversion.cpp)
+target_link_libraries(conversion_test PRIVATE libremidi Catch2::Catch2WithMain)
+
 add_executable(error_test tests/unit/error.cpp)
 target_link_libraries(error_test PRIVATE libremidi Catch2::Catch2WithMain)
 
@@ -36,6 +39,7 @@ add_executable(midifile_write_tracks_test tests/integration/midifile_write_track
 target_link_libraries(midifile_write_tracks_test PRIVATE libremidi Catch2::Catch2WithMain)
 
 include(CTest)
+add_test(NAME conversion_test COMMAND conversion_test)
 add_test(NAME error_test COMMAND error_test)
 add_test(NAME midiin_test COMMAND midiin_test --allow-running-no-tests)
 add_test(NAME midiout_test COMMAND midiout_test --allow-running-no-tests)

diff --git a/examples/sysextest.cpp b/examples/sysextest.cpp
@@ -47,7 +47,7 @@ int main(int argc, const char** argv)
   {
     message.clear();
     message.push_back(240);
-    for (auto i = 0U; i < args.count; i++)
+    for (int i = 0; i < args.count; i++)
       message.push_back(i % 128);
 
     message.push_back(247);

diff --git a/examples/utils.hpp b/examples/utils.hpp
@@ -164,7 +164,7 @@ struct arguments
       return false;
     }
 
-    if (index >= 0 && index < ports.size())
+    if (index >= 0 && index < std::ssize(ports))
     {
       std::cout << "Opening " << ports[index].display_name << std::endl;
       const auto err = midi.open_port(ports[index]);

diff --git a/include/libremidi/cmidi2.hpp b/include/libremidi/cmidi2.hpp
@@ -2559,6 +2559,7 @@ enum cmidi2_midi_conversion_result
   CMIDI2_CONVERSION_RESULT_INVALID_DTE_SEQUENCE = 0x11,
   CMIDI2_CONVERSION_RESULT_INVALID_STATUS = 0x13,
   CMIDI2_CONVERSION_RESULT_INCOMPLETE_SYSEX7 = 0x20,
+  CMIDI2_CONVERSION_RESULT_INVALID_INPUT = 0x40,
 };
 
 static inline void
@@ -2686,7 +2687,11 @@ cmidi2_convert_midi1_to_ump(cmidi2_midi_conversion_context* context)
     else
     {
       // fixed sized message
-      size_t len = cmidi2_midi1_get_message_size(context->midi1 + *sIdx, sLen - *sIdx);
+      size_t remaining = sLen - *sIdx;
+      size_t len = cmidi2_midi1_get_message_size(context->midi1 + *sIdx, remaining);
+      if (len > remaining)
+        return CMIDI2_CONVERSION_RESULT_INVALID_INPUT;
+
       uint8_t byte2 = context->midi1[*sIdx + 1];
       uint8_t byte3 = len > 2 ? context->midi1[*sIdx + 2] : 0;
       uint8_t channel = context->midi1[*sIdx] & 0xF;

diff --git a/include/libremidi/detail/conversion.hpp b/include/libremidi/detail/conversion.hpp
@@ -14,8 +14,6 @@ struct midi1_to_midi2
   stdx::error
   convert(const unsigned char* message, std::size_t size, int64_t timestamp, auto on_ump)
   {
-    uint32_t ump[65536 / 4];
-
     context.midi1 = const_cast<unsigned char*>(message);
     context.midi1_num_bytes = size;
     context.midi1_proceeded_bytes = 0;
@@ -26,23 +24,23 @@ struct midi1_to_midi2
     if (auto res = cmidi2_convert_midi1_to_ump(&context); res != CMIDI2_CONVERSION_RESULT_OK)
       return std::errc::invalid_argument;
 
-    on_ump(context.ump, context.ump_proceeded_bytes / 4, timestamp);
-    return stdx::error{};
+    return on_ump(context.ump, context.ump_proceeded_bytes / 4, timestamp);
   }
 
   cmidi2_midi_conversion_context context = [] {
     cmidi2_midi_conversion_context tmp;
     cmidi2_midi_conversion_context_initialize(&tmp);
     return tmp;
   }();
+  uint32_t ump[65536 / 4];
 };
 
 struct midi2_to_midi1
 {
-  stdx::error convert(const uint32_t* message, std::size_t size, int64_t timestamp, auto on_midi)
+  stdx::error
+  convert(const uint32_t* message, std::size_t /* size */, int64_t timestamp, auto on_midi)
   {
-    uint8_t midi[65536];
-    const auto n
+    auto n
         = cmidi2_convert_single_ump_to_midi1(midi, sizeof(midi), const_cast<uint32_t*>(message));
     if (n > 0)
       return on_midi(midi, n, timestamp);
@@ -55,6 +53,7 @@ struct midi2_to_midi1
     cmidi2_midi_conversion_context_initialize(&tmp);
     return tmp;
   }();
+  uint8_t midi[65536];
 };
 
 }
diff --git a/include/libremidi/libremidi.hpp b/include/libremidi/libremidi.hpp
@@ -219,12 +219,6 @@ class LIBREMIDI_EXPORT midi_out
   stdx::error send_message(unsigned char b0, unsigned char b1, unsigned char b2) const;
 
   // Avoid silly mistakes:
-  stdx::error send_message(int16_t b0) const noexcept = delete;
-  stdx::error send_message(int32_t b0) const noexcept = delete;
-  stdx::error send_message(int64_t b0) const noexcept = delete;
-  stdx::error send_message(uint16_t b0) const noexcept = delete;
-  stdx::error send_message(uint32_t b0) const noexcept = delete;
-  stdx::error send_message(uint64_t b0) const noexcept = delete;
   stdx::error send_message(auto* message) const noexcept = delete;
   stdx::error send_message(const auto* message) const noexcept = delete;
 

diff --git a/tests/unit/conversion.cpp b/tests/unit/conversion.cpp
@@ -0,0 +1,105 @@
+#include "../include_catch.hpp"
+
+#include <libremidi/detail/conversion.hpp>
+#include <libremidi/libremidi.hpp>
+
+#include <array>
+#include <bit>
+
+TEST_CASE("convert midi1 to midi2", "[midi_in]")
+{
+  libremidi::midi1_to_midi2 m1;
+  GIVEN("Correct input")
+  {
+    bool ok = false;
+    auto msg = libremidi::channel_events::control_change(3, 35, 100);
+
+    std::vector<uint32_t> res;
+    auto err
+        = m1.convert(msg.bytes.data(), msg.size(), 0, [&](uint32_t* ump, std::size_t sz, int64_t) {
+      ok = true;
+      res.assign(ump, ump + sz);
+      return stdx::error{};
+    });
+
+    THEN("Correct conversion")
+    {
+      REQUIRE(err == stdx::error{});
+      REQUIRE(ok);
+      auto expected = cmidi2_ump_midi2_cc(0, 2, 35, 100 << 25);
+      union
+      {
+        uint32_t u[2];
+        int64_t i;
+      } e{.i = expected};
+
+      REQUIRE(res.size() == 2);
+      REQUIRE(e.u[1] == res[0]);
+      REQUIRE(e.u[0] == res[1]);
+    }
+  }
+
+  GIVEN("Wrong input")
+  {
+    bool ok = false;
+    unsigned char arr[1]{156};
+    auto err = m1.convert(arr, 1, 0, [&](uint32_t*, std::size_t, int64_t) {
+      ok = true;
+      return stdx::error{};
+    });
+    THEN("We get an error")
+    {
+      REQUIRE(err != stdx::error{});
+      REQUIRE(ok == false);
+    }
+  }
+}
+
+TEST_CASE("convert midi2 to midi1", "[midi_in]")
+{
+  libremidi::midi2_to_midi1 m1;
+  GIVEN("Correct input")
+  {
+    bool ok = false;
+    auto msg = cmidi2_ump_midi2_cc(0, 2, 35, 100 << 25);
+    union
+    {
+      uint32_t u[2];
+      int64_t i;
+    } e{.i = msg};
+    using namespace std;
+    swap(e.u[0], e.u[1]);
+
+    std::vector<uint8_t> res;
+    auto err = m1.convert(e.u, 2, 0, [&](uint8_t* midi, std::size_t sz, int64_t) {
+      ok = true;
+      res.assign(midi, midi + sz);
+      return stdx::error{};
+    });
+
+    THEN("Correct conversion")
+    {
+      REQUIRE(err == stdx::error{});
+      REQUIRE(ok);
+      auto msg = libremidi::channel_events::control_change(3, 35, 100);
+
+      REQUIRE(res.size() == 3);
+      REQUIRE(res == std::vector<uint8_t>(msg.begin(), msg.end()));
+    }
+  }
+
+  GIVEN("Wrong input")
+  {
+    bool ok = false;
+    uint32_t res = 0xFFFFFFFF;
+    auto err = m1.convert(&res, 1, 0, [&](uint8_t*, std::size_t, int64_t) {
+      ok = true;
+      return stdx::error{};
+    });
+    THEN("We get an error")
+    {
+      REQUIRE(err != stdx::error{});
+      REQUIRE(ok == false);
+    }
+  }
+}