[compiler-rt] FuzzedDataProvider: add ConsumeData method.

[Dor1s]

Jonathan @metzman please take a look when you get a chance. I'll add the tests later. Two comments:

1. ConsumeMemory was requested by the users a few times and I think makes sense
2. hash is my own idea and I'm not sure it's a good one. What's your opinion?

Thank you!

[metzman]

Mostly LGTM.
What's hash for? To seed an RNG?
I'm not 100% hashing the whole array is the right way to do things but it might be better than nothing.
I guess changing this is possible but not ideal since corpus elements won't map to the same behavior if we change it to say, the last byte or something similar.

compiler-rt/include/fuzzer/FuzzedDataProvider.h

252

I think saying "Not recommended in general" is a bit weird. What's wrong with it for the intended purpose?
maybe say it's the std::hash. When I read "the hash" my first thought was "which? md5? sha1? etc."

255

I think we need to include <functional> to use std::hash right?

256

I would guess this will be a long way off.

[Dor1s]

remove hash method, rename ConsumeMemory to ConsumeData, add test

[Dor1s]

Thanks for the review. I can't think of a good application for the hash. Anything I come up with is better to be done either without hashing or even without FDP.

• Removed the hash
• Added test
• Renamed ConsumeMemory to ConsumeData, as the former sounds a bit ambiguous (like memory consumption is more about memory being allocated, while here it's just values being copied, i.e. data)

[Dor1s]

I'm gonna rebase and commit, since there isn't much to review compared to the PS#1 (just a test).

compiler-rt/include/fuzzer/FuzzedDataProvider.h

252

yeah, this is one more argument not to add such method, deleted

255

good catch, I've also forgot <> for the template parameter, but anyway it's deleted now.

[metzman]

still LGTM

compiler-rt/include/fuzzer/FuzzedDataProvider.h

246

Might be better not to use the same name "Consume" as other methods since this method is used a bit differently than other methods which return their value.

[Dor1s]

compiler-rt/include/fuzzer/FuzzedDataProvider.h

246

Hmm... I agree it's different, but what would be a good alternative? Copy or Write are also not very intuitive as the internal pointer gets advanced and the data is in fact being consumed.

Also feels like we could just get rid of the Consume prefix for all the methods, but it's too late now :D

[metzman]

compiler-rt/include/fuzzer/FuzzedDataProvider.h

246

Don't have an alternative. Probably best to leave as is.

compiler-rt/include/fuzzer/FuzzedDataProvider.h

if (max > zero && min < zero && max > min + std::numeric_limits<T>::max()) {
       }
     } else {
       range = max - min;
     }
 
     return result + range * ConsumeProbability<T>();
   }
 
+  // Writes |num_bytes| of input data to the given destination pointer. If there
+  // is not enough data left, writes all remaining bytes. Return value is the
+  // number of bytes written.
+  // In general, it's better to avoid using this function, but it may be useful
+  // in cases when it's necessary to fill a certain buffer or object with
+  // fuzzing data.
+  size_t ConsumeData(void *destination, size_t num_bytes) {
+    num_bytes = std::min(num_bytes, remaining_bytes_);
+    CopyAndAdvance(destination, num_bytes);
+    return num_bytes;
+  }
+
   // 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;
 
+  void CopyAndAdvance(void *destination, size_t num_bytes) {
+    std::memcpy(destination, data_ptr_, num_bytes);
+    Advance(num_bytes);
+  }
+
   void Advance(size_t num_bytes) {
     if (num_bytes > remaining_bytes_)
       abort();
 
     data_ptr_ += num_bytes;
     remaining_bytes_ -= num_bytes;
   }
 
   template <typename T>
-  std::vector<T> ConsumeBytes(size_t size, size_t num_bytes_to_consume) {
+  std::vector<T> ConsumeBytes(size_t size, size_t num_bytes) {
     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);
     if (size == 0) {
-      if (num_bytes_to_consume != 0)
+      if (num_bytes != 0)
         abort();
       return result;
     }
 
-    std::memcpy(result.data(), data_ptr_, num_bytes_to_consume);
-    Advance(num_bytes_to_consume);
+    CopyAndAdvance(result.data(), 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();
     return result;
   }
 

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

TEST(FuzzedDataProvider, ConsumeFloatingPoint) {
   ASSERT_NEAR(std::numeric_limits<double>::lowest(),
               DataProv.ConsumeFloatingPoint<double>(), 1e-10);
   ASSERT_FLOAT_EQ(float(123.0),
                   DataProv.ConsumeFloatingPointInRange<float>(123.0, 777.0));
   ASSERT_DOUBLE_EQ(double(-13.37), DataProv.ConsumeFloatingPointInRange<double>(
                                        -13.37, 31.337));
 }
 
+TEST(FuzzedDataProvider, ConsumeData) {
+  FuzzedDataProvider DataProv(Data, sizeof(Data));
+  uint8_t Buffer[10] = {};
+  EXPECT_EQ(sizeof(Buffer), DataProv.ConsumeData(Buffer, sizeof(Buffer)));
+  std::vector<uint8_t> Expected(Data, Data + sizeof(Buffer));
+  EXPECT_EQ(Expected, std::vector<uint8_t>(Buffer, Buffer + sizeof(Buffer)));
+
+  EXPECT_EQ(size_t(2), DataProv.ConsumeData(Buffer, 2));
+  Expected[0] = Data[sizeof(Buffer)];
+  Expected[1] = Data[sizeof(Buffer) + 1];
+  EXPECT_EQ(Expected, std::vector<uint8_t>(Buffer, Buffer + sizeof(Buffer)));
+
+  // Exhaust the buffer.
+  EXPECT_EQ(std::vector<uint8_t>(Data + 12, Data + sizeof(Data)),
+            DataProv.ConsumeRemainingBytes<uint8_t>());
+  EXPECT_EQ(size_t(0), DataProv.ConsumeData(Buffer, sizeof(Buffer)));
+  EXPECT_EQ(Expected, std::vector<uint8_t>(Buffer, Buffer + sizeof(Buffer)));
+}
+
 int main(int argc, char **argv) {
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }