stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy by jmarantz · Pull Request #8779 · envoyproxy/envoy

jmarantz · 2019-10-27T01:51:29Z

Description: Stats have a reasonably generous hard limit of 64k bytes per "."-separated segment. However fuzzing is evidently happy to make longer strings, based on the fuzz crash report

This change also has the benefit of further reducing memory use; see gold-file updates in the PR.

Risk Level: low
Testing: just this one fuzz test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

Thanks, curious what enforces the stat name limits.

test/common/stats/symbol_table_fuzz_test.cc

htuch · 2019-10-28T01:30:45Z

test/common/stats/symbol_table_fuzz_test.cc

+
+  if (str.size() >= StatNameMaxSize) {
+    size_t halfway = str.size() / 2;
+    addSymbol(pool, symbol_table, str.substr(0, halfway));


No need to have this kind of smart logic; if you just cap the string at StatNameMaxSize, the fuzzer will be able to do what is implied in this logic if it is helpful to the exploration.

I'm changing the symbol-table impl to simply not have this limit, which should simplify fuzzing. It works I think but still not quite ready for review. Will ping again when ready.

This is ready for review now.

asraa

I figured out how to max_len to at least libfuzzer in oss-fuzz, but would it benefit if we only consume up to StatNameMaxSize per symbol?

I will test run the max_len for one @htuch's fuzzers for now, cc you on it and lmk

test/common/stats/symbol_table_fuzz_test.cc

…, or on the length of a fake-symbol table stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…es or symbols. it's bytes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-10-29T19:30:36Z

ready for review now

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-10-30T00:01:35Z

/azp run envoy-macos

azure-pipelines · 2019-10-30T00:01:43Z

Azure Pipelines successfully started running 1 pipeline(s).

source/common/stats/symbol_table_impl.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ass. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…xy#8779). Signed-off-by: Joshua Marantz <jmarantz@google.com>

…(discovered in #8779). (#9360) * Add test for release(), which was not working (discovered in #8779). Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

test/common/stats/symbol_table_fuzz_test.cc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-12-18T22:09:31Z

/retest

repokitteh-read-only · 2019-12-18T22:09:36Z

🔨 rebuilding ci/circleci: api (failed build)

🐱

Caused by: a #8779 (comment) was created by @jmarantz.

see: more, trace.

asraa

This LGTM from the fuzzing / test utility side now

jmarantz · 2019-12-20T16:50:02Z

Over to @mattklein123 for senior maintainer review. Thank you!

mattklein123

LGTM from a skim.

mattklein123 · 2019-12-20T17:23:55Z

test/common/stats/stat_test_utility.cc

 #endif
 }

+// TODO(jmarantz): this utility is intended to be costs both for unit tests


"costs both"? Feel free to fix in a follow up.

Thanks Matt, rolling that into #9355

* master: (167 commits) stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy (envoyproxy#8779) Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. (envoyproxy#9362) tools: API boosting support for using decls and enum constants. (envoyproxy#9418) Fix incorrect cluster InitializePhase type (envoyproxy#9379) build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (envoyproxy#9427) fuzz: fix incorrect evaluator test (envoyproxy#9402) server: fix bogus startup log message (envoyproxy#9404) tools: Add protoxform tests (envoyproxy#9241) api: options after import (envoyproxy#9413) misc: use std::move instead of constructing a copy (envoyproxy#9415) tools: API boosting support for rewriting elaborated types. (envoyproxy#9375) docs: fix invalid transport_socket value (envoyproxy#9403) fix typo in docs (envoyproxy#9394) srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. (envoyproxy#9366) api: generate whole directory and sync (envoyproxy#9382) bazel: Add load statements for proto_library (envoyproxy#9367) Fix typo (envoyproxy#9388) Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result) (envoyproxy#9290) http1 encode trailers in chunk encoding (envoyproxy#8667) Add mode to PipeInstance (envoyproxy#8423) ...

…(discovered in envoyproxy#8779). (envoyproxy#9360) * Add test for release(), which was not working (discovered in envoyproxy#8779). Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

…mits, using the new MemBlock wrapper for memcpy (envoyproxy#8779) * Avoid crashes in fuzz-tests due to input strings being too long. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

Avoid crashes in fuzz-tests due to input strings being too long.

5d40fc4

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz requested a review from htuch October 27, 2019 12:37

mattklein123 assigned asraa Oct 27, 2019

htuch reviewed Oct 28, 2019

View reviewed changes

asraa reviewed Oct 28, 2019

View reviewed changes

test/common/stats/symbol_table_fuzz_test.cc Outdated Show resolved Hide resolved

jmarantz added 2 commits October 28, 2019 18:15

Eliminate 64k limit on # of segments in a real symbol-table stat-name…

8f9ab93

…, or on the length of a fake-symbol table stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

some cleanup

beb4684

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~stats: Avoid asserts in fuzz-tests due to input strings being too long.~~ WiP stats: Avoid asserts in fuzz-tests due to input strings being too long. Oct 28, 2019

jmarantz added 8 commits October 28, 2019 22:30

Share helper methods for encoding/decoding symbols and sizes.

d143365

Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanup

5ef1a49

Signed-off-by: Joshua Marantz <jmarantz@google.com>

typo

c425551

Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix confusing about whether the encoding-length was the number of byt…

f680ccc

…es or symbols. it's bytes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix over-aggressive inequality for fake symbol tables

9fee9f5

Signed-off-by: Joshua Marantz <jmarantz@google.com>

don't recompute the number of bytes consumed.

184fbed

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-fuzzer-not-too-long

6ac3dee

Signed-off-by: Joshua Marantz <jmarantz@google.com>

remove dynamo-stats test change which is not needed

9c62a9e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WiP stats: Avoid asserts in fuzz-tests due to input strings being too long.~~ stats: Avoid asserts in fuzz-tests due to input strings being too long. Oct 29, 2019

jmarantz changed the title ~~stats: Avoid asserts in fuzz-tests due to input strings being too long.~~ stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits Oct 29, 2019

use totalSizeBytes to simplify some code.

4482ca7

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch reviewed Oct 30, 2019

View reviewed changes

source/common/stats/symbol_table_impl.cc Outdated Show resolved Hide resolved

jmarantz added 5 commits October 31, 2019 22:17

Merge branch 'master' into stats-fuzzer-not-too-long

14220bb

Signed-off-by: Joshua Marantz <jmarantz@google.com>

add MemBlock abstraction to isolate risky memory operations.

4d345d0

Signed-off-by: Joshua Marantz <jmarantz@google.com>

reduce need to do byte access to the memory buffers.

5b6f0db

Signed-off-by: Joshua Marantz <jmarantz@google.com>

remove remainder of memcpy and pointer arithmetic outside MemBlock cl…

70ffe33

…ass. Signed-off-by: Joshua Marantz <jmarantz@google.com>

format

9f1eae7

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits~~ WiP stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits Nov 2, 2019

some cleanup -- still needs more unit tests.

cf33e18

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Dec 11, 2019

util: Add MemBlockBuilder class to help manage blocks of memory for safe memcpy #9306

Merged

jmarantz changed the title ~~stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, adding a new MemBlock wrapper for memcpy~~ WiP stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, adding a new MemBlock wrapper for memcpy Dec 11, 2019

jmarantz added 3 commits December 14, 2019 12:59

Merge branch 'master' into stats-fuzzer-not-too-long

006b5c7

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-fuzzer-not-too-long

aa70e5e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

post-merge cleanup

9033ac6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WiP stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, adding a new MemBlock wrapper for memcpy~~ stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy Dec 15, 2019

jmarantz added 2 commits December 15, 2019 12:53

more cleanups

d19d722

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Cleanup lifetime issues in test infrastructure.

ae1da1b

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Dec 16, 2019

stats: add mechanism to create dynamic tokens without locks or sharing #9355

Merged

jmarantz added a commit to jmarantz/envoy that referenced this pull request Dec 16, 2019

Add test for release(), which was not working (discovered in envoypro…

2445781

…xy#8779). Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 4 commits December 16, 2019 15:09

update MemBlockBuilder interface change

bc9871e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-fuzzer-not-too-long

ae0b1dd

Signed-off-by: Joshua Marantz <jmarantz@google.com>

popuplate -> setCacacity

f4ef7c6

Signed-off-by: Joshua Marantz <jmarantz@google.com>

add direct fuzzing of the encoder.

3edfac7

Signed-off-by: Joshua Marantz <jmarantz@google.com>

asraa reviewed Dec 18, 2019

View reviewed changes

test/common/stats/symbol_table_fuzz_test.cc Outdated Show resolved Hide resolved

Share testing infrastructure between fuzz-tests and unit-tests.

4f5be6c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

asraa approved these changes Dec 19, 2019

View reviewed changes

jmarantz assigned mattklein123 Dec 20, 2019

mattklein123 approved these changes Dec 20, 2019

View reviewed changes

jmarantz merged commit 6eeced6 into envoyproxy:master Dec 20, 2019

jmarantz deleted the stats-fuzzer-not-too-long branch December 20, 2019 18:18

Conversation

jmarantz commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Oct 29, 2019

Uh oh!

jmarantz commented Oct 30, 2019

Uh oh!

azure-pipelines bot commented Oct 30, 2019

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Dec 18, 2019

Uh oh!

repokitteh-read-only bot commented Dec 18, 2019

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Dec 20, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmarantz commented Oct 27, 2019 •

edited

Loading