🔒 [security fix] Gracefully handle poisoned locks and enforce safe unwrap policy#13
🔒 [security fix] Gracefully handle poisoned locks and enforce safe unwrap policy#13tcrypt25519 wants to merge 2 commits intomasterfrom
Conversation
- Replace unsafe `.expect()` and `.unwrap()` on locks with error handling. - Update `MempoolHandle` API to return `Result<..., TrackerError>` for all lock-acquiring methods. - Handle lock poisoning in background tasks by exiting loops safely. - Add `TrackerError::LockPoisoned` variant for structured error handling. - Enforce `clippy::unwrap_used` and `clippy::expect_used` denials outside of tests.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Changed Files
|
There was a problem hiding this comment.
Code Review
This pull request introduces stricter linting rules to deny unwrap and expect in non-test code and refactors the MempoolHandle API to return Result types instead of Option to handle lock poisoning. However, these changes introduce a potential deadlock in estimated_blocks_to_confirm due to nested read locks on self.inner when calling self.gas_ahead. Additionally, the breaking changes to the public API have not been reflected in the integration tests, which will cause compilation failures.
I am having trouble creating individual review comments. Click here to see my feedback.
crates/mempooloracle/src/lib.rs (508)
The refactoring of the MempoolHandle public API to return Result instead of Option is a breaking change that has not been reflected in the integration tests. Specifically, crates/mempooloracle/tests/mempool_oracle.rs still uses the old API (e.g., lines 62-63, 78-79), which will cause the test suite to fail compilation. Please update the tests to handle the new Result return types.
crates/mempooloracle/src/lib.rs (520-536)
This implementation introduces a potential deadlock due to nested read locks on self.inner. estimated_blocks_to_confirm acquires a read lock at line 520 and then calls self.gas_ahead(id) at line 525, which attempts to acquire the same read lock again. On some platforms (like Linux with certain pthread configurations), a reader can block if a writer is waiting, leading to a deadlock if the same thread already holds a read lock.
To fix this, you should avoid calling public methods that acquire locks from within other methods that already hold the lock. Inlining the gas_ahead logic or refactoring it into a private method that takes a reference to the inner state is recommended.
pub fn estimated_blocks_to_confirm(&self, id: &TxId) -> Result<Option<u64>, TrackerError> {
let inner = self.inner.read().map_err(|_| TrackerError::LockPoisoned)?;
let tx = match inner.find_tx_by_id(id) {
Some(tx) => tx,
None => return Ok(None),
};
if inner.classify_transaction(&tx) != TxClassification::Marketable {
return Ok(None);
}
let effective_priority_fee = inner.effective_priority_fee(&tx);
let key_for_tx = (std::cmp::Reverse(effective_priority_fee), tx.sender, tx.nonce);
let mut gas_ahead: u64 = 0;
for (key, _) in inner.priority_queue.range(..key_for_tx) {
let (_, addr, nonce) = key;
if let Some(tx_ahead) = inner.find_tx_by_addr_and_nonce(*addr, *nonce) {
gas_ahead += tx_ahead.gas_limit;
}
}
let usable_capacity =
(inner.last_block_gas_limit as f64 * (1.0 - inner.private_flow_ratio)) as u64;
if usable_capacity == 0 {
return Ok(Some(u64::MAX));
}
Ok(Some(gas_ahead.saturating_add(tx.gas_limit) / usable_capacity))
}
🎯 What: This security fix addresses unsafe lock acquisitions in the
⚠️ Risk: A single thread panic while holding a lock could previously cause other threads to panic upon lock acquisition, leading to a full process crash or Denial of Service (DoS) of the oracle's P2P transport and tracking logic.
mempooloraclecrate. It replaces panics on poisoned locks with structured error handling and graceful shutdowns.🛡️ Solution:
TrackerError::LockPoisonedinstead of panicking.MempoolHandlepublic API was updated to returnResult, allowing callers to handle synchronization issues.#![cfg_attr(not(test), deny(clippy::unwrap_used, clippy::expect_used))]to the crate root to ensure no new unsafe unwraps are introduced in the future.PR created automatically by Jules for task 13036901566451122350 started by @tcrypt25519