[libFuzzer] Improve FuzzedDataProvider helper.

[Dor1s]

Summary

The following changes are made based on the feedback from Tim King:

• Removed default template parameters, to have less assumptions.
• Implemented ConsumeBytesWithTerminator method.
• Made PickValueInArray method work with initializer_list argument.
• Got rid of data_type type alias, that was redundant.
• Refactored ConsumeBytes logic into a private method for better code reuse.
• Replaced implementation defined unsigned to signed conversion.
• Fixed ConsumeRandomLengthString to always call shrink_to_fit.
• Clarified and fixed some commments.
• Applied clang-format to both the library and the unittest source.

Tested on Linux, Mac, Windows.

[Dor1s]

Add static_assert guards to ConvertUnsignedToSigned.

[Dor1s]

rebase

[Dor1s]

The change is tested and ready for review. Please take a look :)

[Dor1s]

Ping :)

[morehouse]

LGTM

[Dor1s]

Thanks a lot, Matt!

[Dor1s]

Hm, I'm doing the following:

$ arc commit --revision D63348
Committing 'D63348: [libFuzzer] Improve FuzzedDataProvider helper.'...
Done.

But the commit doesn't seem to happen.

[Dor1s]

Trying rebase, re-running the tests, and committing again.

compiler-rt/trunk/lib/fuzzer/tests/FuzzedDataProviderUnittest.cpp

const uint8_t Data[] = {
     0x7A, 0xA5, 0x12, 0x09, 0x26, 0x7E, 0x7E, 0x03, 0x4F, 0xFD, 0xC0, 0xFD,
     0xEA, 0x4F, 0x83, 0x85, 0x39, 0x62, 0xFB, 0xA2, 0x33, 0xD9, 0x2D, 0xB1,
     0x30, 0x6F, 0x88, 0xAB, 0x61, 0xCB, 0x32, 0xEB, 0x30, 0xF9, 0x51, 0xF6,
     0x1F, 0x3A, 0x11, 0x4D, 0xFD, 0x54, 0xD6, 0x3D, 0x43, 0x73, 0x39, 0x16,
     0xCF, 0x3D, 0x29, 0x4A};
 
 TEST(FuzzedDataProvider, ConsumeBytes) {
   FuzzedDataProvider DataProv(Data, sizeof(Data));
-  EXPECT_EQ(std::vector<char>(1, 0x8A), DataProv.ConsumeBytes<char>(1));
+  EXPECT_EQ(std::vector<unsigned char>(1, 0x8A),
+            DataProv.ConsumeBytes<unsigned char>(1));
   EXPECT_EQ(std::vector<uint8_t>(
                 {0x19, 0x0D, 0x44, 0x37, 0x0D, 0x38, 0x5E, 0x9B, 0xAA, 0xF3}),
             DataProv.ConsumeBytes<uint8_t>(10));
 
-  std::vector<unsigned char> UChars = DataProv.ConsumeBytes(24);
+  std::vector<unsigned char> UChars = DataProv.ConsumeBytes<unsigned char>(24);
   EXPECT_EQ(std::vector<unsigned char>({0xDA, 0xAA, 0x88, 0xF2, 0x9B, 0x6C,
                                         0xBA, 0xBE, 0xB1, 0xF2, 0xCF, 0x13,
                                         0xB8, 0xAC, 0x1A, 0x7F, 0x1C, 0xC9,
                                         0x90, 0xD0, 0xD9, 0x5C, 0x42, 0xB3}),
             UChars);
 
   EXPECT_EQ(std::vector<signed char>(Data + 1 + 10 + 24, Data + sizeof(Data)),
             DataProv.ConsumeBytes<signed char>(31337));
 }
 
+TEST(FuzzedDataProvider, ConsumeBytesWithTerminator) {
+  FuzzedDataProvider DataProv(Data, sizeof(Data));
+  EXPECT_EQ(std::vector<unsigned char>({0x8A, 0x00}),
+            DataProv.ConsumeBytesWithTerminator<unsigned char>(1));
+  EXPECT_EQ(std::vector<uint8_t>({0x19, 0x0D, 0x44, 0x37, 0x0D, 0x38, 0x5E,
+                                  0x9B, 0xAA, 0xF3, 111}),
+            DataProv.ConsumeBytesWithTerminator<uint8_t>(10, 111));
+
+  std::vector<unsigned char> UChars =
+      DataProv.ConsumeBytesWithTerminator<unsigned char>(24);
+  EXPECT_EQ(std::vector<unsigned char>(
+                {0xDA, 0xAA, 0x88, 0xF2, 0x9B, 0x6C, 0xBA, 0xBE, 0xB1,
+                 0xF2, 0xCF, 0x13, 0xB8, 0xAC, 0x1A, 0x7F, 0x1C, 0xC9,
+                 0x90, 0xD0, 0xD9, 0x5C, 0x42, 0xB3, 0x00}),
+            UChars);
+
+  std::vector<signed char> Expected(Data + 1 + 10 + 24, Data + sizeof(Data));
+  Expected.push_back(65);
+  EXPECT_EQ(Expected,
+            DataProv.ConsumeBytesWithTerminator<signed char>(31337, 65));
+}
+
 TEST(FuzzedDataProvider, ConsumeBytesAsString) {
   FuzzedDataProvider DataProv(Data, sizeof(Data));
   EXPECT_EQ(std::string("\x8A\x19\x0D\x44\x37\x0D\x38\x5E\x9B\xAA\xF3\xDA"),
             DataProv.ConsumeBytesAsString(12));
   EXPECT_EQ(std::string(Data + 12, Data + sizeof(Data)),
             DataProv.ConsumeBytesAsString(31337));
 }
 
TEST(FuzzedDataProvider, ConsumeRandomLengthString) {
   EXPECT_EQ(size_t(876), String.length());
   EXPECT_EQ(std::string(), DataProv.ConsumeRandomLengthString(1));
 }
 
 TEST(FuzzedDataProvider, ConsumeRemainingBytes) {
   {
     FuzzedDataProvider DataProv(Data, sizeof(Data));
     EXPECT_EQ(std::vector<uint8_t>(Data, Data + sizeof(Data)),
-              DataProv.ConsumeRemainingBytes());
-    EXPECT_EQ(std::vector<uint8_t>(), DataProv.ConsumeRemainingBytes());
+              DataProv.ConsumeRemainingBytes<uint8_t>());
+    EXPECT_EQ(std::vector<uint8_t>(),
+              DataProv.ConsumeRemainingBytes<uint8_t>());
   }
 
   {
     FuzzedDataProvider DataProv(Data, sizeof(Data));
     EXPECT_EQ(std::vector<uint8_t>(Data, Data + 123),
-              DataProv.ConsumeBytes(123));
+              DataProv.ConsumeBytes<uint8_t>(123));
     EXPECT_EQ(std::vector<char>(Data + 123, Data + sizeof(Data)),
               DataProv.ConsumeRemainingBytes<char>());
   }
 }
 
 TEST(FuzzedDataProvider, ConsumeRemainingBytesAsString) {
   {
     FuzzedDataProvider DataProv(Data, sizeof(Data));
     EXPECT_EQ(std::string(Data, Data + sizeof(Data)),
               DataProv.ConsumeRemainingBytesAsString());
     EXPECT_EQ(std::string(""), DataProv.ConsumeRemainingBytesAsString());
   }
 
   {
     FuzzedDataProvider DataProv(Data, sizeof(Data));
     EXPECT_EQ(std::vector<uint8_t>(Data, Data + 123),
-              DataProv.ConsumeBytes(123));
+              DataProv.ConsumeBytes<uint8_t>(123));
     EXPECT_EQ(std::string(Data + 123, Data + sizeof(Data)),
               DataProv.ConsumeRemainingBytesAsString());
   }
 }
 
 TEST(FuzzedDataProvider, ConsumeIntegral) {
   FuzzedDataProvider DataProv(Data, sizeof(Data));
   EXPECT_EQ(int32_t(-903266865), DataProv.ConsumeIntegral<int32_t>());
TEST(FuzzedDataProvider, PickValueInArray) {
   EXPECT_EQ(3, DataProv.PickValueInArray(Array));
   EXPECT_EQ(2, DataProv.PickValueInArray(Array));
 
   EXPECT_EQ(uint8_t(0x9D), DataProv.PickValueInArray(Data));
   EXPECT_EQ(uint8_t(0xBA), DataProv.PickValueInArray(Data));
   EXPECT_EQ(uint8_t(0x69), DataProv.PickValueInArray(Data));
   EXPECT_EQ(uint8_t(0xD6), DataProv.PickValueInArray(Data));
 
+  EXPECT_EQ(uint32_t(777), DataProv.PickValueInArray<uint32_t>({1337, 777}));
+  EXPECT_EQ(uint32_t(777), DataProv.PickValueInArray<uint32_t>({1337, 777}));
+  EXPECT_EQ(uint64_t(1337), DataProv.PickValueInArray<uint64_t>({1337, 777}));
+  EXPECT_EQ(size_t(777), DataProv.PickValueInArray<size_t>({1337, 777}));
+  EXPECT_EQ(int16_t(1337), DataProv.PickValueInArray<int16_t>({1337, 777}));
+  EXPECT_EQ(int32_t(777), DataProv.PickValueInArray<int32_t>({1337, 777}));
+  EXPECT_EQ(int64_t(777), DataProv.PickValueInArray<int64_t>({1337, 777}));
+
   // Exhaust the buffer.
   auto String = DataProv.ConsumeBytesAsString(31337);
-  EXPECT_EQ(size_t(1007), String.length());
+  EXPECT_EQ(size_t(1000), String.length());
   EXPECT_EQ(uint8_t(0x8A), DataProv.PickValueInArray(Data));
 }
 
 TEST(FuzzedDataProvider, ConsumeEnum) {
   FuzzedDataProvider DataProv(Data, sizeof(Data));
   enum class Enum {
     Zero,
     One,
TEST(FuzzedDataProvider, ConsumeEnum) {
   EXPECT_EQ(Enum::Zero, DataProv.ConsumeEnum<Enum>());
 }
 
 TEST(FuzzedDataProvider, remaining_bytes) {
   FuzzedDataProvider DataProv(Data, sizeof(Data));
   EXPECT_EQ(size_t(1024), DataProv.remaining_bytes());
   EXPECT_EQ(false, DataProv.ConsumeBool());
   EXPECT_EQ(size_t(1024 - 1), DataProv.remaining_bytes());
-  EXPECT_EQ(std::vector<uint8_t>(Data, Data + 8), DataProv.ConsumeBytes(8));
+  EXPECT_EQ(std::vector<uint8_t>(Data, Data + 8),
+            DataProv.ConsumeBytes<uint8_t>(8));
   EXPECT_EQ(size_t(1024 - 1 - 8), DataProv.remaining_bytes());
 
   // Exhaust the buffer.
   EXPECT_EQ(std::vector<uint8_t>(Data + 8, Data + sizeof(Data) - 1),
-            DataProv.ConsumeRemainingBytes());
+            DataProv.ConsumeRemainingBytes<uint8_t>());
   EXPECT_EQ(size_t(0), DataProv.remaining_bytes());
 }
 
 int main(int argc, char **argv) {
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }

compiler-rt/trunk/lib/fuzzer/utils/FuzzedDataProvider.h

 //===- FuzzedDataProvider.h - Utility header for fuzz targets ---*- C++ -* ===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 // A single header library providing an utility class to break up an array of
-// bytes (supposedly provided by a fuzzing engine) for multiple consumers.
-// Whenever run on the same input, provides the same output, as long as its
-// methods are called in the same order, with the same arguments.
+// bytes. Whenever run on the same input, provides the same output, as long as
+// its methods are called in the same order, with the same arguments.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_FUZZER_FUZZED_DATA_PROVIDER_H_
 #define LLVM_FUZZER_FUZZED_DATA_PROVIDER_H_
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <cstring>
+#include <initializer_list>
 #include <string>
 #include <type_traits>
 #include <utility>
 #include <vector>
 
 class FuzzedDataProvider {
- public:
-  typedef uint8_t data_type;
-
   // |data| is an array of length |size| that the FuzzedDataProvider wraps to
   // provide more granular access. |data| must outlive the FuzzedDataProvider.
-  FuzzedDataProvider(const uint8_t* data, size_t size)
+  FuzzedDataProvider(const uint8_t *data, size_t size)
       : data_ptr_(data), remaining_bytes_(size) {}
   ~FuzzedDataProvider() = default;
 
   // Returns a std::vector containing |num_bytes| of input data. If fewer than
   // |num_bytes| of data remain, returns a shorter std::vector containing all
-  // of the data that's left.
-  template <typename T = data_type>
-  std::vector<T> ConsumeBytes(size_t num_bytes) {
-    static_assert(sizeof(T) == sizeof(data_type), "Incompatible data type.");
-
+  // of the data that's left. Can be used with any byte sized type, such as
+  // char, unsigned char, uint8_t, etc.
+  template <typename T> std::vector<T> ConsumeBytes(size_t num_bytes) {
     num_bytes = std::min(num_bytes, remaining_bytes_);
+    return ConsumeBytes<T>(num_bytes, num_bytes);
+  }
 
-    // The point of using the size-based constructor below is to increase the
-    // odds of having a vector object with capacity being equal to the length.
-    // That part is always implementation specific, but at least both libc++ and
-    // libstdc++ allocate the requested number of bytes in that constructor,
-    // which seems to be a natual choice for other implementations as well.
-    // To increase the odds even more, we also call |shrink_to_fit| below.
-    std::vector<T> result(num_bytes);
-    std::memcpy(result.data(), data_ptr_, num_bytes);
-    Advance(num_bytes);
-
-    // Even though |shrink_to_fit| is also implementation specific, we expect it
-    // to provide an additional assurance in case vector's constructor allocated
-    // a buffer which is larger than the actual amount of data we put inside it.
-    result.shrink_to_fit();
+  // Similar to |ConsumeBytes|, but also appends the terminator value at the end
+  // of the resulting vector. Useful, when a mutable null-terminated C-string is
+  // needed, for example. But that is a rare case. Better avoid it, if possible,
+  // and prefer using |ConsumeBytes| or |ConsumeBytesAsString| methods.
+  template <typename T>
+  std::vector<T> ConsumeBytesWithTerminator(size_t num_bytes,
+                                            T terminator = 0) {
+    num_bytes = std::min(num_bytes, remaining_bytes_);
+    std::vector<T> result = ConsumeBytes<T>(num_bytes + 1, num_bytes);
+    result.back() = terminator;
     return result;
   }
 
-  // Prefer using |ConsumeBytes| unless you actually need a std::string object.
-  // Returns a std::string containing |num_bytes| of input data. If fewer than
-  // |num_bytes| of data remain, returns a shorter std::string containing all
-  // of the data that's left.
+  // Returns a std::string containing |num_bytes| of input data. Using this and
+  // |.c_str()| on the resulting string is the best way to get an immutable
+  // null-terminated C string. If fewer than |num_bytes| of data remain, returns
+  // a shorter std::string containing all of the data that's left.
   std::string ConsumeBytesAsString(size_t num_bytes) {
-    static_assert(sizeof(std::string::value_type) == sizeof(data_type),
+    static_assert(sizeof(std::string::value_type) == sizeof(uint8_t),
                   "ConsumeBytesAsString cannot convert the data to a string.");
 
     num_bytes = std::min(num_bytes, remaining_bytes_);
     std::string result(
-        reinterpret_cast<const std::string::value_type*>(data_ptr_), num_bytes);
+        reinterpret_cast<const std::string::value_type *>(data_ptr_),
+        num_bytes);
     Advance(num_bytes);
     return result;
   }
 
-  // Returns a number in the range [min, max] by consuming bytes from the input
-  // data. The value might not be uniformly distributed in the given range. If
-  // there's no input data left, always returns |min|. |min| must be less than
-  // or equal to |max|.
-  template <typename T>
-  T ConsumeIntegralInRange(T min, T max) {
+  // Returns a number in the range [min, max] by consuming bytes from the
+  // input data. The value might not be uniformly distributed in the given
+  // range. If there's no input data left, always returns |min|. |min| must
+  // be less than or equal to |max|.
+  template <typename T> T ConsumeIntegralInRange(T min, T max) {
     static_assert(std::is_integral<T>::value, "An integral type is required.");
     static_assert(sizeof(T) <= sizeof(uint64_t), "Unsupported integral type.");
 
     if (min > max)
       abort();
 
     // Use the biggest type possible to hold the range and the result.
     uint64_t range = static_cast<uint64_t>(max) - min;
     uint64_t result = 0;
     size_t offset = 0;
 
     while (offset < sizeof(T) * CHAR_BIT && (range >> offset) > 0 &&
            remaining_bytes_ != 0) {
       // Pull bytes off the end of the seed data. Experimentally, this seems to
       // allow the fuzzer to more easily explore the input space. This makes
       // sense, since it works by modifying inputs that caused new code to run,
       // and this data is often used to encode length of data read by
       // |ConsumeBytes|. Separating out read lengths makes it easier modify the
       // contents of the data that is actually read.
       --remaining_bytes_;
       result = (result << CHAR_BIT) | data_ptr_[remaining_bytes_];
       offset += CHAR_BIT;
     }
 
-    // Avoid division by 0, in the case |range + 1| results in overflow.
+    // Avoid division by 0, in case |range + 1| results in overflow.
     if (range != std::numeric_limits<decltype(range)>::max())
       result = result % (range + 1);
 
     return static_cast<T>(min + result);
   }
 
   // Returns a std::string of length from 0 to |max_length|. When it runs out of
   // input data, returns what remains of the input. Designed to be more stable
   // with respect to a fuzzer inserting characters than just picking a random
   // length and then consuming that many bytes with |ConsumeBytes|.
   std::string ConsumeRandomLengthString(size_t max_length) {
     // Reads bytes from the start of |data_ptr_|. Maps "\\" to "\", and maps "\"
     // followed by anything else to the end of the string. As a result of this
     // logic, a fuzzer can insert characters into the string, and the string
     // will be lengthened to include those new characters, resulting in a more
     // stable fuzzer than picking the length of a string independently from
     // picking its contents.
     std::string result;
+
+    // Reserve the anticipated capaticity to prevent several reallocations.
+    result.reserve(std::min(max_length, remaining_bytes_));
     for (size_t i = 0; i < max_length && remaining_bytes_ != 0; ++i) {
-      char next = static_cast<char>(data_ptr_[0]);
+      char next = ConvertUnsignedToSigned<char>(data_ptr_[0]);
       Advance(1);
       if (next == '\\' && remaining_bytes_ != 0) {
-        next = static_cast<char>(data_ptr_[0]);
+        next = ConvertUnsignedToSigned<char>(data_ptr_[0]);
         Advance(1);
         if (next != '\\')
-          return result;
+          break;
       }
       result += next;
     }
 
     result.shrink_to_fit();
     return result;
   }
 
   // Returns a std::vector containing all remaining bytes of the input data.
-  template <typename T = data_type>
-  std::vector<T> ConsumeRemainingBytes() {
+  template <typename T> std::vector<T> ConsumeRemainingBytes() {
     return ConsumeBytes<T>(remaining_bytes_);
   }
 
   // Prefer using |ConsumeRemainingBytes| unless you actually need a std::string
   // object.
   // Returns a std::vector containing all remaining bytes of the input data.
   std::string ConsumeRemainingBytesAsString() {
     return ConsumeBytesAsString(remaining_bytes_);
   }
 
   // Returns a number in the range [Type's min, Type's max]. The value might
   // not be uniformly distributed in the given range. If there's no input data
   // left, always returns |min|.
-  template <typename T>
-  T ConsumeIntegral() {
+  template <typename T> T ConsumeIntegral() {
     return ConsumeIntegralInRange(std::numeric_limits<T>::min(),
                                   std::numeric_limits<T>::max());
   }
 
   // Reads one byte and returns a bool, or false when no data remains.
   bool ConsumeBool() { return 1 & ConsumeIntegral<uint8_t>(); }
 
-  // Returns a value from |array|, consuming as many bytes as needed to do so.
-  // |array| must be a fixed-size array.
+  // Returns a copy of a value selected from a fixed-size |array|.
   template <typename T, size_t size>
-  T PickValueInArray(T (&array)[size]) {
+  T PickValueInArray(const T (&array)[size]) {
+    static_assert(size > 0, "The array must be non empty.");
     return array[ConsumeIntegralInRange<size_t>(0, size - 1)];
   }
 
+  template <typename T>
+  T PickValueInArray(std::initializer_list<const T> list) {
+    // static_assert(list.size() > 0, "The array must be non empty.");
+    return *(list.begin() + ConsumeIntegralInRange<size_t>(0, list.size() - 1));
+  }
+
   // Return an enum value. The enum must start at 0 and be contiguous. It must
   // also contain |kMaxValue| aliased to its largest (inclusive) value. Such as:
   // enum class Foo { SomeValue, OtherValue, kMaxValue = OtherValue };
-  template <typename T>
-  T ConsumeEnum() {
+  template <typename T> T ConsumeEnum() {
     static_assert(std::is_enum<T>::value, "|T| must be an enum type.");
     return static_cast<T>(ConsumeIntegralInRange<uint32_t>(
         0, static_cast<uint32_t>(T::kMaxValue)));
   }
 
   // Reports the remaining bytes available for fuzzed input.
   size_t remaining_bytes() { return remaining_bytes_; }
 
- private:
-  FuzzedDataProvider(const FuzzedDataProvider&) = delete;
-  FuzzedDataProvider& operator=(const FuzzedDataProvider&) = delete;
+  FuzzedDataProvider(const FuzzedDataProvider &) = delete;
+  FuzzedDataProvider &operator=(const FuzzedDataProvider &) = delete;
 
   void Advance(size_t num_bytes) {
     if (num_bytes > remaining_bytes_)
       abort();
 
     data_ptr_ += num_bytes;
     remaining_bytes_ -= num_bytes;
   }
 
-  const data_type* data_ptr_;
+  template <typename T>
+  std::vector<T> ConsumeBytes(size_t size, size_t num_bytes_to_consume) {
+    static_assert(sizeof(T) == sizeof(uint8_t), "Incompatible data type.");
+
+    // The point of using the size-based constructor below is to increase the
+    // odds of having a vector object with capacity being equal to the length.
+    // That part is always implementation specific, but at least both libc++ and
+    // libstdc++ allocate the requested number of bytes in that constructor,
+    // which seems to be a natural choice for other implementations as well.
+    // To increase the odds even more, we also call |shrink_to_fit| below.
+    std::vector<T> result(size);
+    std::memcpy(result.data(), data_ptr_, num_bytes_to_consume);
+    Advance(num_bytes_to_consume);
+
+    // Even though |shrink_to_fit| is also implementation specific, we expect it
+    // to provide an additional assurance in case vector's constructor allocated
+    // a buffer which is larger than the actual amount of data we put inside it.
+    result.shrink_to_fit();
+    return result;
+  }
+
+  template <typename TS, typename TU> TS ConvertUnsignedToSigned(TU value) {
+    static_assert(sizeof(TS) == sizeof(TU), "Incompatible data types.");
+    static_assert(!std::numeric_limits<TU>::is_signed,
+                  "Source type must be unsigned.");
+    static_assert(std::numeric_limits<TS>::is_signed,
+                  "Destination type must be signed.");
+
+    // TODO(Dor1s): change to `if constexpr` once C++17 becomes mainstream.
+    if (std::numeric_limits<TS>::is_modulo)
+      return static_cast<TS>(value);
+
+    // Avoid using implementation-defined unsigned to signer conversions.
+    // To learn more, see https://stackoverflow.com/questions/13150449.
+    if (value <= std::numeric_limits<TS>::max())
+      return static_cast<TS>(value);
+    else {
+      constexpr auto TS_min = std::numeric_limits<TS>::min();
+      return TS_min + static_cast<char>(value - TS_min);
+    }
+  }
+
+  const uint8_t *data_ptr_;
   size_t remaining_bytes_;
 };
 
-#endif  // LLVM_FUZZER_FUZZED_DATA_PROVIDER_H_
+#endif // LLVM_FUZZER_FUZZED_DATA_PROVIDER_H_