tests: Add fuzzing harness for ISO-8601 related functions #17291

practicalswift commented at 10:20 PM on October 28, 2019: contributor

Add fuzzing harness for ISO-8601 related functions.

Testing this PR

Run:

$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/parse_iso8601
…

fanquake added the label Tests on Oct 28, 2019

DrahtBot commented at 12:33 AM on October 29, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17229 (tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions by practicalswift)
#17225 (tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. by practicalswift)
#17136 (tests: Add fuzzing harness for various PSBT related functions by practicalswift)
#17129 (tests: Add fuzzing harness for miniscript::FromScript(...) by practicalswift)
#17109 (tests: Add fuzzing harness for various functions consuming only integrals by practicalswift)
#17093 (tests: Add fuzzing harness for various CTx{In,Out} related functions by practicalswift)
#17071 (tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions by practicalswift)
#17051 (tests: Add deserialization fuzzing harnesses by practicalswift)
#17050 (tests: Add fuzzing harnesses for functions parsing scripts, numbers, JSON and HD keypaths (bip32) by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj commented at 7:32 PM on October 29, 2019: member

Concept ACK :smile:

in src/test/fuzz/parse_iso8601.cpp:25 in 72b77c7614 outdated

  20 | +
  21 | +    const std::string iso8601_datetime = FormatISO8601DateTime(random_time);
  22 | +    const int64_t parsed_time_1 = ParseISO8601DateTime(iso8601_datetime);
  23 | +    if (random_time >= 0) {
  24 | +        assert(parsed_time_1 >= 0);
  25 | +        if (iso8601_datetime.length() == 20) {

elichai commented at 12:50 PM on October 30, 2019:

can you explain the rational for this if? shouldn't it always be 20?

practicalswift commented at 1:03 PM on October 30, 2019:

Unfortunately not:

$ FormatISO8601DateTime(std::numeric_limits<int64_t>::min())
-219246529--2134443519-00T08:29:52Z

$ FormatISO8601DateTime(0)
1970-01-01T00:00:00Z

$ FormatISO8601DateTime(std::numeric_limits<int64_t>::max())
219250468--2134443519-00T15:30:07Z

Thus we're only expecting the round-trip to work in the 20 char case :)

elichai commented at 1:11 PM on October 30, 2019:

what the heck is that :O

elichai commented at 12:52 PM on October 30, 2019: contributor

Concept ACK. ~~Though it might also be nice to fuzz them separately. (specifically the Parse function). Because if Format works correctly than parse here is always checked under sunny day scenario.~~

EDIT: You already do that :)

tests: Add fuzzing harness for ISO-8601 related functions d5dbb4898c

docs: Add undefined to --with-sanitizers=fuzzer,address 595cc9bcaf

in test/fuzz/test_runner.py:18 in 72b77c7614 outdated

  11 | @@ -12,6 +12,10 @@
  12 |  import subprocess
  13 |  import logging
  14 |  
  15 | +# Fuzzers known to lack a seed corpus in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus
  16 | +FUZZERS_MISSING_CORPORA = [
  17 | +    "parse_iso8601",
  18 | +]

MarcoFalke commented at 1:28 PM on October 30, 2019:

Can remove this?

Also: (I know it is not related to this pull request), but could you please add undefined to --with-sanitizers=fuzzer,address in the doc/fuzzing.md readme?

MarcoFalke commented at 1:29 PM on October 30, 2019:

btw, the seeds look rather boring this time: https://github.com/bitcoin-core/qa-assets/commit/0710bfd617ed8cbb337b7be79efba771632e9b1f

practicalswift commented at 1:34 PM on October 30, 2019:

Done! Please re-review :)

practicalswift force-pushed on Oct 30, 2019

MarcoFalke referenced this in commit 341e8d355d on Oct 30, 2019

MarcoFalke merged this on Oct 30, 2019

MarcoFalke closed this on Oct 30, 2019

in src/test/fuzz/parse_iso8601.cpp:21 in 595cc9bcaf

  16 | +    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
  17 | +
  18 | +    const int64_t random_time = fuzzed_data_provider.ConsumeIntegral<int64_t>();
  19 | +    const std::string random_string = fuzzed_data_provider.ConsumeRemainingBytesAsString();
  20 | +
  21 | +    const std::string iso8601_datetime = FormatISO8601DateTime(random_time);

MarcoFalke commented at 8:28 PM on December 18, 2019:

This will fail when the year overflows after adding 1900. E.g.

$ base64 crash-59af3ac3c0a4395f770dbbd0f956cd680aa65a3d 
AAAAALQ3CVA=

practicalswift commented at 9:48 PM on December 18, 2019:

@MarcoFalke I've seen that case. I'm running with this patch in my local tree:

diff --git a/src/util/time.cpp b/src/util/time.cpp
index 2afff2626..c311c6a49 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -98,6 +98,14 @@ std::string FormatISO8601DateTime(int64_t nTime) {
 #else
     gmtime_r(&time_val, &ts);
 #endif
+    if (!((ts.tm_year >= 0 && ts.tm_year <= 9999-1900) &&
+          (ts.tm_mon >= 0 && ts.tm_mon <= 11) &&
+          (ts.tm_mday >= 1 && ts.tm_mday <= 31) &&
+          (ts.tm_hour >= 0 && ts.tm_hour <= 24) &&
+          (ts.tm_min >= 0 && ts.tm_min <= 59) &&
+          (ts.tm_sec >= 0 && ts.tm_sec <= 59))) {
+        return {};
+    }
     return strprintf("%04i-%02i-%02iT%02i:%02i:%02iZ", ts.tm_year + 1900, ts.tm_mon + 1, ts.tm_mday, ts.tm_hour, ts.tm_min, ts.tm_sec);
 }
 
@@ -109,6 +117,11 @@ std::string FormatISO8601Date(int64_t nTime) {
 #else
     gmtime_r(&time_val, &ts);
 #endif
+    if (!((ts.tm_year >= 0 && ts.tm_year <= 9999-1900) &&
+          (ts.tm_mon >= 0 && ts.tm_mon <= 11) &&
+          (ts.tm_mday >= 1 && ts.tm_mday <= 31))) {
+          return {};
+    }
     return strprintf("%04i-%02i-%02i", ts.tm_year + 1900, ts.tm_mon + 1, ts.tm_mday);
 }

Perhaps worth submitting upstreams?

jasonbcox referenced this in commit efd365b98e on Jul 13, 2020

practicalswift deleted the branch on Apr 10, 2021

kittywhiskers referenced this in commit 3cbbf15812 on Feb 27, 2022

kittywhiskers referenced this in commit 3bac40886b on Feb 27, 2022

kittywhiskers referenced this in commit e90b8596f8 on Feb 28, 2022

kittywhiskers referenced this in commit 2d0f83cc82 on Feb 28, 2022

kittywhiskers referenced this in commit 9b55d769cb on Feb 28, 2022

kittywhiskers referenced this in commit 00b246f296 on Mar 13, 2022

kittywhiskers referenced this in commit 166232b6f3 on Mar 24, 2022

DrahtBot locked this on Aug 18, 2022