Add more fuzzers and libFuzzer harness #553

isaacbrodsky · 2021-12-22T22:50:06Z

This adds additional fuzzing harnesses (consult README.md in the PR for which functions should be covered and uncovered - a few are left uncovered since they are trivial) and also adds an adapter from AFL++ to libFuzzer. The idea being we can move the fuzzer harness from https://github.com/google/oss-fuzz/blob/master/projects/h3/h3_fuzzer.c to in repo (replaces #448) and cover all functions.

Possible improvements include testing the build using afl++ itself and hooking this up to OSS-Fuzz.

cc @AdamKorcz

.github/workflows/test-fuzzer.yml

coveralls · 2021-12-22T22:57:21Z

Coverage decreased (-0.04%) to 98.141% when pulling d501e51 on isaacbrodsky:llvm-fuzzer-harness into a1157d8 on uber:master.

isaacbrodsky · 2021-12-22T23:09:58Z

The windows failure appears to be related to a problem fixed in #550 about those functions not having DECLSPEC.

isaacbrodsky · 2022-01-03T16:58:25Z

CMakeLists.txt

+    if(ENABLE_LIBFUZZER)
+        list(APPEND H3_COMPILE_FLAGS -fsanitize=fuzzer,address,undefined)
+        list(APPEND H3_LINK_FLAGS -fsanitize=fuzzer,address,undefined)
+    endif()


Note that these settings don't really work for the other binaries since main gets redefined.

nrabinowitz

This looks great overall - I think my confusion here is mostly my own lack of understanding of fuzzers, not problems with the code, but more comments might be helpful overall.

nrabinowitz · 2022-01-03T19:29:38Z

CMakeLists.txt

@@ -44,6 +44,9 @@ option(BUILD_BENCHMARKS "Build benchmarking applications." ON)
 option(BUILD_FUZZERS "Build fuzzer applications (for use with afl)." ON)
 option(BUILD_FILTERS "Build filter applications." ON)
 option(BUILD_GENERATORS "Build code generation applications." ON)
+# If ON, libfuzzer settings are used to build the fuzzer harnesses. If OFF, a frontend
+# for afl++ is provided instead.


Can you explain the frontend part of this?

frontend in this context means an implementation of main that accepts arguments in a way that AFL can integrate with. Should I expand on the comments for this option?

nrabinowitz · 2022-01-03T19:38:38Z

src/apps/fuzzers/fuzzerCompact.c

+    H3Index *compacted = calloc(inputSize, sizeof(H3Index));
+    H3_EXPORT(compactCells)(input, compacted, inputSize);
+
+    // fuzz uncompactCells using output of above


Does this make sense? Would we miss cases for uncompactCells that can't be generated by the output of compact?

A second run of uncompactCells is done below, using the original input data. Not sure if this additional run of uncompact on the compact output is needed but it seemed like a reasonable check to append.

nrabinowitz · 2022-01-03T19:39:25Z

src/apps/fuzzers/fuzzerCompact.c

+
+    // fuzz compactCells
+    H3Index *compacted = calloc(inputSize, sizeof(H3Index));
+    H3_EXPORT(compactCells)(input, compacted, inputSize);


We don't need the error here?

Correct. I consider failing and returning the appropriate error code acceptable behavior. Unacceptable behavior would be undefined behavior, faulting/crashing/out of bounds reads, etc.

Edit: Oh, I see we use the output of this function later. In this particular case I don't think it's too bad since we are guaranteed that the memory pointed to by compacted has been zeroed by calloc. If this were data on the stack using it in a potentially unintialized state could be bad. I will update this one to check the error code.

nrabinowitz · 2022-01-03T19:49:43Z

src/apps/fuzzers/fuzzerH3SetToLinkedGeo.c

+    }
+    const inputArgs *args = (const inputArgs *)data;
+    if (args->sz >= 1024) {
+        return 0;


For my info - it seems like there's a class of issue here where the user passes in an array and a size that don't match, and we get an invalid array access. Since we can't deal with this (we don't know the array size), are we assuming this is outside the bounds of testing, and is user error? Since this is a pretty serious error, do we need to indicate where users might need to be careful in their own code not to hit this?

This is a serious issue with memory management in C. I considered this case to be out of scope for the fuzzers since I don't know of any way to make H3 resilient to this kind of error.

We should be careful to document the expected array sizes when users pass memory into H3 so this can be correctly implemented on the user side.

nrabinowitz · 2022-01-03T19:52:06Z

src/apps/fuzzers/fuzzerIndexIO.c

+        return 0;
+    }
+    inputArgs args;
+    memcpy(&args, data, sizeof(inputArgs));


For my info - why memcpy here and casts in the other tests?

This is needed because we modify args.str on the next line to ensure it meets the contract that it is null terminated. This cannot be done on the original buffer because of const and because libfuzzer will complain.

Worth a comment in that case I think

Will add. Come to think of it we may wish to address this in the API of stringToH3 by passing the buffer length.

nrabinowitz · 2022-01-03T19:58:56Z

src/apps/fuzzers/fuzzerPolygonToCells.c

+    H3Error err = H3_EXPORT(maxPolygonToCellsSize)(geoPolygon, res, &sz);
+    if (!err && sz < MAX_SZ) {
+        if (sz < 0) {
+            printf("Oh no - sz is negative\n");


I'm confused here - should this return 0 and exit? If not, why print the error?

As it is it will crash on line 57. Not sure if this will be relevant after #551.

nrabinowitz · 2022-01-03T20:05:40Z

src/apps/fuzzers/fuzzerPolygonToCells.c

+        return 0;
+    }
+    geoPolygon.holes = calloc(geoPolygon.numHoles, sizeof(GeoLoop));
+    size_t offset = sizeof(inputArgs);


I'm quite confused here - isn't the fuzzer only going to provide sizeof(inputArgs) worth of data? Where does the rest of the data after offset come from in that case?

I will need to check if this will work with AFL (might need to adjust the seed file so it has data after inputArgs). For libFuzzer it does not know about inputArgs so it will generate larger test cases.

src/apps/fuzzers/fuzzerResolutions.c

src/apps/fuzzers/fuzzerCellArea.c

src/apps/fuzzers/fuzzerCompact.c

ajfriend · 2022-01-16T19:58:44Z

src/apps/fuzzers/fuzzerResolutions.c

+    double out;
+    H3_EXPORT(getHexagonAreaAvgKm2)(args->res, &out);
+    H3_EXPORT(getHexagonAreaAvgM2)(args->res, &out);
+    H3_EXPORT(getHexagonEdgeLengthAvgKm)(args->res, &out);
+    H3_EXPORT(getHexagonEdgeLengthAvgM)(args->res, &out);
+
+    int64_t outInt;
+    H3_EXPORT(getNumCells)(args->res, &outInt);
+
+    H3Index pentagons[12];
+    H3_EXPORT(getPentagons)(args->res, pentagons);


This is probably just a reflection of me not knowing how fuzzers work, but when you're doing multiple sequential tests like this, does it become harder to test the last function because you need to "get past" all the previous ones? Is there any value is splitting out each function separately? But that's also a lot of annoying boilerplate...

It's correct that if any of the previous functions called were to crash, this function wouldn't be exercised. But in that case we'd still have found a crash warranting investigation anyways. If we explicitly terminate the fuzzing run before reaching this function (i.e. if getNumCells were to return an error then we return 0), then that would not be a good test because this function's invocation would be dependent on the previous function. (And that is not part of the contract of calling this function, as with the memory allocation size functions.) That doesn't happen here so I believe this is a valid exercise of all the functions in this file.

src/apps/fuzzers/fuzzerLocalIj.c

Isaac Brodsky added 8 commits December 22, 2021 13:43

Add LLVM fuzzer harness

0277ce5

Add AFL++ test case generator

1c8ec2e

Fuzz more gridDisk functions

b89c76b

add fuzzerH3SetToLinkedGeo

79d2e2c

Add more fuzzers

a28a807

Additional fuzzers

9445cbb

add fuzzerVertexes

f971dcf

Add test-fuzzer script

e0c8841

isaacbrodsky requested review from nrabinowitz, dfellis and ajfriend December 22, 2021 22:50

Isaac Brodsky added 2 commits December 22, 2021 14:51

Fix linux build

007b7c3

Fix fuzzerIndexIO

02abb99

isaacbrodsky commented Dec 22, 2021

View reviewed changes

.github/workflows/test-fuzzer.yml Outdated Show resolved Hide resolved

isaacbrodsky and others added 14 commits December 22, 2021 15:11

test-fuzzer use subshell for ls

caedc90

Update test-fuzzer again

de5a2c3

Fix test-fuzzer again

8bbc36a

fuzzerCompact

021c994

Update readme

2195863

libFuzzer tests

4a65123

reformat header

1e7066e

README updates

0ede718

fuzzerDirectedEdge

65b4ef3

fuzzerLocalIj

79b1f44

fix fuzzerDirectedEdge build

25360ef

Fix fuzzer programs

ac4b918

remove logging

1145bce

remove h3Println

cd14266

add fuzzerPoylgonToCells

76532d8

isaacbrodsky commented Jan 3, 2022

View reviewed changes

nrabinowitz approved these changes Jan 3, 2022

View reviewed changes

Isaac Brodsky added 2 commits January 3, 2022 13:11

Update per review

84bc4e9

Merge branch 'master' into llvm-fuzzer-harness

323d9e9

isaacbrodsky force-pushed the llvm-fuzzer-harness branch from 248b1cf to 323d9e9 Compare January 3, 2022 22:13

Isaac Brodsky added 5 commits January 3, 2022 14:17

Add comment on memcpy per review

e04c62c

Fix potential crash in vertexRotations

0016f1c

Merge branch 'master' into llvm-fuzzer-harness

7807131

Catch possible failure in getIcosahedronFaces

4b4e623

Don't assert specific error in testVertex

d501e51

isaacbrodsky mentioned this pull request Jan 11, 2022

Add polygonToCellsNoHoles fuzzer #557

Merged

ajfriend reviewed Jan 11, 2022

View reviewed changes

src/apps/fuzzers/fuzzerCellArea.c Show resolved Hide resolved

ajfriend reviewed Jan 11, 2022

View reviewed changes

src/apps/fuzzers/fuzzerCompact.c Show resolved Hide resolved

ajfriend reviewed Jan 11, 2022

View reviewed changes

src/apps/fuzzers/fuzzerCompact.c Show resolved Hide resolved

ajfriend reviewed Jan 11, 2022

View reviewed changes

src/apps/fuzzers/fuzzerCompact.c Show resolved Hide resolved

This was referenced Jan 13, 2022

Bug fixes for compactCells #558

Merged

Bug fixes for directed edge #559

Merged

Bug fixes for h3SetToLinkedGeo #560

Merged

Bug fixes for Local IJ functions #562

Merged

ajfriend reviewed Jan 16, 2022

View reviewed changes

ajfriend reviewed Jan 17, 2022

View reviewed changes

src/apps/fuzzers/fuzzerLocalIj.c Show resolved Hide resolved

ajfriend approved these changes Jan 17, 2022

View reviewed changes

isaacbrodsky merged commit 9330ee1 into uber:master Jan 17, 2022

isaacbrodsky deleted the llvm-fuzzer-harness branch January 17, 2022 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more fuzzers and libFuzzer harness #553

Add more fuzzers and libFuzzer harness #553

isaacbrodsky commented Dec 22, 2021 •

edited

Loading

coveralls commented Dec 22, 2021 •

edited

Loading

isaacbrodsky commented Dec 22, 2021

isaacbrodsky Jan 3, 2022

nrabinowitz left a comment

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

nrabinowitz Jan 3, 2022

isaacbrodsky Jan 3, 2022

ajfriend Jan 16, 2022

isaacbrodsky Jan 16, 2022 •

edited

Loading

Add more fuzzers and libFuzzer harness #553

Add more fuzzers and libFuzzer harness #553

Conversation

isaacbrodsky commented Dec 22, 2021 • edited Loading

coveralls commented Dec 22, 2021 • edited Loading

isaacbrodsky commented Dec 22, 2021

Choose a reason for hiding this comment

nrabinowitz 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

isaacbrodsky Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

isaacbrodsky commented Dec 22, 2021 •

edited

Loading

coveralls commented Dec 22, 2021 •

edited

Loading

isaacbrodsky Jan 16, 2022 •

edited

Loading