8000 Remove null pointer deref from `LogicError`, just do `std::abort` by Bronek · Pull Request #5338 · XRPLF/rippled · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
/ rippled Public

Remove null pointer deref from LogicError, just do std::abort #5338

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 5 commits into from
Mar 18, 2025
Merged

Remove null pointer deref from LogicError, just do std::abort #5338

merged 5 commits into from
Mar 18, 2025

Conversation

Bronek
Copy link
Collaborator
@Bronek Bronek commented Mar 6, 2025 •
edited
Loading

High Level Overview of Change

Remove UB from LogicError so we can be certain that there will be always a stacktrace.

Context of Change

De-referencing a null pointer is an old trick to generate SIGSEGV which would typically also create a stacktrace. However it is also an UB (undefined behaviour) and compilers can do something else. A more robust way to create a stacktrace while crashing the program is to use std::abort, which we have also used in this location for a long time. If we combine the two, we might not get the expected behaviour - the UB (which is nullpointer deref) followed by std::abort, as handled in certain compiler versions may not immediately cause a crash. We have observed stacktrace being wiped instead, and thread put in indeterminate state, then stacktrace created without any useful information. So despite making "two attempts at stacktrace", we get no useful stacktrace, just a crash.

Note, we must not use std::unreachable here, since it also invokes UB.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

@codecov Codecov
Copy link
codecov bot commented Mar 7, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.1%. Comparing base (d167d48) to head (999a36a).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5338     +/-   ##
=========================================
- Coverage     78.1%   78.1%   -0.0%     
=========================================
  Files          790     790             
  Lines        67916   67912      -4     
  Branches      8233    8236      +3     
=========================================
- Hits         53031   53024      -7     
- Misses       14885   14888      +3
Files with missing lines Coverage Δ
src/libxrpl/basics/contract.cpp 42.9% <ø> (+15.6%) ⬆️

... and 4 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.

@Bronek Bronek requested a review from vlntb March 7, 2025 15:27
@bthomee bthomee 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 Mar 18, 2025
@bthomee bthomee merged commit 345ddc7 into develop Mar 18, 2025
24 checks passed
@bthomee bthomee deleted the Bronek/abort_in_LogicError branch March 18, 2025 16:45
mvadari added a commit that referenced this pull request Mar 20, 2025
@mvadari @legleux
@bthomee @ximinez @Bronek @Tokeiito @kuznetsss @cindyyan317 @vvysokikh1 @godexsoft
Merge develop into ripple/smart-escrow (#5357)
911c046 
* Set version to 2.4.0

* refactor: Remove unused and add missing includes (#5293)

The codebase is filled with includes that are unused, and which thus can be removed. At the same time, the files often do not include all headers that contain the definitions used in those files. This change uses clang-format and clang-tidy to clean up the includes, with minor manual intervention to ensure the code compiles on all platforms.

* refactor: Calculate numFeatures automatically (#5324)

Requiring manual updates of numFeatures is an annoying manual process that is easily forgotten, and leads to frequent merge conflicts. This change takes advantage of the `XRPL_FEATURE` and `XRPL_FIX` macros, and adds a new `XRPL_RETIRE` macro to automatically set `numFeatures`.

* refactor: Improve ordering of headers with clang-format (#5343)

Removes all manual header groupings from source and header files by leveraging clang-format options.

* Rename "deadlock" to "stall" in `LoadManager` (#5341)

What the LoadManager class does is stall detection, which is not the same as deadlock detection. In the condition of severe CPU starvation, LoadManager will currently intentionally crash rippled reporting `LogicError: Deadlock detected`. This error message is misleading as the condition being detected is not a deadlock. This change fixes and refactors the code in response.

* Adds hub.xrpl-commons.org as a new Bootstrap Cluster (#5263)

* fix: Error message for ledger_entry rpc (#5344)

Changes the error to `malformedAddress` for `permissioned_domain` in the `ledger_entry` rpc, when the account is not a string. This change makes it more clear to a user what is wrong with their request.

* fix: Handle invalid marker parameter in grpc call (#5317)

The `end_marker` is used to limit the range of ledger entries to fetch. If `end_marker` is less than `marker`, a crash can occur. This change adds an additional check.

* fix: trust line RPC no ripple flag (#5345)

The Trustline RPC `no_ripple` flag gets set depending on `lsfDefaultRipple` flag, which is not a flag of a trustline but of the account root. The `lsfDefaultRipple` flag does not provide any insight if this particular trust line has `lsfLowNoRipple` or `lsfHighNoRipple` flag set, so it should not be used here at all. This change simplifies the logic.

* refactor: Updates Conan dependencies: RocksDB (#5335)

Updates RocksDB to version 9.7.3, the latest version supported in Conan 1.x. A patch for 9.7.4 that fixes a memory leak is included.

* fix: Remove null pointer deref, just do abort (#5338)

This change removes the existing undefined behavior from `LogicError`, so we can be certain that there will be always a stacktrace.

De-referencing a null pointer is an old trick to generate `SIGSEGV`, which would typically also create a stacktrace. However it is also an undefined behaviour and compilers can do something else. A more robust way to create a stacktrace while crashing the program is to use `std::abort`, which we have also used in this location for a long time. If we combine the two, we might not get the expected behaviour - namely, the nullpointer deref followed by `std::abort`, as handled in certain compiler versions may not immediately cause a crash. We have observed stacktrace being wiped instead, and thread put in indeterminate state, then stacktrace created without any useful information.

* chore: Add PR number to payload (#5310)

This PR adds one more payload field to the libXRPL compatibility check workflow - the PR number itself.

* chore: Update link to ripple-binary-codec (#5355)

The link to ripple-binary-codec's definitions.json appears to be outdated. The updated link is also documented here: https://xrpl.org/docs/references/protocol/binary-format#definitions-file

* Prevent consensus from getting stuck in the establish phase (#5277)

- Detects if the consensus process is "stalled". If it is, then we can declare a 
  consensus and end successfully even if we do not have 80% agreement on
  our proposal.
  - "Stalled" is defined as:
    - We have a close time consensus
    - Each disputed transaction is individually stalled:
      - It has been in the final "stuck" 95% requirement for at least 2
        (avMIN_ROUNDS) "inner rounds" of phaseEstablish,
      - and either all of the other trusted proposers or this validator, if proposing,
        have had the same vote(s) for at least 4 (avSTALLED_ROUNDS) "inner
        rounds", and at least 80% of the validators (including this one, if
        appropriate) agree about the vote (whether yes or no).
- If we have been in the establish phase for more than 10x the previous
  consensus establish phase's time, then consensus is considered "expired",
  and we will leave the round, which sends a partial validation (indicating
  that the node is moving on without validating). Two restrictions avoid
  prematurely exiting, or having an extended exit in extreme situations.
  - The 10x time is clamped to be within a range of 15s
    (ledgerMAX_CONSENSUS) to 120s (ledgerABANDON_CONSENSUS).
  - If consensus has not had an opportunity to walk through all avalanche
    states (defined as not going through 8 "inner rounds" of phaseEstablish),
    then ConsensusState::Expired is treated as ConsensusState::No.
- When enough nodes leave the round, any remaining nodes will see they've
  fallen behind, and move on, too, generally before hitting the timeout. Any
  validations or partial validations sent during this time will help the
  consensus process bring the nodes back together.

---------

Co-authored-by: Michael Legleux <mlegleux@ripple.com>
Co-authored-by: Bart <bthomee@users.noreply.github.com>
Co-authored-by: Ed Hennis <ed@ripple.com>
Co-authored-by: Bronek Kozicki <brok@incorrekt.com>
Co-authored-by: Darius Tumas <Tokeiito@users.noreply.github.com>
Co-authored-by: Sergey Kuznetsov <skuznetsov@ripple.com>
Co-authored-by: cyan317 <120398799+cindyyan317@users.noreply.github.com>
Co-authored-by: Vlad <129996061+vvysokikh1@users.noreply.github.com>
Co-authored-by: Alex Kremer <akremer@ripple.com>
Tapanito pushed a commit to Tapanito/rippled that referenced this pull request Mar 24, 2025
@Bronek @Tapanito
fix: Remove null pointer deref, just do abort (XRPLF#5338)
01027bc 
This change removes the existing undefined behavior from `LogicError`, so we can be certain that there will be always a stacktrace.

De-referencing a null pointer is an old trick to generate `SIGSEGV`, which would typically also create a stacktrace. However it is also an undefined behaviour and compilers can do something else. A more robust way to create a stacktrace while crashing the program is to use `std::abort`, which we have also used in this location for a long time. If we combine the two, we might not get the expected behaviour - namely, the nullpointer deref followed by `std::abort`, as handled in certain compiler versions may not immediately cause a crash. We have observed stacktrace being wiped instead, and thread put in indeterminate state, then stacktrace created without any useful information.
Tapanito pushed a commit to Tapanito/rippled that referenced this pull request Mar 31, 2025
@Bronek @Tapanito
fix: Remove null pointer deref, just do abort (XRPLF#5338)
c8b583c 
This change removes the existing undefined behavior from `LogicError`, so we can be certain that there will be always a stacktrace.

De-referencing a null pointer is an old trick to generate `SIGSEGV`, which would typically also create a stacktrace. However it is also an undefined behaviour and compilers can do something else. A more robust way to create a stacktrace while crashing the program is to use `std::abort`, which we have also used in this location for a long time. If we combine the two, we might not get the expected behaviour - namely, the nullpointer deref followed by `std::abort`, as handled in certain compiler versions may not immediately cause a crash. We have observed stacktrace being wiped instead, and thread put in indeterminate state, then stacktrace created without any useful information.
Sign up for free to join this 66F3 conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthomee bthomee bthomee approved these changes

@vlntb vlntb vlntb approved these changes

Assignees
No one assigned
Labels
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.
3 participants
@Bronek @bthomee @vlntb
0