Skip to content

udp listener fuzzer#15974

Merged
asraa merged 9 commits intoenvoyproxy:mainfrom
DavidKorczynski:main
Apr 21, 2021
Merged

udp listener fuzzer#15974
asraa merged 9 commits intoenvoyproxy:mainfrom
DavidKorczynski:main

Conversation

@DavidKorczynski
Copy link
Copy Markdown
Contributor

@DavidKorczynski DavidKorczynski commented Apr 14, 2021

Signed-off-by: davkor david@adalogics.com

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:]

Signed-off-by: davkor <david@adalogics.com>
Signed-off-by: davkor <david@adalogics.com>
Signed-off-by: davkor <david@adalogics.com>
@repokitteh-read-only
Copy link
Copy Markdown

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.

Signed-off-by: davkor <david@adalogics.com>
Signed-off-by: davkor <david@adalogics.com>
Signed-off-by: davkor <david@adalogics.com>
Copy link
Copy Markdown
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

Signed-off-by: davkor <david@adalogics.com>
Signed-off-by: davkor <david@adalogics.com>
Copy link
Copy Markdown
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?

Signed-off-by: davkor <david@adalogics.com>
@DavidKorczynski
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

asraa commented Apr 21, 2021

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

@DavidKorczynski
Copy link
Copy Markdown
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 <david@adalogics.com>
Signed-off-by: Gokul Nair <gnair@twitter.com>
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