test msgpack encoder error cases (#15)

* cover more error cases in msgpack encoder
* speed up coverage report generation
* don't execute a readme file

Author: dgoffredo

bin/README.md

@@ -10,3 +10,10 @@ cd lcov-1.16/
 make install
 cd ../
 rm -r lcov-1.16 lcov-1.16.tar.gz
+
+# Install a faster JSON library for Perl using Perl's package manager, cpan.
+# Newer versions of lcov use JSON instead of an old binary format, so if the
+# JSON library is slow, lcov is slow.
+# The PERL_MM_USE_DEFAULT=1 environment variable prevents cpan from prompting
+# us. 
+PERL_MM_USE_DEFAULT=1 cpan JSON:XS

bin/install-lcov

@@ -55,7 +55,8 @@ if [ "$coverage_flags" != '' ]; then
     rm -rf .coverage
     mkdir -p .coverage
     # Compile the .gcdo files into an intermediate format.
-    lcov --quiet --capture --directory .build --output-file .coverage/raw.info # -rc geninfo_json_module=JSON::XS
+    # Use the faster JSON Perl module, JSON:XS, to speed things up.
+    lcov --quiet --capture --directory .build --output-file .coverage/raw.info --rc geninfo_json_module=JSON::XS
     # Filter out system headers and test drivers.
     lcov --quiet --remove .coverage/raw.info -o .coverage/filtered.info '/usr/*' "$(pwd)/test/*" '*json.hpp'
     # Generate an HTML coverage report at ".coverage/report/index.html".

bin/test

@@ -1,6 +1,7 @@
 #include "msgpack.h"
 
 #include <cassert>
+#include <climits>
 #include <limits>
 #include <type_traits>
 
@@ -50,7 +51,7 @@ void push_number_big_endian(std::string& buffer, Integer integer) {
   // effectively copies the bytes of `value` backwards.
   const int size = sizeof value;
   for (int i = 0; i < size; ++i) {
-    const char byte = (value >> (8 * ((size - 1) - i))) & 0xFF;
+    const char byte = (value >> (CHAR_BIT * ((size - 1) - i))) & 0xFF;
     buf[i] = byte;
   }
 
@@ -93,20 +94,20 @@ void pack_double(std::string& buffer, double value) {
   push_number_big_endian(buffer, memory.as_integer);
 }
 
-Expected<void> pack_string(std::string& buffer, StringView value) {
-  const auto size = value.size();
+Expected<void> pack_string(std::string& buffer, const char* begin,
+                           std::size_t size) {
   const auto max = std::numeric_limits<std::uint32_t>::max();
   if (size > max) {
     return Error{Error::MESSAGEPACK_ENCODE_FAILURE,
                  make_overflow_message("string", size, max)};
   }
   buffer.push_back(static_cast<char>(types::STR32));
   push_number_big_endian(buffer, static_cast<std::uint32_t>(size));
-  buffer.append(value.begin(), value.end());
+  buffer.append(begin, size);
   return {};
 }
 
-Expected<void> pack_array(std::string& buffer, size_t size) {
+Expected<void> pack_array(std::string& buffer, std::size_t size) {
   const auto max = std::numeric_limits<std::uint32_t>::max();
   if (size > max) {
     return Error{Error::MESSAGEPACK_ENCODE_FAILURE,
@@ -117,7 +118,7 @@ Expected<void> pack_array(std::string& buffer, size_t size) {
   return {};
 }
 
-Expected<void> pack_map(std::string& buffer, size_t size) {
+Expected<void> pack_map(std::string& buffer, std::size_t size) {
   const auto max = std::numeric_limits<std::uint32_t>::max();
   if (size > max) {
     return Error{Error::MESSAGEPACK_ENCODE_FAILURE,

src/datadog/msgpack.cpp

@@ -11,6 +11,7 @@
 //
 // [1]: https://msgpack.org/index.html
 
+#include <climits>
 #include <cstddef>
 #include <cstdint>
 #include <string>
@@ -30,6 +31,8 @@ void pack_integer(std::string& buffer, std::int32_t value);
 void pack_double(std::string& buffer, double value);
 
 Expected<void> pack_string(std::string& buffer, StringView value);
+Expected<void> pack_string(std::string& buffer, const char* begin,
+                           std::size_t size);
 
 Expected<void> pack_array(std::string& buffer, std::size_t size);
 
@@ -65,17 +68,16 @@ Expected<void> pack_map(std::string& buffer, const PairIterable& pairs,
 // an even number of arguments.  First in each pair of arguments is `key`, the
 // key name of the corresponding map item.  Second in each pair of arguments is
 // `pack_value`, a function that encodes the corresponding value.  `pack_value`
-// is invoked with two arguments: the first is a reference to `buffer`, and the
-// second is a reference to the current value.  `pack_value` returns an
-// `Expected<void>`. If the return value is an error, then iteration is halted
-// and the error is returned.  If some other error occurs, then an error is
-// returned.  Otherwise, the non-error value is returned.
-template <typename Key, typename PackValue, typename... Rest>
-Expected<void> pack_map(std::string& buffer, Key&& key, PackValue&& pack_value,
-                        Rest&&... rest);
-
-template <typename Key, typename PackValue, typename... Rest>
-Expected<void> pack_map_suffix(std::string& buffer, Key&& key,
+// is invoked with one argument: a reference to `buffer`. `pack_value` returns
+// an `Expected<void>`.  If the return value is an error, then iteration is
+// halted and the error is returned.  If some other error occurs, then an error
+// is returned.  Otherwise, the non-error value is returned.
+template <typename PackValue, typename... Rest>
+Expected<void> pack_map(std::string& buffer, StringView key,
+                        PackValue&& pack_value, Rest&&... rest);
+
+template <typename PackValue, typename... Rest>
+Expected<void> pack_map_suffix(std::string& buffer, StringView key,
                                PackValue&& pack_value, Rest&&... rest);
 Expected<void> pack_map_suffix(std::string& buffer);
 
@@ -117,25 +119,23 @@ Expected<void> pack_map(std::string& buffer, const PairIterable& pairs,
   return result;
 }
 
-template <typename Key, typename PackValue, typename... Rest>
-Expected<void> pack_map(std::string& buffer, Key&& key, PackValue&& pack_value,
-                        Rest&&... rest) {
-  Expected<void> result;
+template <typename PackValue, typename... Rest>
+Expected<void> pack_map(std::string& buffer, StringView key,
+                        PackValue&& pack_value, Rest&&... rest) {
   static_assert(
       sizeof...(rest) % 2 == 0,
       "pack_map must receive an even number of arguments after the first.");
-  result = pack_map(buffer, 1 + sizeof...(rest) / 2);
-  if (!result) {
-    return result;
-  }
-  result = pack_map_suffix(buffer, std::forward<Key>(key),
-                           std::forward<PackValue>(pack_value),
-                           std::forward<Rest>(rest)...);
-  return result;
+  static_assert(
+      sizeof...(rest) / 2 <= UINT32_MAX,
+      "You're passing more than eight billion arguments to a function.");
+  (void)pack_map(buffer, 1 + sizeof...(rest) / 2);
+
+  return pack_map_suffix(buffer, key, std::forward<PackValue>(pack_value),
+                         std::forward<Rest>(rest)...);
 }
 
-template <typename Key, typename PackValue, typename... Rest>
-Expected<void> pack_map_suffix(std::string& buffer, Key&& key,
+template <typename PackValue, typename... Rest>
+Expected<void> pack_map_suffix(std::string& buffer, StringView key,
                                PackValue&& pack_value, Rest&&... rest) {
   Expected<void> result;
   result = pack_string(buffer, key);
@@ -159,6 +159,10 @@ inline void pack_integer(std::string& buffer, std::int32_t value) {
   pack_integer(buffer, std::int64_t(value));
 }
 
+inline Expected<void> pack_string(std::string& buffer, StringView value) {
+  return pack_string(buffer, value.begin(), value.size());
+}
+
 }  // namespace msgpack
 }  // namespace tracing
 }  // namespace datadog

src/datadog/msgpack.h

@@ -21,6 +21,7 @@ add_executable(tests
     test_datadog_agent.cpp
     test_glob.cpp
     test_limiter.cpp
+    test_msgpack.cpp
     test_smoke.cpp
     test_span.cpp
     test_span_sampler.cpp

test/CMakeLists.txt

@@ -0,0 +1,136 @@
+#include <datadog/error.h>
+#include <datadog/msgpack.h>
+
+#include <cstdint>
+#include <string>
+#include <utility>
+
+#include "test.h"
+
+namespace datadog {
+namespace tracing {
+
+bool operator==(const Error& left, const Error& right) {
+  return left.code == right.code && left.message == right.message;
+}
+
+}  // namespace tracing
+}  // namespace datadog
+
+using namespace datadog::tracing;
+
+TEST_CASE("array element fails to encode") {
+  std::string destination;
+  const int dummy[] = {42};
+  const Error error{Error::OTHER, "any error will do"};
+  const auto result = msgpack::pack_array(
+      destination, dummy, [&](std::string&, int) { return error; });
+
+  REQUIRE_FALSE(result);
+  REQUIRE(result.error() == error);
+}
+
+TEST_CASE("map element fails to encode") {
+  std::string destination;
+  const Error error{Error::OTHER, "any error will do"};
+
+  SECTION("sequence of pairs") {
+    const std::pair<std::string, int> pairs[] = {
+        std::pair<std::string, int>{"dummy", 42}};
+    const auto result = msgpack::pack_map(
+        destination, pairs, [&](std::string&, int) { return error; });
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error() == error);
+  }
+
+  SECTION("key/value arguments") {
+    const auto succeed = [](std::string&) { return nullopt; };
+    const auto fail = [&](std::string&) { return error; };
+    const auto result =
+        msgpack::pack_map(destination, "foo", succeed, "bar", fail);
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error() == error);
+  }
+}
+
+// The following group of tests verify that encoding routines return an error
+// if the size of their input cannot fit in 32 bits.
+// This is impossible to do on a 32-bit system, so these tests are excluded by
+// the preprocessor when the pointer size is not greater than four.
+#if UINTPTR_MAX > UINT32_MAX
+
+// `OversizedSequence` can be used to test oversized inputs for both the
+// sequence overloads of both the array and map encoding functions.
+struct OversizedSequence {
+  std::size_t size() const {
+    return std::size_t(std::numeric_limits<std::uint32_t>::max()) + 1;
+  }
+
+  const std::pair<std::string, int>* begin() const { return nullptr; }
+  const std::pair<std::string, int>* end() const { return nullptr; }
+};
+
+TEST_CASE("oversized string") {
+  const char* const dummy = "doesn't matter";
+  const auto oversized =
+      std::size_t(std::numeric_limits<std::uint32_t>::max()) + 1;
+  std::string destination;
+  const auto result = msgpack::pack_string(destination, dummy, oversized);
+
+  REQUIRE_FALSE(result);
+  REQUIRE(result.error().code == Error::MESSAGEPACK_ENCODE_FAILURE);
+  REQUIRE(destination == "");
+}
+
+TEST_CASE("oversized array") {
+  std::string destination;
+
+  SECTION("just the header") {
+    const auto oversized =
+        std::size_t(std::numeric_limits<std::uint32_t>::max()) + 1;
+    const auto result = msgpack::pack_array(destination, oversized);
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error().code == Error::MESSAGEPACK_ENCODE_FAILURE);
+  }
+
+  SECTION("sequence") {
+    const auto result =
+        msgpack::pack_array(destination, OversizedSequence{},
+                            [](const auto&, const auto&) { return nullopt; });
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error().code == Error::MESSAGEPACK_ENCODE_FAILURE);
+  }
+
+  REQUIRE(destination == "");
+}
+
+TEST_CASE("oversized map") {
+  SECTION("just the header") {
+    const auto oversized =
+        std::size_t(std::numeric_limits<std::uint32_t>::max()) + 1;
+    std::string destination;
+    const auto result = msgpack::pack_map(destination, oversized);
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error().code == Error::MESSAGEPACK_ENCODE_FAILURE);
+    REQUIRE(destination == "");
+  }
+
+  SECTION("sequence of pairs") {
+    std::string destination;
+    const OversizedSequence oversized;
+    const auto result =
+        msgpack::pack_map(destination, oversized,
+                          [](const auto&, const auto&) { return nullopt; });
+
+    REQUIRE_FALSE(result);
+    REQUIRE(result.error().code == Error::MESSAGEPACK_ENCODE_FAILURE);
+    REQUIRE(destination == "");
+  }
+}
+
+#endif