Skip to content
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

Fuzzing improvements #1617

Merged
merged 5 commits into from
Nov 17, 2020
Merged

Fuzzing improvements #1617

merged 5 commits into from
Nov 17, 2020

Conversation

mmisono
Copy link
Collaborator

@mmisono mmisono commented Nov 14, 2020

This PR includes three changes. Basically, add a main function for fuzzing for convenience.

1

Although the current bpftrace can be fuzzed with AFL, there are several
possible optimizations for fuzzing. This add -DBUILD_FUZZ CMake options
and it defined FUZZ. We can use it in the source code to optimize bpftrace
for fuzzing. Of course, such bpftrace would not work for tracing.

2

Add a simple main for fuzzing. This would make it easier to optimize some
processing for fuzzing and integrate with another fuzzer.
There are several duplicated lines in the original main function. I'll
plan to unify them later.

3

I realized that reading debug/tracing/available_filter_functions takes
time, and that slow down fuzzing severely. Especially, BTF's constructor
always call the function no matter if it does not use the function. So this
patches changes it, and now BTF's constructor does not call the
function, and always report tracee function is available. It should be
OK since during fuzzing we don't load the program in the kernel and
trace. Now average AFL's exec speed becomes from 1 to up to 20 in my machine.

I think we can add more several things to improve fuzzing, but I'll make this PR for the time being to make review easy.

Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

src/main.cpp Outdated
}
catch (const std::exception& ex)
{
// failed to compile
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should abort here, and let the fuzzer notify us. Does invalid IR result in an exception?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm doesnt really care. Unless you really screwed up, in that case it segvs or goes oom. Thats why we have the codegen-validator check to find our IR mistakes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. then I leave it as is.

src/main.cpp Outdated

#else

#if !defined(NODE_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i'd prefer a special fuzz only binary instead of doing ifdefs everywhere. That way you can just disable BTF support completely instead of what happens now.

So in src/CMakelist.txt

add_executable(bpftrace_fuzz, ..files.. fuzz_main.cpp ...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also want to fuzz BTF related part, but the separate a binary seems good to me. I'll try it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added fuzz_main.cpp, and now when building fuzz binary that name is bpftrace_fuzz. I'll rebase commits if it's desirable.

@mmisono
Copy link
Collaborator Author

mmisono commented Nov 15, 2020

Ah, before merging I need to update the document. I'll do it later.

@mmisono mmisono added the do-not-merge Changes are not ready to be merged into master yet label Nov 15, 2020
@mmisono mmisono removed the do-not-merge Changes are not ready to be merged into master yet label Nov 15, 2020
@mmisono
Copy link
Collaborator Author

mmisono commented Nov 15, 2020

Updated.

set(BPFTRACE bpftrace_fuzz)
else()
set(MAIN_SRC main.cpp)
set(BPFTRACE bpftrace)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be good to unset (set(MAIN_SRC "") iirc) these at the end to avoid them leaking into cmake state.

Although the current bpftrace can be fuzzed with AFL, there are several
possible optimizations for fuzzing. This add -DBUILD_FUZZ CMake options
and it defined FUZZ. We can use it in the source code to optimize bpftrace
for fuzzing. Of course such bpftrace would not work for tracing.
Add simple main for fuzzing. This would make it easier to optimize some
processing for fuzzing and integrate with another fuzzer.
There are several duplicated lines in the original main function. I'll
plan to unify them later.
I realized that reading debug/tracing/available_filter_functions takes
time, and that slow down fuzzing severely. Especially, BTF's constructor
always call the function no matter if it does not use the function. So this
patches changes it, and now BTF's constructor does not call the
function, and always report tracee function is available. It should be
OK since during fuzzing we don't load the program in the kernel and
trace. Now average AFL's exec speed becomes from 1 to up to 20 in my machine.
@mmisono mmisono merged commit 593c8c5 into bpftrace:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants