Fix TOCTOU race in newEngineReference null checks during engine reset (#21404)#21572
Fix TOCTOU race in newEngineReference null checks during engine reset (#21404)#21572sean- wants to merge 2 commits intoopensearch-project:mainfrom
Conversation
sean-
requested review from
a team,
Bukhtawar,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
PR Reviewer Guide 🔍(Review updated until commit 789103a)Here are some key observations to aid the review process:
|
|
❌ Gradle check result for 974ecae: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
…opensearch-project#21404) Store newEngineReference.get() in a local variable before the null check to prevent a race where another thread nulls the reference between the check and subsequent use, which would cause a NullPointerException. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
sean-
force-pushed
the
worktree-fix-engine-ref-race
branch
from
974ecae to
24b6d00
Compare
|
Persistent review updated to latest commit 24b6d00 |
|
❌ Gradle check result for 24b6d00: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
| @Override | ||
| public GatedCloseable<IndexCommit> acquireLastIndexCommit(boolean flushFirst) { | ||
| if (newEngineReference.get() == null) { | ||
| Indexer indexer = newEngineReference.get(); |
There was a problem hiding this comment.
I'm fine with this change because it indeed does look like a TOCTOU race, but with the semantics of SetOnce it's actually impossible for the reference to change if newEngineReference.get() is not null.
|
❌ Gradle check result for 24b6d00: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Persistent review updated to latest commit 789103a |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21572 +/- ##
============================================
- Coverage 73.48% 73.38% -0.11%
+ Complexity 74646 74541 -105
============================================
Files 5980 5980
Lines 338777 338780 +3
Branches 48848 48848
============================================
- Hits 248964 248610 -354
- Misses 70026 70345 +319
- Partials 19787 19825 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Store newEngineReference.get() in a local variable before the null check to prevent a race where another thread nulls the reference between the check and subsequent use, which would cause a NullPointerException.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.