Skip to content

fuzz: Check for addrv1 compatibility before using addrv1 serializer. Fuzz addrv2 serialization. #20247

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
Oct 27, 2020
Merged

fuzz: Check for addrv1 compatibility before using addrv1 serializer. Fuzz addrv2 serialization. #20247

maflcko merged 1 commit into from
Oct 27, 2020

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 26, 2020

Fuzz addrv2 address serialization.

Check for addrv1 compatibility before using addrv1 serializer.

Before this

$ src/test/fuzz/netaddr_deserialize
netaddr_deserialize: test/fuzz/deserialize.cpp:84: void 
    (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &) [T = CNetAddr]:
    Assertion `Deserialize<T>(Serialize(obj)) == obj' failed.

After this patch:

$ src/test/fuzz/netaddr_deserialize
…

@practicalswift practicalswift changed the title fuzz: Check for addrv1 compatibility before using addrv1 serializer. Fuzz addrv2 address serialization. fuzz: Check for addrv1 compatibility before using addrv1 serializer. Fuzz addrv2 serialization. Oct 26, 2020
@practicalswift practicalswift force-pushed the fuzzers-netaddr-post-addrv2 branch from d0f2d4c to 903f3d0 Compare October 26, 2020 16:37
@maflcko
Copy link
Member

maflcko commented Oct 26, 2020

review ACK 903f3d0

@DrahtBot DrahtBot added the Tests label Oct 26, 2020
sipa
sipa reviewed Oct 26, 2020
View reviewed changes
@maflcko maflcko merged commit fa463f1 into bitcoin:master Oct 27, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 27, 2020
Merge bitcoin#20247: fuzz: Check for addrv1 compatibility before usin…
e6bcd86 
…g addrv1 serializer. Fuzz addrv2 serialization.

903f3d0 fuzz: Check for addrv1 compatibility before using addrv1 serializer (practicalswift)

Pull request description:

  Fuzz addrv2 address serialization.

  Check for addrv1 compatibility before using addrv1 serializer.

  Before this

  ```
  $ src/test/fuzz/netaddr_deserialize
  netaddr_deserialize: test/fuzz/deserialize.cpp:84: void
      (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &) [T = CNetAddr]:
      Assertion `Deserialize<T>(Serialize(obj)) == obj' failed.
  ```

  After this patch:

  ```
  $ src/test/fuzz/netaddr_deserialize
  …
  ```

ACKs for top commit:
  MarcoFalke:
    review ACK 903f3d0

Tree-SHA512: a9ddb71cc31c877fa3dd78dbc908d1e30b4790398fefe19e6541f1fca81e8560f7a11fa099ef3943b94401974c472e523484fdf66f1c23ff2e998558ba4b65de
@practicalswift
Copy link
Contributor Author

practicalswift commented Nov 1, 2020

@vasild The results you're describing below were from fuzzing a version of master prior to the merge of this PR, right? :)

hmm, it just occured to me (after staring at some fuzzing failure) that some CNetAddr objects do not ser+deser into the original. For example a torv3 address when serialized (as done by the fuzzer, ADDRV2_FORMAT is not set) end up with 16 zeroes.

@vasild
Copy link
Contributor

vasild commented Nov 1, 2020

Yes, prior to this PR. Thanks for fixing this!

ACK 903f3d0

@practicalswift practicalswift mentioned this pull request Nov 2, 2020
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 3, 2020
Merge bitcoin#20289: fuzz: Check for addrv1 compatibility before usin…
5174b53 
…g addrv1 serializer/deserializer on CService

c2cf8a1 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService (practicalswift)

Pull request description:

  Check for addrv1 compatibility before using addrv1 serializer/deserializer on `CService`:

  Before this patch:

  ```
  $ src/test/fuzz/service_deserialize
  service_deserialize: test/fuzz/deserialize.cpp:85:
      void (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &, const int) [T = CService]:
      Assertion `Deserialize<T>(Serialize(obj, version)) == obj' failed.
  ```

  After this patch:

  ```
  $ src/test/fuzz/service_deserialize
  …
  ```

  Related change: bitcoin#20247

ACKs for top commit:
  MarcoFalke:
    review ACK c2cf8a1

Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 3, 2020
Merge bitcoin#20289: fuzz: Check for addrv1 compatibility before usin…
ac6844b 
…g addrv1 serializer/deserializer on CService

c2cf8a1 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService (practicalswift)

Pull request description:

  Check for addrv1 compatibility before using addrv1 serializer/deserializer on `CService`:

  Before this patch:

  ```
  $ src/test/fuzz/service_deserialize
  service_deserialize: test/fuzz/deserialize.cpp:85:
      void (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &, const int) [T = CService]:
      Assertion `Deserialize<T>(Serialize(obj, version)) == obj' failed.
  ```

  After this patch:

  ```
  $ src/test/fuzz/service_deserialize
  …
  ```

  Related change: bitcoin#20247

ACKs for top commit:
  MarcoFalke:
    review ACK c2cf8a1

Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
@practicalswift practicalswift deleted the fuzzers-netaddr-post-addrv2 branch April 10, 2021 19:42
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2021
@PastaPastaPasta
Merge bitcoin#20289: fuzz: Check for addrv1 compatibility before usin…
f5be380 
…g addrv1 serializer/deserializer on CService

c2cf8a1 fuzz: Check for addrv1 compatibility before using addrv1 serializer on CService (practicalswift)

Pull request description:

  Check for addrv1 compatibility before using addrv1 serializer/deserializer on `CService`:

  Before this patch:

  ```
  $ src/test/fuzz/service_deserialize
  service_deserialize: test/fuzz/deserialize.cpp:85:
      void (anonymous namespace)::AssertEqualAfterSerializeDeserialize(const T &, const int) [T = CService]:
      Assertion `Deserialize<T>(Serialize(obj, version)) == obj' failed.
  ```

  After this patch:

  ```
  $ src/test/fuzz/service_deserialize
  …
  ```

  Related change: bitcoin#20247

ACKs for top commit:
  MarcoFalke:
    review ACK c2cf8a1

Tree-SHA512: dba6ddc60e8ef621011d844281461f1741de08c4af1a2b7156c810af44306cef7ec582de5974752db02ca085cfd23da0296d70b694e59ee262589d829fa0626e
kwvg added a commit to kwvg/dash that referenced this pull request Mar 17, 2022
@kwvg
merge bitcoin#20247: Check for addrv1 compatibility before using addr…
c982fc5 
…v1 serializer. Fuzz addrv2 serialization.
@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

@sipa sipa sipa 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.

5 participants

@practicalswift @maflcko @vasild @sipa @DrahtBot