-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AFL fuzzing #91
AFL fuzzing #91
Conversation
CMakeLists.txt
Outdated
# fuzzing applications | ||
# | ||
|
||
add_executable( add fuzz/add.cpp ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we can rename add.cpp to fuzz or something)
fuzz/add/testcases/input.txt
Outdated
@@ -0,0 +1,4 @@ | |||
1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should delete this and have a test runner that reads the JSON files and generates the necessary directories / input files.
Previous PR for reference: #88 |
@@ -0,0 +1,4 @@ | |||
1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we should delete this long term, but i left it around so there's an example of how to run afl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can settle on an input file format after more usage and examples, but this looks fine for now.
CMakeLists.txt
Outdated
@@ -15,7 +15,7 @@ set( CMAKE_INCLUDE_CURRENT_DIR ON ) | |||
include_directories( program/src/ ) | |||
|
|||
# gcc compiler/linker flags | |||
add_compile_options( -ggdb -Wall -Wextra -Werror -m64 ) | |||
add_compile_options( -ggdb -Wall -Wextra -Werror -Wno-unused-function -m64 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? If actually needed, it would be preferable to mark these functions with [[maybe_unused]]
, etc., rather than disabling the warning globally.
program/src/oracle/pd.h
Outdated
{ | ||
pd_t r[1]; | ||
pd_sub( r, n1, n2, p ); | ||
return r->v_ > 0L; | ||
} | ||
|
||
static void pd_sqrt( pd_t *r, pd_t *val, const int64_t *f ) | ||
[[maybe_unused]] static void pd_sqrt( pd_t *r, pd_t *val, const int64_t *f ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix these either by marking them as inline
or moving the function definitions to a pd.c
file. I'll make that change separately though; this is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README.md
Outdated
You can run these tests using a command like: | ||
|
||
``` | ||
docker run -t --platform linux/amd64 -v "$(pwd)"/findings:/home/pyth/pyth-client/findings pyth-fuzz sh -c "./afl/afl-fuzz -i ./pyth-client/pyth/tests/fuzz/add/testcases -o ./pyth-client/findings ./pyth-client/build/fuzz add" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might look cleaner to break this into multiple lines, e.g.,
docker run -t \
--platform linux/amd64 \
-v "$(pwd)"/findings:/home/pyth/pyth-client/findings \
pyth-fuzz sh -c \
"./afl/afl-fuzz" \
"-i ./pyth-client/pyth/tests/fuzz/add/testcases" \
"-o ./pyth-client/findings" \
"./pyth-client/build/fuzz add"
|
||
``` | ||
docker run -t --platform linux/amd64 -v "$(pwd)"/findings:/home/pyth/pyth-client/findings pyth-fuzz sh -c "./pyth-client/build/fuzz add < ./pyth-client/findings/crashes/id\:000000\,sig\:06\,src\:000000\,op\:flip1\,pos\:0" | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multi-line here as well.
@@ -0,0 +1,4 @@ | |||
1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can settle on an input file format after more usage and examples, but this looks fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Avoid duplicating definitions across compilation units. - Allow including `pd.h` in test sources without adding -Wno-unused-function (pyth-network#91).
- Allows including `pd.h` in test sources without adding `-Wno-unused-function`. See pyth-network#91
Tracking related changes with Issue #99. |
- Allows including `pd.h` in test sources without adding `-Wno-unused-function`. - See pyth-network#91
|
||
std::cout.precision(12); | ||
|
||
if (strcmp(argv[1], "add") == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a better way to do this?
I thought a little bit about how we can avoid some of the issues with the previous approach, and here's a possible solution. The idea is that we make a single fuzzing program that dispatches to different property-based tests based on its command line arguments. We can then declare test cases using JSON by specifying the command line argument + stdin. It's not a perfect solution -- there's still some C++ boilerplate and such -- but it seems like a good effort/reward tradeoff.
This code isn't the cleanest, but hopefully the idea is clear. If you like the idea, I'll clean it up. I also haven't addressed any of the comments on the previous PR yet.