Sponsored Content
Skip to content

fix: Add null check - #7305

Merged
bthomee merged 2 commits into
developfrom
pratik/fix-GetAggregatePrice-null-deref
May 21, 2026
Merged

fix: Add null check#7305
bthomee merged 2 commits into
developfrom
pratik/fix-GetAggregatePrice-null-deref

Conversation

@pratikmankawde

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Add a missing null check after txRead() in iteratePriceData() (GetAggregatePrice.cpp), preventing a potential nullptr dereference.

Context of Change

iteratePriceData() walks the chain of previous transactions that modified a PriceOracle ledger object. At line 72, it calls ledger->txRead(prevTx).second to get the transaction metadata, then immediately dereferences it on the next line (meta->getFieldArray(sfAffectedNodes)) without checking for null.

This is inconsistent with every other txRead call site in the codebase, which all check for null before dereferencing. If txRead returns a null metadata pointer, this would be a nullptr dereference / crash.

Triggering conditions: The bug cannot be triggered remotely on a node with consistent data. The sfPreviousTxnID field is maintained by the transaction engine and always points to a real transaction. For txRead to return null, the ledger would need to exist but not contain the referenced transaction — which requires pre-existing local data corruption (SHAMap structural corruption, transaction engine bug, or incomplete ledger reconstruction).

The fix adds a null guard that returns early, matching the existing defensive pattern used throughout the function.

API Impact

No API impact. This is a defensive null check — behavior is unchanged for nodes with consistent data.

Test Plan

Existing GetAggregatePrice_test passes (no behavioral change for valid data paths)
Code inspection confirms the fix matches the defensive pattern used at other txRead call sites in the codebase

pratikmankawde and others added 2 commits May 21, 2026 11:24
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Opus 4.6 · Prompt: V15

@pratikmankawde
pratikmankawde requested review from bthomee and mvadari May 21, 2026 10:33
@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (afcf6fb) to head (7c01370).

Files with missing lines Patch % Lines
...xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp 66.7% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #7305     +/-   ##
=========================================
- Coverage     82.1%   82.1%   -0.0%     
=========================================
  Files         1011    1011             
  Lines        76343   76345      +2     
  Branches      7417    7412      -5     
=========================================
- Hits         62691   62683      -8     
- Misses       13652   13662     +10     
Files with missing lines Coverage Δ
...xrpld/rpc/handlers/orderbook/GetAggregatePrice.cpp 98.8% <66.7%> (-0.6%) ⬇️

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

@bthomee
bthomee requested a review from Copilot May 21, 2026 11:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a defensive null check in iteratePriceData() to avoid dereferencing a null transaction metadata pointer returned by txRead(), improving robustness in the presence of unexpected ledger inconsistencies (e.g., local corruption) without changing behavior on valid data paths.

Changes:

  • Add a nullptr guard after ledger->txRead(prevTx).second before accessing sfAffectedNodes.
  • Simplify the metadata pointer type declaration by using std::shared_ptr<STObject const> directly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pratikmankawde pratikmankawde 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 May 21, 2026
@bthomee
bthomee added this pull request to the merge queue May 21, 2026
Merged via the queue into develop with commit f6fd5dd May 21, 2026
2 of 3 checks passed
@bthomee
bthomee deleted the pratik/fix-GetAggregatePrice-null-deref branch May 21, 2026 14:04
@bthomee bthomee added this to the 3.2.0 milestone May 27, 2026
Kassaking7 pushed a commit to Kassaking7/rippled that referenced this pull request Jun 2, 2026
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

4 participants