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.

Sign up for GitHub

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

Jump to bottom

fuzz: Call SendMessages after ProcessMessage to increase coverage #20674

Merged
merged 1 commit into from
Dec 27, 2020
Merged

fuzz: Call SendMessages after ProcessMessage to increase coverage #20674

merged 1 commit into from
Dec 27, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Dec 16, 2020

No description provided.

@practicalswift
Copy link
Contributor

Concept ACK: more is more when it comes to coverage :)

@practicalswift
Copy link
Contributor

practicalswift commented Dec 16, 2020 •
edited
Loading

Tested ACK fa09f97

Before:

INITED cov: 15027 ft: 49881 corp: 1258/1045Kb exec/s: 181 rss: 683Mb

After:

INITED cov: 16201 ft: 52112 corp: 1260/1038Kb exec/s: 93 rss: 669Mb

Net result: Increase in coverage: Decrease in execs/second.

In fuzzing we can always add more hardware to the fuzzing farm in order to increase execs/second. Better coverage on the other hand is relatively costly to obtain. Thus we want to optimise for coverage rather than execs/second in the general case.

@laanwj laanwj added the Tests label Dec 16, 2020
@dhruv
Copy link
Contributor

dhruv commented Dec 19, 2020 •
edited
Loading

tACK fa09f97

I used some additional seeds and ran fuzz tests (took care to remove any further seeds generated between runs):

FUZZ=process_message src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/process_message
master:    #9987	INITED cov: 23996 ft: 91376 corp: 1999/59Mb exec/s: 59 rss: 863Mb
pr/20674:  #9987	INITED cov: 25491 ft: 94009 corp: 1976/62Mb exec/s: 27 rss: 827Mb

FUZZ=process_messages src/test/fuzz/fuzz qa-assets/fuzz_seed_corpus/process_messages
master:   #14819	INITED cov: 28055 ft: 180329 corp: 3885/119Mb exec/s: 22 rss: 657Mb
pr/20674: #14675	INITED cov: 30072 ft: 183415 corp: 3584/58Mb exec/s: 35 rss: 703Mb

@MarcoFalke Do I understand correctly that:

  • Previously, the test tried fuzz inputs to m_node.ProcessMessage(). Since some of those are valid, as coverage increases, m_node accumulates valid responses to send back to p2p_node
  • This (very clever) PR takes those accumulated in-memory responses and exercises the code to flush them via SendMessages(p2p_node)

If that's all correct, I think there might be more coverage to gain if we can invoke p2p_node.ProcessMessage() using those in-memory responses(since they are probably valid p2p messages). I am not sure this is possible without a significant refactor. If it is useful, I'd be interested in helping extend this with your guidance.

dhruv
dhruv reviewed Dec 19, 2020
View reviewed changes
src/test/fuzz/process_message.cpp
@@ -75,6 +75,10 @@ void fuzz_target(const std::vector<uint8_t>& buffer, const std::string& LIMIT_TO
GetTime<std::chrono::microseconds>(), std::atomic<bool>{false});
} catch (const std::ios_base::failure&) {
}
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that we do not need try { ... } catch (const std::ios_base::failure&) {} here because the inputs to SendMessages() have effectively been filtered for "sane inputs" by ProcessMessage()?

Copy link
Member

Choose a reason for hiding this comment

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

std::ios_base::failure is thrown by the serialization framework when deserialization fails. SendMessages AFAIK doesn't deserialize anything, it just acts on already parsed data. So I wouldn't say it's just been "filtered" - it has been completely processed already, and the result of that processing may trigger responses in SendMessages.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks, @sipa !

@Crypt-iQ
Copy link
Contributor

cr ACK fa09f97

sipa
sipa reviewed Dec 27, 2020
View reviewed changes
Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

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

utACK fa09f97

src/test/fuzz/process_message.cpp
@@ -75,6 +75,10 @@ void fuzz_target(const std::vector<uint8_t>& buffer, const std::string& LIMIT_TO
GetTime<std::chrono::microseconds>(), std::atomic<bool>{false});
} catch (const std::ios_base::failure&) {
}
{
Copy link
Member

Choose a reason for hiding this comment

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

std::ios_base::failure is thrown by the serialization framework when deserialization fails. SendMessages AFAIK doesn't deserialize anything, it just acts on already parsed data. So I wouldn't say it's just been "filtered" - it has been completely processed already, and the result of that processing may trigger responses in SendMessages.

@fanquake fanquake merged commit 31e5116 into bitcoin:master Dec 27, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 27, 2020
@fanquake
Merge bitcoin#20674: fuzz: Call SendMessages after ProcessMessage to …
6c610b9 
…increase coverage

fa09f97 fuzz: Call SendMessages after ProcessMessage to increase coverage (MarcoFalke)

Pull request description:

ACKs for top commit:
  practicalswift:
    Tested ACK fa09f97
  dhruv:
    tACK fa09f97
  Crypt-iQ:
    cr ACK fa09f97
  sipa:
    utACK fa09f97

Tree-SHA512: 87c52aa38f902c4f6c9c2380f486a3ab21edc0e21e48bb619cdb67cfd698154cc57b170eef31fc940c0bb2c878e155847de03fc6e4cd85bed25f10c4f80c747b
@maflcko maflcko deleted the 2012-fuzzSendMessages branch December 29, 2020 08:31
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipa sipa sipa left review comments

@dhruv dhruv dhruv left review comments

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@maflcko @practicalswift @dhruv @Crypt-iQ @sipa @laanwj @fanquake