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.

Sign up for GitHub

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

Jump to bottom

Wip: fuzz: init manager #8714

Closed
wants to merge 1 commit into from
Closed

Wip: fuzz: init manager #8714

wants to merge 1 commit into from

Conversation

lambdai
Copy link
Contributor

@lambdai lambdai commented Oct 22, 2019 •
edited
Loading

Signed-off-by: Yuchen Dai silentdai@gmail.com

Description:
The goal is to verify the init manager behavior since I am adding shared target in #8268 #8523

Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

@lambdai
init fuzz
99ae4a5 
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
htuch
htuch suggested changes Oct 24, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks @lambdai, a few questions, but nice to see other folks contributing fuzzers. Also, CC @asraa who might have some thoughts about the most effective way to fuzz this subsystem.
/wait

test/common/init/init_fuzz.proto
}

message InitTestCase {
repeated InitAction actions = 1;
Copy link
Member

Choose a reason for hiding this comment

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

@asraa do you think we should advocate for FuzzDataProvider here (for performance, and because the trace format is simple), or are there good reasons to keep this a proto trace format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FuzzDataProvider! TIL. Will explore and introduce

Copy link
Member

Choose a reason for hiding this comment

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

There is a downside; it's harder to hand-craft corpuses (and understand failing corpus files). It would be ideal if there was some CLI tool that translated from a proto representation of the binary data to the raw binary for FuzzDataProvider, so that both of these can be better handled. Not sure if anyone has built this kind of thing already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no!! I had suggested this change yesterday but I didn't know GH review process has an extra submit button.

Yeah, I personally think that if the actions are simple enough to describe, using a FuzzedDataProvider to chose oneof some number of actions is better.

But if there's a strong case for the order of the scenarios bringing interesting behavior (the ordering of ready, add target, destruction) without necessarily strictly increasing code coverage, I think the proto trace format is better. I only think though that if you decide to do the proto trace format way, that you enter a good number of corpus entries.

test/common/init/init_fuzz_test.cc

DEFINE_PROTO_FUZZER(const test::common::init::InitTestCase& input) {
if (input.actions_size() < 3) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually it would be:
if initDriver determine the input as "not a legit input` (e.g. use-after-free, or 0 ready() is signal so as not initialized)

test/common/init/init_fuzz_test.cc
return;
}
InitDriver driver(1, 1);
ENVOY_LOG_MISC(critical, "input size {}", input.actions_size());
Copy link
Member

Choose a reason for hiding this comment

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

Please make all log messages <= debug, since any emitted logs will slow down fuzzing. The fuzzer will emit critical in ClusterFuzz.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will remove when it's ready to review

test/common/init/init_fuzz_test.cc
for (const auto& act : input.actions()) {
driver.transit(act);
}
RELEASE_ASSERT(driver.ready_flags_[0], "manager not initialized");
Copy link
Member

Choose a reason for hiding this comment

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

How do you know this will be non-empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The driver should be initialized with reasonable number of manager.

test/common/init/init_fuzz_test.cc

class InitDriver {
public:
InitDriver(int manager_size, int target_size) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer fixed size types in Envoy code style, unsigned where it makes sense like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

test/common/init/init_fuzz_test.cc
if (input.actions_size() < 3) {
return;
}
InitDriver driver(1, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this parameterized when it's only used with a fixed size?

test/common/init/init_fuzz_test.cc
int tid = (param % 1);
switch (action.action()) {
case test::common::init::INIT_MANAGER_INITIALIZE:
ms_[mid]->initialize(*ws_[mid]);
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide an explanation of what you are aiming to verify by fuzzing? I'm guessing it's not crash resilience, so it's some correctness property. Is the range of possible inputs so large that we can't just use some unit tests or brute force testing? Etc.

@htuch htuch requested a review from asraa October 24, 2019 05:55
@lambdai
Copy link
Contributor Author

lambdai commented Oct 24, 2019

@htuch Thank you for the early review!
The plan is to fuzz test the SharedTargetImpl introduced in #8523 because I don't want to enumerate the possible sequences where N init manager initialize 1 shared target.

The sequences has limited number but it's about 10~ish ! if N = 2

I plan to use the InitDriver to determine the expectation and handle invalid sequence.

Most of the comments are awe to the working-in-progress InitDriver

@lambdai
Copy link
Contributor Author

lambdai commented Oct 24, 2019

Close for now to avoid confusion

Thanks for the reference of data provider!

@lambdai lambdai closed this Oct 24, 2019
@lambdai lambdai mentioned this pull request Nov 13, 2019
Merged
htuch pushed a commit that referenced this pull request Nov 25, 2019
@lambdai @htuch
init: add shared target (#9008)
95dfc51 
Extracted from #8523

@mergeconflict mentioned we can also maintain a collection of TargetImpl(and a initialize_ flag, std::once_flag) at the target owner.

I agree it works but a SharedTarget provides stronger cohesion.

Follow up:

* update #8523 on top of this PR
* complete fuzz test #8714

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch requested changes

@asraa asraa Awaiting requested review from asraa

Assignees

@htuch htuch

Labels
waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lambdai @asraa @htuch