Skip to content

[Do not Merge] Exploratory fix for PMMR header crash #3807

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yeastplume
Copy link
Member

(This is here for testing and diagnosis only, for now.)

This is tryiong track down a capacity overflow panic reported in the keybase dev channel that can happen when a node receives PMMR segment data from a peer during PIBD sync. The code was blindly trusting the segment size announced by the peer and trying to allocate huge amounts of memory if that size was bogus (either intentionally malicious or just corrupted).

To fix / diagnose this, we're trying to:

  • introduce checks in Segment::read within core/src/core/pmmr/segment.rs.
  • set a max limit based on the largest segment height currently configured in chain/src/pibd_params.rs
  • Improve error handling and log message when the issue occurs

This should help pinpoint where the error is coming from (whether it's an intentionally malicious peer or an error in the client code somewhere)

@yeastplume
Copy link
Member Author

Interestingly, that change triffered a test failure in the ser_round_trip test indicating that a nonsensical value was being read. After some tracking, it appears there was a mismatch between how a segment was being written and read, which may explain the original issue. Running a few tests to confirm we can still sync after the fix.

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.

1 participant