Fuzz: added fuzz test for network filter "local_ratelimit" by jianwen612 · Pull Request #11608 · envoyproxy/envoy

jianwen612 · 2020-06-16T21:23:03Z

Commit Message:
Additional Description:

Added fuzz test code for network filter "local_ratelimit" which is defined in extensions/filters/network/local_ratelimit/local_ratelimit.h

Added protobuf as the fuzz test input.

Added a basic test case to cover the code.

Risk Level: low
Testing:
Increased the coverage from 1.2% to 100% on the filter's .cc and .h files.

/cc @asraa
/cc @samkerner

) Signed-off-by: Auni Ahsan <auni@google.com> Signed-off-by: jianwen <jianwendong@google.com>

…s branch merge up-to-date master branch Signed-off-by: jianwen <jianwendong@google.com> Signed-off-by: jianwen <jianwendong@google.com>

Signed-off-by: jianwen <jianwendong@google.com>

50 ms Signed-off-by: jianwen <jianwendong@google.com>

Signed-off-by: jianwen <jianwendong@google.com>

activeFilter. Signed-off-by: jianwen <jianwendong@google.com>

Signed-off-by: jianwen <jianwendong@google.com>

added protobuf file. Signed-off-by: jianwen <jianwendong@google.com>

Signed-off-by: jianwen <jianwendong@google.com>

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

Signed-off-by: jianwen <jianwendong@google.com>

sqkerner

This change looks good to me.

htuch

Generally looks great, a couple comment. Thanks.
/wait

htuch · 2020-06-16T23:18:28Z

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

+      }
+    }
+  } catch (const EnvoyException e) {
+    ENVOY_LOG_MISC(debug, "EnvoyException in fuzz test: {}", e.what());


Why is an arbitrary EnvoyException OK behavior here?

Why is an arbitrary EnvoyException OK behavior here?

I catch EnvoyException because in
source/extensions/filters/network/local_ratelimit/local_ratelimit.cc
There is an EnvoyException thrown:
if (fill_interval_ < std::chrono::milliseconds(50)) { throw EnvoyException("local rate limit token bucket fill timer must be >= 50ms"); }

Do you have any suggestion on how to deal with this case?

Jianwen has some history about this --
I think it's OK to add this constraint in the token bucket proto (which is only used in this filter), but the question came up of having 100% line coverage in the filter.

But the middleground might be just to catch the exception when creating the config

Jianwen has some history about this --
I think it's OK to add this constraint in the token bucket proto (which is only used in this filter), but the question came up of having 100% line coverage in the filter.

But the middleground might be just to catch the exception when creating the config

That makes sense. Modified.

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

test/extensions/filters/network/local_ratelimit/BUILD

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

asraa

Thanks! Would love to see some stats like exec/sec as well.

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

added TODO removed unused deps and headers Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 · 2020-06-17T17:26:09Z

Thanks! Would love to see some stats like exec/sec as well.

At the beginning, exec/s: 300-500
#453 NEW cov: 5713 ft: 16233 corp: 127/28Kb lim: 4096 exec/s: 453 rss: 333Mb L: 415/935 MS: 8 CopyPart-ShuffleBytes-CopyPart-ShuffleBytes-ChangeBit-Custom-InsertByte-Custom-

After #[3000] : exec/s: 170
#3006 REDUCE cov: 80648 ft: 115428 corp: 297/94Kb lim: 4096 exec/s: 176 rss: 479Mb L: 461/1163 MS: 1 CustomCrossOver-

After #[10000]: exec/s: 111
#10116 REDUCE cov: 80653 ft: 118605 corp: 411/153Kb lim: 4096 exec/s: 111 rss: 768Mb L: 994/1582 MS: 3 CrossOver-Custom-CustomCrossOver-

Update: I ran it for additional several times, each time I deleted the corpus but the exec/s is different. Sometimes when #[4000] it slows down to 100. I remember the first time when I ran it, the exec/s slowed down to 30 exec/s. Is this normal?

Signed-off-by: jianwen <jianwendong@google.com>

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

sqkerner · 2020-06-18T17:40:06Z

Thanks! Would love to see some stats like exec/sec as well.

At the beginning, exec/s: 300-500
#453 NEW cov: 5713 ft: 16233 corp: 127/28Kb lim: 4096 exec/s: 453 rss: 333Mb L: 415/935 MS: 8 CopyPart-ShuffleBytes-CopyPart-ShuffleBytes-ChangeBit-Custom-InsertByte-Custom-

After #[3000] : exec/s: 170
#3006 REDUCE cov: 80648 ft: 115428 corp: 297/94Kb lim: 4096 exec/s: 176 rss: 479Mb L: 461/1163 MS: 1 CustomCrossOver-

After #[10000]: exec/s: 111
#10116 REDUCE cov: 80653 ft: 118605 corp: 411/153Kb lim: 4096 exec/s: 111 rss: 768Mb L: 994/1582 MS: 3 CrossOver-Custom-CustomCrossOver-

Update: I ran it for additional several times, each time I deleted the corpus but the exec/s is different. Sometimes when #[4000] it slows down to 100. I remember the first time when I ran it, the exec/s slowed down to 30 exec/s. Is this normal?

It is common for a fuzzer to have a high exec rate at first, then slow down. The reason is that only "shallow" code paths are explored at first. If only a small set of specific inputs cause coverage of a code path that is slow, the fuzzer will not execute it until it runs long enough to find such an input.

The simplest way to improve fuzzer speed is to write less text to logs.

Signed-off-by: jianwen <jianwendong@google.com>

htuch · 2020-06-19T21:06:54Z

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

+  }
+
+} // NOLINT
+  // Silence clang-tidy here because it thinks there is a memory leak for "fill_timer_"


Why are you seeing this and we don't see this in other tests? Can we just destruct at the end?

Why are you seeing this and we don't see this in other tests? Can we just destruct at the end?

Actually we can't. It will crash and tell you that you are trying to free it twice.

This is the comment of this class about whether to delete or not(it also confuses me why it is designed like this): https://github.com/envoyproxy/envoy/blob/master/test/mocks/event/mocks.cc#L41

I think the reason why I'm seeing this is because this is the only place to use this(Event::MockTimer*) outside of unit test(gTest).

It's possible that gTest will clear all the memory inside a test case after the test case is run, and thus the clang-tidy won't give a warning like this.

But libfuzzer doesn't do this. It's running all the time and may not clear the memory after each test case(after n runs it will check memory leak), so clang-tidy gives this warning.

Currently all the usages of Event::MockTimer* are inside gtest(unit test). All the test cases don't destruct it at the end.

If you have any idea on how to deal with it, please let me know, thanks!

Hmm, it looks like the local ratelimit Config object has a unique_ptr to the fill timer. So, it should be destructed, along with ActiveFilter, and free the timer. Do you see the Config object going through its destructor?

Hmm, it looks like the local ratelimit Config object has a unique_ptr to the fill timer. So, it should be destructed, along with ActiveFilter, and free the timer. Do you see the Config object going through its destructor?

In these classes there are no explicit destructors. I tried using a {} block to wrap all the code and observe through a weak_ptr<Config>, the use_count shows 0 after the code block(inside the block the count is 2).
So Config_ should have been destructed after each run. Since fill_timer_ is a unique_ptr inside Config_, it should also have been deleted. I think this explains why when I delete it manually it says I'm trying to free twice.

I think clang-tidy is just having a false positive; there is no leak. Is there a way to narrow the NO_LINT to just this variable?

I think clang-tidy is just having a false positive; there is no leak. Is there a way to narrow the NO_LINT to just this variable?

I tried to use NO_LINT on the variable but it doesn't have any effect. The error given by clang-tidy is in the last line of the block which has potential memory leak issues. NO_LINT could only have effect on one line instead of one variable. After reading the documentation about NO_LINT, I think the only param we can specify is the error type. We could use NOLINT(clang-analyzer-cplusplus.NewDeleteLeaks) instead.

Yeah, the more specific one works, thanks.

htuch

Looks good, just want to figure out what is going on with NOLINT, this shouldn't be needed.
/wait

Signed-off-by: jianwen <jianwendong@google.com>

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc

Signed-off-by: jianwen <jianwendong@google.com>

htuch

LGTM, thanks!

…y#11608) Added fuzz test code for network filter "local_ratelimit" which is defined in extensions/filters/network/local_ratelimit/local_ratelimit.h Added protobuf as the fuzz test input. Added a basic test case to cover the code. Risk Level: low Testing: Increased the coverage from 1.2% to 100% on the filter's .cc and .h files. Signed-off-by: jianwen <jianwendong@google.com>

…y#11608) Added fuzz test code for network filter "local_ratelimit" which is defined in extensions/filters/network/local_ratelimit/local_ratelimit.h Added protobuf as the fuzz test input. Added a basic test case to cover the code. Risk Level: low Testing: Increased the coverage from 1.2% to 100% on the filter's .cc and .h files. Signed-off-by: jianwen <jianwendong@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Auni Ahsan and others added 10 commits June 8, 2020 16:15

integration: Configure drain time in integration tests (envoyproxy#11345

79127df

) Signed-off-by: Auni Ahsan <auni@google.com> Signed-off-by: jianwen <jianwendong@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into thi…

afc91db

…s branch merge up-to-date master branch Signed-off-by: jianwen <jianwendong@google.com> Signed-off-by: jianwen <jianwendong@google.com>

added new empty test file

75d5a2d

Signed-off-by: jianwen <jianwendong@google.com>

finished the code framework for test

dabdf71

Signed-off-by: jianwen <jianwendong@google.com>

modified api at token_bucket.proto, set fill_interval to greater than

33fe08f

50 ms Signed-off-by: jianwen <jianwendong@google.com>

added shadow api change

1cf56aa

Signed-off-by: jianwen <jianwendong@google.com>

reverted changes in api. Added exception catch for constructor of

458e804

activeFilter. Signed-off-by: jianwen <jianwendong@google.com>

Added catch block for the constructors and actions. Fixed the style

791c785

Signed-off-by: jianwen <jianwendong@google.com>

added a test case for covering the code

056b2a4

Signed-off-by: jianwen <jianwendong@google.com>

deleted a comment.

8487024

Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 requested review from junr03 and mattklein123 as code owners June 16, 2020 21:23

jianwen612 added 3 commits June 16, 2020 16:26

removed unnecessary header

aaec3c2

Signed-off-by: jianwen <jianwendong@google.com>

removed unnecessary "using" in .cc.

6f3add0

added protobuf file. Signed-off-by: jianwen <jianwendong@google.com>

added a new line in proto file

d4c8583

Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 marked this pull request as draft June 16, 2020 21:54

fixed an unnecessary message in protobuf

87338b0

Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 marked this pull request as ready for review June 16, 2020 22:03

sqkerner reviewed Jun 16, 2020

View reviewed changes

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc Outdated Show resolved Hide resolved

jianwen612 added 3 commits June 16, 2020 17:27

when unexpected actions come, just abort and give a warning.

b067fda

Signed-off-by: jianwen <jianwendong@google.com>

fixed a typo

4cc6e7a

Signed-off-by: jianwen <jianwendong@google.com>

removed "break" in "default"

4635ea1

Signed-off-by: jianwen <jianwendong@google.com>

sqkerner previously approved these changes Jun 16, 2020

View reviewed changes

htuch suggested changes Jun 16, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 16, 2020

htuch self-assigned this Jun 16, 2020

asraa reviewed Jun 17, 2020

View reviewed changes

test/extensions/filters/network/local_ratelimit/BUILD Outdated Show resolved Hide resolved

asraa reviewed Jun 17, 2020

View reviewed changes

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc Show resolved Hide resolved

asraa suggested changes Jun 17, 2020

View reviewed changes

modified some mocks to static

116b367

added TODO removed unused deps and headers Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 dismissed sqkerner’s stale review via 2f6c005 June 17, 2020 17:08

repokitteh-read-only bot removed the waiting label Jun 17, 2020

jianwen612 requested a review from asraa June 17, 2020 17:27

silence the memory leak warning

298fae6

Signed-off-by: jianwen <jianwendong@google.com>

asraa reviewed Jun 18, 2020

View reviewed changes

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc Show resolved Hide resolved

try-catch only include constructor of config

02fd33d

Signed-off-by: jianwen <jianwendong@google.com>

jianwen612 requested review from asraa and htuch June 18, 2020 21:04

htuch reviewed Jun 19, 2020

View reviewed changes

htuch suggested changes Jun 19, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 19, 2020

added reference to PGV issue

7c80a1b

Signed-off-by: jianwen <jianwendong@google.com>

repokitteh-read-only bot removed the waiting label Jun 19, 2020

jianwen612 requested a review from htuch June 19, 2020 23:31

htuch reviewed Jun 22, 2020

View reviewed changes

test/extensions/filters/network/local_ratelimit/local_ratelimit_fuzz_test.cc Outdated Show resolved Hide resolved

jianwen612 added 3 commits June 22, 2020 11:16

fixed test case

eb8d518

Signed-off-by: jianwen <jianwendong@google.com>

removed local variable name suffix "_"

9ad96fe

Signed-off-by: jianwen <jianwendong@google.com>

use NewDeleteLeaks as param for NOLINT

4dcda31

Signed-off-by: jianwen <jianwendong@google.com>

htuch approved these changes Jun 23, 2020

View reviewed changes

htuch merged commit 658ef43 into envoyproxy:master Jun 23, 2020

jianwen612 mentioned this pull request Jul 14, 2020

[Fuzz] Make network-layer filter fuzz test generic #12084

Closed

Conversation

jianwen612 commented Jun 16, 2020

Uh oh!

Uh oh!

sqkerner left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jianwen612 commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sqkerner commented Jun 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianwen612 Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jianwen612 commented Jun 17, 2020 •

edited

Loading

jianwen612 Jun 22, 2020 •

edited

Loading