Add libfuzzer compatible fuzz harness #7512

nathaniel-brough · 2023-04-14T23:29:34Z

As discussed in #4088 the fuzz_simplify harness is an easy candidate to begin support for coverage-driven feedback required for high-performance fuzz testing.

nathaniel-brough · 2023-04-14T23:35:35Z

I wasn't sure on how best to manage the Cmake build files for fuzz tests, so I'm certainly open to suggestions on some of the changes here :)

alexreinking · 2023-04-15T01:03:20Z

Because of the special toolchain requirements, I think this test should be moved to test/fuzz/simplify.cpp. That folder should get its own CMakeLists.txt containing the following:

tests(GROUPS fuzz
      SOURCES
      simplify.cpp
      )
target_compile_options(fuzz_simplify PRIVATE -fsanitize=fuzzer,address)
target_link_options(fuzz_simplify PRIVATE -fsanitize=fuzzer,address)

The test/CMakeLists.txt should be modified to contain the following:

# near the top:
include(CheckCXXCompilerFlag)

# toward the bottom
check_cxx_compiler_flag("-fsanitize=fuzzer,address" HAS_FUZZ_FLAGS)
cmake_dependent_option(
    WITH_TEST_FUZZ "Build fuzz tests" ON
    HAS_FUZZ_FLAGS OFF
)
if (WITH_TEST_FUZZ)
    add_subdirectory(fuzz)
endif ()

The guiding principle here is "ask for what you want", here the -fsanitize=fuzzer,address flag. Checking a proxy like the compiler/platform combo is suspect. In fact, there are compilers for both Windows and macOS that support libFuzzer.

nathaniel-brough · 2023-04-15T19:48:15Z

Thanks for the quick response, I've made the suggested changes and also added some of the precursors for multi-fuzzer engine builds for oss-fuzz :)

test/CMakeLists.txt

test/fuzz/simplify.cpp

test/fuzz/CMakeLists.txt

nathaniel-brough · 2023-04-17T19:29:16Z

Looks like I've got at least a couple of formatting things to fix.

On a different note, I've got a corresponding draft PR to integrate with OSS-fuzz. A couple of things I'll need for that integration to go through;

A primary contact email address (associated with a google/gmail account).
A list of email addresses that you want to have CC'd into automatic bug reports from oss-fuzz.
A contributor here with write access to Halide to comment mentioning that you approve of integration with oss-fuzz.

If you are happy for me to do so, I can pull the email addresses from the git log, it's worth noting however that these emails will be listed in plaintext in the oss-fuzz repo.

test/fuzz/CMakeLists.txt

test/CMakeLists.txt

steven-johnson · 2023-04-17T23:40:08Z

Part of the point of checking is that it's less necessary to know the complete matrix of compiler vendors and versions

No, but when it inevitably fails somewhere, having comments and/or messages to help explain would be nice

As discussed in halide#4088 the fuzz_simplify harness is an easy candidate to begin support for coverage driven feedback that is required for high performance fuzz testing.

test/CMakeLists.txt

nathaniel-brough · 2023-04-19T00:48:02Z

Ok so I think there is a bit of a bug with cmakes CHECK_CXX_COMPILER_FLAG as it doesn't seem to support checking sanitiser flags. Something to do with it needing those sanitizer flags during linking as well, so the check fails. I'm going to have a bit of a go at using CheckCXXSourceCompiles instead as it seems to give a bit more control over how/what flags are used during compiling/linking.

test/CMakeLists.txt

Co-authored-by: Alex Reinking <[email protected]>

nathaniel-brough · 2023-04-20T16:51:18Z

I think I've covered most things here. Is this build failure related to my changes? The error is;

The following tests FAILED:
	243 - correctness_multi_way_select (Failed)
Errors while running CTest
program finished with exit code 8
elapsedTime=578.168036

steven-johnson · 2023-04-24T18:06:31Z

LGTM, waiting on @alexreinking approval

alexreinking

This is great! Thanks for the contribution!

nathaniel-brough

@steven-johnson would you mind double checking these? I'll have a PR up shortly with these changes.

nathaniel-brough · 2023-05-04T17:49:13Z

test/fuzz/simplify.cpp

 Type fuzz_types[] = {UInt(1), UInt(8), UInt(16), UInt(32), Int(8), Int(16), Int(32)};
 const int fuzz_type_count = sizeof(fuzz_types) / sizeof(fuzz_types[0]);

 std::string fuzz_var(int i) {
    return std::string(1, 'a' + i);
 }

-Expr random_var() {
-    int fuzz_count = rng() % fuzz_var_count;


TODO: change as per #7546

nathaniel-brough · 2023-05-04T17:50:05Z

test/fuzz/simplify.cpp

    std::vector<int> divisors = {t.lanes()};
    for (int dd = 2; dd < t.lanes(); dd++) {
        if (t.lanes() % dd == 0) {
            divisors.push_back(dd);
        }
    }

-    return divisors[rng() % divisors.size()];


Looks fine as per #7546

nathaniel-brough · 2023-05-04T17:51:40Z

test/fuzz/simplify.cpp

    if (T.is_int() && T.bits() == 32) {
        overflow_undef = true;
    }
    if (T.is_scalar()) {
-        int var = rng() % fuzz_var_count + 1;


Should be fdp.ConsumeIntegralInRange<int>(0, fuzz_var_count) as per #7546 NOTE: drop the + 1

The semantics of fuzz_var_count aren't clear to me -- in various for loops we do for (int i = 0; i < fuzz_var_count; i++) which implies that we need to limit the range to max=fuzz_var_count-1?

Yeah looking at this again the var variable seems to only get used once on line 56, so I could probably just get rid of it and replace the condition on line 56 with fdp.ConsumeBool.

I'm guessing it was +1 in the beginning so that the condition on line 56 was sometimes true/false. In any case it seems like a really roundabout way of doing it. I'm just going to replace it with the consumebool method.

nathaniel-brough · 2023-05-04T17:54:23Z

test/fuzz/simplify.cpp

            return cast(T, v1);
        } else {
            if (overflow_undef) {
                // For Int(32), we don't care about correctness during
                // overflow, so just use numbers that are unlikely to
                // overflow.
-                return cast(T, (int)(rng() % 256 - 128));
+                return cast(T, fdp.ConsumeIntegralInRange<int>(-128, 128));


Should be fdp.ConsumeIntegralInRange<int>(-128, 127) as per #7546

nathaniel-brough · 2023-05-04T17:55:37Z

test/fuzz/simplify.cpp

            } else {
-                return cast(T, (int)(rng() - RAND_MAX / 2));
+                return cast(T, fdp.ConsumeIntegralInRange<int>(-RAND_MAX / 2, RAND_MAX / 2));


Should be fdp.ConsumeIntegralInRange<int>(-RAND_MAX / 2, RAND_MAX / 2 -1) as per #7546

...except, RAND_MAX applies to the rand() call in stdlib; does libfuzzer also make the same promise about range?

Well the allowable range is going to be std::numeric_limits<int>::min() -> std::numeric_limits<int>::min() which is what ConsumeIntegral<int> will do but then if you want to constrain it further you can use the ConsumeIntegralInRange so no there aren't any promises other than the given range.

The FuzzedDataProvider class is just taking a stream of bytes from the fuzzer and turning them into other types,in this case an integral. The FuzzedDataProvider class just makes managing this easier as it essentially moves a cursor along the byte stream so that you aren't consuming the the same data twice.

nathaniel-brough · 2023-05-04T17:56:18Z

test/fuzz/simplify.cpp

-    int op = rng() % op_count;
+    Expr a = random_expr(fdp, T, depth);
+    Expr b = random_expr(fdp, T, depth);
+    int op = fdp.ConsumeIntegralInRange<int>(0, op_count);


Should be fdp.ConsumeIntegralInRange<int>(0, op_count-1) as per #7546

or better yet, use PickValueInArray()?

True that would be better

nathaniel-brough · 2023-05-04T17:56:35Z

test/fuzz/simplify.cpp

    }

    const int bin_op_count = sizeof(make_bin_op) / sizeof(make_bin_op[0]);
    const int bool_bin_op_count = sizeof(make_bool_bin_op) / sizeof(make_bool_bin_op[0]);
    const int op_count = bin_op_count + bool_bin_op_count + 5;

-    int op = rng() % op_count;
+    int op = fdp.ConsumeIntegralInRange<int>(0, op_count);


Should be fdp.ConsumeIntegralInRange<int>(0, op_count-1)

Yes.. but looking at this now, the logic is a little fragile; the switch statement has implicit assumptions about the count and ordering of the two op arrays. What happens if someone inserts a new one? Would be nice to make this a bit more robust, e.g.:

Use custom enum values to associate the op functions with the switch statements?

Or maybe, rework make_bin_op and make_bool_bin_op arrays so that each entry is a pair<> with the existing op + a lambda function that contains the code from the relevant switch case, so that the switch goes away entirely?

(I realize the fragility was pre-existing before your changes, but improving the resilience here seems easy and worthwhile while we are here)

Yeah, looking at this again a second time I'm struggling to understand what is going on here. Where did this 5 come from.

Oh nevermind I see what is going on here... that's a bit wack. I'll refactor as per your suggestions.

steven-johnson · 2023-05-04T18:08:19Z

test/fuzz/simplify.cpp

-Type random_type(int width) {
-    Type T = fuzz_types[rng() % fuzz_type_count];
+Type random_type(FuzzedDataProvider &fdp, int width) {
+    Type T = fdp.PickValueInArray(fuzz_types);


Hadn't noticed this call existed -- perhaps we should aggressively use it in other cases too where possible (e.g. line 47, line 102)

It won't work on line 47 as its a vector and even though it would have been trivial to implement it doesn't work on vectors. But 102 could definetely use that.

nathaniel-brough mentioned this pull request Apr 14, 2023

Integrating with OSS-Fuzz #4088

Open

nathaniel-brough force-pushed the main branch from b85d5af to f21ac2b Compare April 14, 2023 23:33

nathaniel-brough force-pushed the main branch from f21ac2b to 1049fc5 Compare April 15, 2023 19:45

nathaniel-brough force-pushed the main branch from 1049fc5 to bfe1cd8 Compare April 16, 2023 22:31

steven-johnson reviewed Apr 17, 2023

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

steven-johnson reviewed Apr 17, 2023

View reviewed changes

test/fuzz/simplify.cpp Show resolved Hide resolved

steven-johnson reviewed Apr 17, 2023

View reviewed changes

test/fuzz/CMakeLists.txt Outdated Show resolved Hide resolved

alexreinking requested changes Apr 17, 2023

View reviewed changes

test/fuzz/CMakeLists.txt Outdated Show resolved Hide resolved

test/CMakeLists.txt Outdated Show resolved Hide resolved

steven-johnson requested a review from halidebuildbots April 18, 2023 16:42

steven-johnson mentioned this pull request Apr 18, 2023

Use env vars instead of CMake vars for Dawn/Node bindings #7422

Merged

nathaniel-brough added 2 commits April 18, 2023 15:00

Add libfuzzer compatible fuzz harness

b69d717

As discussed in halide#4088 the fuzz_simplify harness is an easy candidate to begin support for coverage driven feedback that is required for high performance fuzz testing.

fix: Ensure cmake cache variables dominate

2c7c83c

nathaniel-brough force-pushed the main branch from bfe1cd8 to 2c7c83c Compare April 18, 2023 22:00

docs: Add documentation on fuzzing configs

5fb8eb4

nathaniel-brough commented Apr 18, 2023

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

test/CMakeLists.txt Show resolved Hide resolved

nathaniel-brough force-pushed the main branch from 7266dc3 to 2b4acfd Compare April 18, 2023 22:25

build: Log when the toolchain doesn't support libfuzzer

d918853

nathaniel-brough force-pushed the main branch from 2b4acfd to d918853 Compare April 18, 2023 23:03

nathaniel-brough and others added 2 commits April 19, 2023 11:24

fix: Corrects detection of compilers support libfuzzer

16b2a54

Merge branch 'main' into pr/7512

dbfdd79

steven-johnson added the buildbot_test_everything Buildbots should run all available tests on this PR (unless build_test_nothing is set). label Apr 19, 2023

fix: format simplify fuzzer

f972e00

alexreinking reviewed Apr 19, 2023

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

chore: change test program to multiline string

2a02014

Co-authored-by: Alex Reinking <[email protected]>

steven-johnson added 2 commits April 20, 2023 11:36

Merge branch 'main' into pr/7512

f53ec05

Merge branch 'main' of https://github.com/silvergasp/Halide into pr/7512

ff589cc

steven-johnson requested review from alexreinking and steven-johnson April 21, 2023 22:56

steven-johnson approved these changes Apr 21, 2023

View reviewed changes

Merge branch 'main' into pr/7512

a1a8e7d

alexreinking approved these changes May 1, 2023

View reviewed changes

alexreinking merged commit 044a8cf into halide:main May 1, 2023

nathaniel-brough mentioned this pull request May 4, 2023

fuzz tests call ConsumeIntegralInRange() incorrectly #7546

Closed

nathaniel-brough commented May 4, 2023

View reviewed changes

steven-johnson reviewed May 4, 2023

View reviewed changes

ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024

Add libfuzzer compatible fuzz harness (halide#7512)

dc298c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add libfuzzer compatible fuzz harness #7512

Add libfuzzer compatible fuzz harness #7512

nathaniel-brough commented Apr 14, 2023

nathaniel-brough commented Apr 14, 2023

alexreinking commented Apr 15, 2023 •

edited

Loading

nathaniel-brough commented Apr 15, 2023 •

edited

Loading

nathaniel-brough commented Apr 17, 2023

steven-johnson commented Apr 17, 2023

nathaniel-brough commented Apr 19, 2023

nathaniel-brough commented Apr 20, 2023 •

edited

Loading

steven-johnson commented Apr 24, 2023

alexreinking left a comment

nathaniel-brough left a comment

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

steven-johnson May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

steven-johnson May 4, 2023

steven-johnson May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

steven-johnson May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

steven-johnson May 4, 2023

steven-johnson May 4, 2023

nathaniel-brough May 4, 2023

nathaniel-brough May 4, 2023

steven-johnson May 4, 2023

nathaniel-brough May 4, 2023

Add libfuzzer compatible fuzz harness #7512

Add libfuzzer compatible fuzz harness #7512

Conversation

nathaniel-brough commented Apr 14, 2023

nathaniel-brough commented Apr 14, 2023

alexreinking commented Apr 15, 2023 • edited Loading

nathaniel-brough commented Apr 15, 2023 • edited Loading

nathaniel-brough commented Apr 17, 2023

steven-johnson commented Apr 17, 2023

nathaniel-brough commented Apr 19, 2023

nathaniel-brough commented Apr 20, 2023 • edited Loading

steven-johnson commented Apr 24, 2023

alexreinking left a comment

Choose a reason for hiding this comment

nathaniel-brough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreinking commented Apr 15, 2023 •

edited

Loading

nathaniel-brough commented Apr 15, 2023 •

edited

Loading

nathaniel-brough commented Apr 20, 2023 •

edited

Loading