Skip to content

fuzz: Extend addrman fuzz test with deserialize #22493

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
Merged
maflcko merged 1 commit into from
Jul 22, 2021
Merged

fuzz: Extend addrman fuzz test with deserialize #22493

maflcko merged 1 commit into from
Jul 22, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 19, 2021

Requested on IRC:

[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely

@fanquake fanquake added the Tests label Jul 19, 2021
This was referenced Jul 19, 2021
Closed
Merged
Closed
vasild
vasild reviewed Jul 19, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK

The unserialize itself is fuzzed in:

bitcoin/src/test/fuzz/deserialize.cpp

Lines 201 to 204 in d3474b8

FUZZ_TARGET_DESERIALIZE(addrman_deserialize, {
CAddrMan am;
DeserializeFromFuzzingInput(buffer, am);
})

What this test adds is calling addrman methods after it is unserialized from fuzzed data. This kind of simulates a disk corruption. I think it may result in the same addr:port being in "new" and "tried" or other "interesting" situations that do not occur from normal operation.

@maflcko maflcko force-pushed the 2107-fuzzAddrDeser branch from fa0a11e to fa74025 Compare July 19, 2021 12:15
vasild
vasild approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK fa740252d2dcc272204b745906785fe26e9a0b77

@maflcko maflcko requested a review from practicalswift July 19, 2021 12:31
jonatack
jonatack reviewed Jul 19, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Tested ACK fa740252d2dcc272204b745906785fe26e9a0b77

maflcko
maflcko commented Jul 19, 2021
View reviewed changes
@maflcko maflcko force-pushed the 2107-fuzzAddrDeser branch from fa74025 to aaaa9c6 Compare July 19, 2021 16:27
@jonatack
Copy link
Member

jonatack commented Jul 19, 2021

ACK aaaa9c6 per git diff fa74025 aaaa9c6

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 19, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Jul 20, 2021
Merged
This was referenced Jul 20, 2021
Closed
Closed
@maflcko
Copy link
Member Author

maflcko commented Jul 20, 2021

@vasild I am seeing there is a conflict. Do you have a preference which one should be merged first?

@DrahtBot DrahtBot mentioned this pull request Jul 20, 2021
Merged
@maflcko maflcko mentioned this pull request Jul 21, 2021
Closed
vasild
vasild approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK aaaa9c6

@vasild I am seeing there is a conflict. Do you have a preference which one should be merged first?

No.

@maflcko
Copy link
Member Author

maflcko commented Jul 22, 2021

Ok, merging the one with the nicer commit-hash first 😅

@maflcko maflcko merged commit bfa52cb into bitcoin:master Jul 22, 2021
@maflcko maflcko deleted the 2107-fuzzAddrDeser branch July 22, 2021 15:04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021
Merge bitcoin#22493: fuzz: Extend addrman fuzz test with deserialize
445a48b 
aaaa9c6 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)

Pull request description:

  Requested on IRC:

  ```
  [18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
  [18:04] <sipa> definitely

ACKs for top commit:
  jonatack:
    ACK aaaa9c6 per `git diff fa74025 aaaa9c6`
  vasild:
    ACK aaaa9c6

Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
@mzumsande mzumsande mentioned this pull request Aug 13, 2021
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@practicalswift practicalswift Awaiting requested review from practicalswift

2 more reviewers

@jonatack jonatack jonatack left review comments

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @jonatack @DrahtBot @vasild @fanquake