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

Wip: fuzz: init manager #8714

Closed
wants to merge 1 commit into from
Closed

Wip: fuzz: init manager #8714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions test/common/init/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_fuzz_test",
"envoy_cc_test",
"envoy_package",
"envoy_proto_library",
)

envoy_package()
Expand Down Expand Up @@ -32,3 +34,20 @@ envoy_cc_test(
"//test/mocks/init:init_mocks",
],
)

envoy_proto_library(
name = "init_fuzz_proto",
srcs = ["init_fuzz.proto"],
deps = ["//test/fuzz:common_proto"],
)

envoy_cc_fuzz_test(
name = "init_fuzz_test",
srcs = ["init_fuzz_test.cc"],
corpus = "init_corpus",
deps = [
":init_fuzz_proto_cc_proto",
"//source/common/init:manager_lib",
"//test/fuzz:utility_lib",
],
)
13 changes: 13 additions & 0 deletions test/common/init/init_corpus/m1t1
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions test/common/init/init_fuzz.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
syntax = "proto3";

package test.common.init;

// Structured input for init_fuzz_test.

enum InitEntityAction {

// manager
INIT_MANAGER_CREATION = 0;
INIT_MANAGER_INITIALIZE = 1;
INIT_MANAGER_DESTRUCTION = 2;

// target
INIT_TARGET_READY = 3;
INIT_TARGET_DESTRUCTION = 4;

// watcher
INIT_WATCHER_DESTRUCTION = 5;

// add
INIT_MANAGER_ADD_TARGET = 6;
}

message InitAction {
InitEntityAction action = 1;
int32 param = 2;
}

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.

}
71 changes: 71 additions & 0 deletions test/common/init/init_fuzz_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#include "common/common/assert.h"
#include "common/init/manager_impl.h"
#include "common/init/target_impl.h"
#include "common/init/watcher_impl.h"

#include "test/common/init/init_fuzz.pb.h"
#include "test/fuzz/fuzz_runner.h"

#include "absl/strings/str_cat.h"
#include "gtest/gtest.h"

namespace Envoy {
namespace Fuzz {
namespace {
using namespace Init;

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

for (int i = 0; i < manager_size; i++) {
ms_.emplace_back(std::make_unique<ManagerImpl>(absl::StrCat("fuzz_manager_", i)));
ws_.emplace_back(std::make_unique<WatcherImpl>(
absl::StrCat("fuzz_watcher_", i), [this, i]() mutable { ready_flags_[i] = true; }));
ready_flags_.emplace_back(false);
}
for (int i = 0; i < target_size; i++) {
ts_.emplace_back(std::make_unique<TargetImpl>(absl::StrCat("fuzz_target_", i), []() {}));
}
}

void transit(const test::common::init::InitAction& action) {
auto param = action.param();
int mid = (param % 1);
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.

break;
case test::common::init::INIT_MANAGER_ADD_TARGET:

ms_[mid]->add(*ts_[tid]);
break;
case test::common::init::INIT_TARGET_READY:

ts_[tid]->ready();
default:
// NOT_IMPLEMENT_YET
break;
}
};

std::vector<bool> ready_flags_;
std::vector<std::unique_ptr<ManagerImpl>> ms_;
std::vector<std::unique_ptr<TargetImpl>> ts_;
std::vector<std::unique_ptr<WatcherImpl>> ws_;
};

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)

}
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?

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

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.

}
} // namespace
} // namespace Fuzz
} // namespace Envoy