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.

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

udp listener fuzzer #15974

Merged
merged 9 commits into from
Apr 21, 2021
Merged

udp listener fuzzer #15974

merged 9 commits into from
Apr 21, 2021

Conversation

DavidKorczynski
Copy link
Contributor

@DavidKorczynski DavidKorczynski commented Apr 14, 2021

Signed-off-by: davkor [email protected]

Commit Message: This adds a fuzz test that targets the UdpListener code.
The fuzzer has been tested over a 30 minute experiment and runs without issues. It currently hits code in /source/common/network/udp_listener_impl.cc, /source/common/network/udp_listener_impl.cc, and source/common/network/io_socket_handle_impl.cc
Additional Description: Cross referencing #14889 CC @asraa
Risk Level: Zero. This adds a fuzz test and does not modify anything in the Envoy proxy itself.
Testing: Compiled with OSS-Fuzz to verify fuzzer runs in OSS-Fuzz environment.
Docs Changes: N/A
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

Hi @DavidKorczynski, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15974 was opened by DavidKorczynski.

see: more, trace.

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks!! You may need to run fix format to help CI
out https://github.com/envoyproxy/envoy/blob/1d1b708c7bf6efa02c41d9ce22cbf1e4a1aeec2c/support/README.md#fixing-format-problems

Couple of initial nits, after CI passes it should generate a fuzz coverage report

test/common/network/udp_fuzz.cc Outdated Show resolved Hide resolved
test/common/network/udp_fuzz.cc Outdated Show resolved Hide resolved
test/common/network/udp_fuzz.cc Outdated Show resolved Hide resolved
test/common/network/udp_fuzz.cc Outdated Show resolved Hide resolved
test/common/network/udp_fuzz.cc Show resolved Hide resolved
test/common/network/udp_fuzz.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! The fuzz coverage reports were generated, this got added the wanted coverage here https://storage.googleapis.com/envoy-pr/bebe107/fuzz_coverage/source/common/network/utility.cc.gcov.html
(compare to https://storage.googleapis.com/envoy-postsubmit/master/fuzz_coverage/source/common/network/utility.cc.gcov.html)

Assuming oss-fuzz runners support UDP_GRO, maybe we can also fuzz that portion?

@DavidKorczynski
Copy link
Contributor Author

DavidKorczynski commented Apr 19, 2021

Thanks! The fuzz coverage reports were generated, this got added the wanted coverage here https://storage.googleapis.com/envoy-pr/bebe107/fuzz_coverage/source/common/network/utility.cc.gcov.html
(compare to https://storage.googleapis.com/envoy-postsubmit/master/fuzz_coverage/source/common/network/utility.cc.gcov.html)

Assuming oss-fuzz runners support UDP_GRO, maybe we can also fuzz that portion?

Sounds good - fixed it up and you can see the coverage report here: https://storage.googleapis.com/envoy-pr/056fbe5/fuzz_coverage/source/common/network/utility.cc.gcov.html

Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

LGTM!

FWIW (for documentation later, not blocking addition to OSS-Fuzz) could you check if this line crashes

ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len,
output.msg_[i].peer_address_->asString());
if you revert the conditions about output.msg_[i].truncated_and_dropped_ from https://github.com/envoyproxy/envoy/pull/14122/files

@asraa asraa merged commit 0c37028 into envoyproxy:main Apr 21, 2021
@DavidKorczynski
Copy link
Contributor Author

LGTM!

FWIW (for documentation later, not blocking addition to OSS-Fuzz) could you check if this line crashes

ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len,
output.msg_[i].peer_address_->asString());

if you revert the conditions about output.msg_[i].truncated_and_dropped_ from https://github.com/envoyproxy/envoy/pull/14122/files

Sure thing. Just to clarify here, is it the logging line or the assert right before the logging line that should crash?

@asraa
Copy link
Contributor

asraa commented Apr 21, 2021

It's the logging line actually :P since peer_address_ will be nullptr

@DavidKorczynski
Copy link
Contributor Author

It's the logging line actually :P since peer_address_ will be nullptr

Ahh :) Thanks for the clarification, will update here with documentation shortly.

gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
* test: common: network: add udp listener fuzzer.

Signed-off-by: davkor <[email protected]>
Signed-off-by: Gokul Nair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants