Skip to content

Fix crash in Slot::deletePeer #5635

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
merged 1 commit into from
Jul 31, 2025
Merged

Fix crash in Slot::deletePeer #5635

merged 1 commit into from
Jul 31, 2025
+19 −5

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Jul 31, 2025 •
edited
Loading

High Level Overview of Change

Fix crash due to recurrent call to Slot::deletePeer (via OverlayImpl::unsquelch) when a peer is disconnected at just the wrong moment.

Context of Change

Similar to #5071

This fixes race condition and UB, I do not think it is possible to write unit tests for this fix. Not with the overall poor coverage we have for PeerImp, but unlikely even if we had much better coverage.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

@Bronek Bronek requested review from gregtatcam and vlntb July 31, 2025 12:58
@Bronek Bronek added the Bug label Jul 31, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 31, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.8%. Comparing base (baf4b83) to head (d672505).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/OverlayImpl.cpp 0.0% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5635   +/-   ##
=======================================
  Coverage     78.8%   78.8%           
=======================================
  Files          814     814           
  Lines        71261   71270    +9     
  Branches      8368    8349   -19     
=======================================
+ Hits         56131   56159   +28     
+ Misses       15130   15111   -19
Files with missing lines Coverage Δ
src/xrpld/overlay/Slot.h 85.0% <100.0%> (+0.2%) ⬆️
src/xrpld/overlay/detail/OverlayImpl.cpp 34.6% <0.0%> (-0.2%) ⬇️

... and 8 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gregtatcam
gregtatcam approved these changes Jul 31, 2025
View reviewed changes
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

LGTM

@Bronek Bronek added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jul 31, 2025
@Bronek
Copy link
Collaborator Author

Bronek commented Jul 31, 2025 •
edited
Loading

merge comment - please ignore codecov failure - as explained there's no unit tests for this case, for a good reason

@bthomee bthomee merged commit d835e97 into develop Jul 31, 2025
26 of 27 checks passed
@bthomee bthomee deleted the Bronek/slot_bugfix branch July 31, 2025 17:08
bthomee pushed a commit that referenced this pull request Jul 31, 2025
@Bronek @bthomee
Fix crash in Slot::deletePeer (#5635)
3c020b8 
Fix crash due to recurrent call to `Slot::deletePeer` (via `OverlayImpl::unsquelch`) when a peer is disconnected at just the wrong moment.
mvadari pushed a commit to mvadari/rippled that referenced this pull request Aug 5, 2025
@Bronek @mvadari
Fix crash in Slot::deletePeer (XRPLF#5635)
1f82192 
Fix crash due to recurrent call to `Slot::deletePeer` (via `OverlayImpl::unsquelch`) when a peer is disconnected at just the wrong moment.
This was referenced Aug 27, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregtatcam gregtatcam gregtatcam approved these changes

@vlntb vlntb vlntb approved these changes

Assignees

No one assigned

Labels

Bug Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bronek @gregtatcam @vlntb @bthomee