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

fuzz: server config validation fuzz test. #3770

Merged
merged 5 commits into from Jul 15, 2018

Conversation

Projects
None yet
3 participants
@anirudhmurali
Copy link
Member

anirudhmurali commented Jul 1, 2018

Title: fuzz: server config validation fuzz test

Description: This fuzzes the bootstrap config message on server validation mode.

Risk Level: Low

Testing: Tested under oss-fuzz

Signed-off-by: Anirudh M m.anirudh18@gmail.com

fuzz: server config validation fuzz test.
This fuzzes the bootstrap config message on validation mode.

Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch
Copy link
Member

htuch left a comment

Thanks, looks like TSAN failure is legit and related to this change, can you investigate? Seems to be some code around shutdown.

"envoy_cc_test",
"envoy_cc_test_library",
"envoy_package",
"envoy_select_hot_restart",

This comment has been minimized.

@htuch

htuch Jul 1, 2018

Member

A number of these are unnecessary; you only added a envoy_cc_fuzz_test target, so this should be the only new one needed.

load(
"//source/extensions:all_extensions.bzl",
"envoy_all_extensions",
)

This comment has been minimized.

@htuch

htuch Jul 1, 2018

Member

Unused.


#include "server/config_validation/server.h"
#include "server/drain_manager_impl.h"
#include "server/hot_restart_nop_impl.h"

This comment has been minimized.

@htuch

htuch Jul 1, 2018

Member

I think a bunch of these headers aren't needed either; they are vestigial from parts that were removed in server_fuzz_test.

@@ -0,0 +1,46 @@
#include <fstream>

This comment has been minimized.

@htuch

htuch Jul 1, 2018

Member

Please add a comment to this file explaining that it is derived from //test/server:server_fuzz_test.cc and the substantive difference.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 3, 2018

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jul 5, 2018

@anirudhmurali please ping me when this is ready for another pass.

addressed failing tests
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch
Copy link
Member

htuch left a comment

I don't think removing those headers would have fixed the TSAN issue; more likely it's flake, but hopefully unrelated to your code. Can you please verify with:

bazel test //test/server/config_validation:config_fuzz_test -config=clang-tsan --runs_per_test=2000

? Thanks.

"//test/mocks/stats:stats_mocks",
"//source/server:configuration_lib",
"//source/server/config_validation:server_lib",
"//source/server:options_lib",
"//test/test_common:environment_lib",

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

In general, these deps should reflect the headers in config_fuzz_test.cc; can you clean this up?

#include "envoy/config/bootstrap/v2/bootstrap.pb.validate.h"

#include "common/network/address_impl.h"
#include "common/thread_local/thread_local_impl.h"

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

Is this needed below?

#include "server/test_hooks.h"

#include "test/fuzz/fuzz_runner.h"
#include "test/integration/server.h"

This comment has been minimized.

@htuch

htuch Jul 9, 2018

Member

Is this needed below?

cleaned up headers and deps
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 10, 2018

Running bazel test //test/server/config_validation:config_fuzz_test -config=clang-tsan --runs_per_test=2000 shows like few (3-7) failing tests out of 2000. All with the same message: Didn't find a registered implementation for name: 'tls'.

I tried linking envoy_all_extensions(), and tested, it throws the same error message which tsan threw in the first commit:

[critical][assert] external/envoy/source/common/thread_local/thread_local_impl.cc:19] assert failure: shutdown_

The ASSERT(shutdown_); statement in ~InstanceImpl() is what is causing this failure. shutdown_ seems to be a atomic boolean object. Which part of the code might have set shutdown_ to false?

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Jul 10, 2018

@anirudhmurali I suggest grepping (or using other code search tools) to understand how shutdown_ is manipulated. I think that adding back the envoy_all_extensions() was correct; what needs to be done here is figure out why we race in shutdown. We generally expect code committers to take time to understand the framework and fix bugs like this as a gating step before commit.

How often are you seeing the flake with shutdown_?

fixed tsan failures + added envoy_all_extensions()
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@anirudhmurali

This comment has been minimized.

Copy link
Member

anirudhmurali commented Jul 12, 2018

Used validateConfig() instead of creating ValidationInstance object. Things work now, all tests pass well. I guess the server crash was because of some improper initialization of stats_store or access_log_lock.
Should I run more tests to uncover flakes?

Target //test/server/config_validation:config_fuzz_test up-to-date:
  bazel-bin/test/server/config_validation/config_fuzz_test
INFO: Elapsed time: 525.372s, Critical Path: 48.30s
INFO: 3258 processes, processwrapper-sandbox.
INFO: Build completed successfully, 3260 total actions
//test/server/config_validation:config_fuzz_test                         PASSED in 0.6s
  Stats over 2000 runs: max = 0.6s, min = 0.3s, avg = 0.5s, dev = 0.0s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

Tests passed with --runs_per_test=2000 set as well.

@htuch
Copy link
Member

htuch left a comment

LGTM modulo nit.

@@ -14,20 +14,18 @@ namespace Server {
// Derived from //test/server:server_fuzz_test.cc, but starts the server in configuration validation
// mode (quits upon validation of the given config)
DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v2::Bootstrap& input) {

This comment has been minimized.

@htuch

htuch Jul 12, 2018

Member

Nit: Remove this empty line.

addressed comments
Signed-off-by: Anirudh M <m.anirudh18@gmail.com>
@htuch

htuch approved these changes Jul 12, 2018

Copy link
Member

htuch left a comment

Thanks.

@htuch htuch merged commit 7caa887 into envoyproxy:master Jul 15, 2018

12 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment