tests: Add fuzzing harness for strprintf(…) #18009

practicalswift commented at 1:57 PM on January 27, 2020: contributor

Add fuzzing harness for strprintf(…).

Update FuzzedDataProvider.h.

Avoid hitting some issues in tinyformat (reported upstreams in https://github.com/c42f/tinyformat/issues/70).

Found issues in tinyformat:

Issue 1. The following causes a signed integer overflow followed by an allocation of 9 GB of RAM (or an OOM in memory constrained environments):

strprintf("%.777777700000000$", 1.0);

Issue 2. The following causes a stack overflow:

strprintf("%987654321000000:", 1);

Issue 3. The following causes a stack overflow:

strprintf("%1$*1$*", -11111111);

Issue 4. The following causes a NULL pointer dereference:

strprintf("%.1s", (char *)nullptr);

Issue 5. The following causes a float cast overflow:

strprintf("%c", -1000.0);

Issue 6. The following causes a float cast overflow followed by an invalid integer negation:

strprintf("%*", std::numeric_limits<double>::lowest());

practicalswift force-pushed on Jan 27, 2020

MarcoFalke commented at 2:34 PM on January 27, 2020: member

When updating FuzzedDataProvider, please include the exact commit id that it was updated to

practicalswift force-pushed on Jan 27, 2020

practicalswift commented at 2:48 PM on January 27, 2020: contributor

@MarcoFalke Good point! Done! :)

practicalswift force-pushed on Jan 27, 2020

DrahtBot added the label Build system on Jan 27, 2020

DrahtBot added the label Tests on Jan 27, 2020

practicalswift force-pushed on Jan 27, 2020

fanquake removed the label Build system on Jan 27, 2020

in src/test/fuzz/strprintf.cpp:104 in 4534f1ef16 outdated

  43 | +        switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 14)) {
  44 | +        case 0:
  45 | +            (void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
  46 | +            break;
  47 | +        case 1:
  48 | +            (void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());

MarcoFalke commented at 7:38 PM on January 27, 2020:

Why can this not be run in case 0? Seems odd to require the fuzzer to pick a path, when both of them can be run subsequently

in src/test/fuzz/strprintf.cpp:56 in 4534f1ef16 outdated

  51 | +            (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
  52 | +            (void)strprintf(format_string, (signed char*)nullptr);
  53 | +            break;
  54 | +        case 3:
  55 | +            (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
  56 | +            (void)strprintf(format_string, (unsigned char*)nullptr);

MarcoFalke commented at 7:41 PM on January 27, 2020:

Why are the nullptr things under case 2 and case 3? Seems odd to put them there, when nothing needs to be parsed. They can be executed outside of any case, no?

in src/test/fuzz/strprintf.cpp:63 in 4534f1ef16 outdated

  58 | +        case 4:
  59 | +            (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<char>());
  60 | +            // Avoid triggering the following crash bug:
  61 | +            // * strprintf("%.1s", (char*)nullptr);
  62 | +            //
  63 | +            // (void)strprintf(format_string, (char*)nullptr);

MarcoFalke commented at 7:42 PM on January 27, 2020:

Same here and for all cases below

MarcoFalke approved

MarcoFalke commented at 7:45 PM on January 27, 2020: member

ACK

nit: in the commit that bumps the provider: Could limit the commit subject to a reasonable length, i.e. move the url to the commit body

practicalswift force-pushed on Jan 27, 2020

practicalswift commented at 9:31 PM on January 27, 2020: contributor

@MarcoFalke Thanks for reviewing! Feedback addressed. Please re-review :)

tests: Update FuzzedDataProvider.h from upstream (LLVM)

Upstream revision: https://github.com/llvm/llvm-project/blob/a44ef027ebca1598892ea9b104d6189aeb3bc2f0/compiler-rt/include/fuzzer/FuzzedDataProvider.h

6ef04912af

tests: Add fuzzer strprintf to FUZZERS_MISSING_CORPORA (temporarily) ccc3c76e2b

tests: Add fuzzing harness for strprintf(...) cc668d06fb

practicalswift force-pushed on Jan 27, 2020

chamajcpradel approved

chamajcpradel commented at 1:30 AM on January 29, 2020: none

Sirs. This is very good test.

Empact commented at 2:49 AM on January 30, 2020: member

Would be helpful to comment on "why" in each commit message - e.g. I was curious to look into why the FuzzedDataProvider.h update was necessary and found that the ConsumeFloatingPoint was introduced recently. https://chris.beams.io/posts/git-commit/#why-not-how

MarcoFalke referenced this in commit 7fcaa8291c on Jan 30, 2020

MarcoFalke merged this on Jan 30, 2020

MarcoFalke closed this on Jan 30, 2020

jasonbcox referenced this in commit 5ab783b981 on Oct 20, 2020

practicalswift deleted the branch on Apr 10, 2021

kittywhiskers referenced this in commit 8f357599cf on Feb 27, 2022

kittywhiskers referenced this in commit 08e22e79de on Feb 27, 2022

kittywhiskers referenced this in commit 23832197b9 on Feb 28, 2022

kittywhiskers referenced this in commit ad831c21da on Feb 28, 2022

kittywhiskers referenced this in commit 547398c238 on Feb 28, 2022

kittywhiskers referenced this in commit 039c5e24fa on Mar 13, 2022

kittywhiskers referenced this in commit 8dc6222163 on Mar 24, 2022

DrahtBot locked this on Aug 16, 2022