Repository navigation
Conversation
…reation Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claponcet
added
type: bug fix
This comment has been minimized.
This comment has been minimized.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Handle retirement waits for context creation, and stale callbacks retry with the current snapshot. The completed static review found that context reuse preserves request-close handling and metrics initialization.
🤖 Bits Code Review · Commit 5cfd892 · @DataDog review to ask questions
| final Collection<Address<?>> addressesOfInterest; | ||
| final WafHandle ctx; | ||
|
|
||
| /** Read-held while creating a context from {@link #ctx}; write-held while closing it. */ |
There was a problem hiding this comment.
Really nice investigation, thanks for the detailed write-up! The description mentions that the long-term fix belongs in libddwaf-java (WafContext(WafHandle) taking the handle's read lock). From a quick look at libddwaf-java master it seems that initWafContext still reads nativeHandle without the handle lock, so I think this workaround could be with us for a while. What do you think about opening a ticket/issue for it and referencing it from the handleLock Javadoc (something like "Workaround for libddwaf-java#NNN, can be removed once WafContext takes the handle read lock")? That way it might be easier to remember to clean this up later. Would that make sense to you?
| final WafHandle ctx; | ||
|
|
||
| /** Read-held while creating a context from {@link #ctx}; write-held while closing it. */ | ||
| final StampedLock handleLock = new StampedLock(); |
There was a problem hiding this comment.
Small thought: since StampedLock is not reentrant, it seems that if something inside the read-locked section ever re-entered doRunWaf on the same thread while the RC thread is waiting for the write lock, it could deadlock. As far as I can see this can't happen today, because the section only builds a WafContext under synchronized(reqCtx). Do you think it would be worth adding a short note to the Javadoc saying this section should stay callback-free? Totally optional, just in case it helps future readers.
| WafContext wafContext = reqCtx.getWafContextIfReady(wafMetricsEnabled, gwCtx.isRasp); | ||
| if (wafContext == null) { | ||
| for (; ; ) { | ||
| long stamp = ctxAndAddr.handleLock.readLock(); |
There was a problem hiding this comment.
I might be missing something here, but it seems that the write lock is only taken after the CAS has already retired the snapshot. If so, a failed tryReadLock() would already tell us that ctxAndAddr is stale. Would it make sense to use tryReadLock() and re-read ctxAndAddresses when it returns 0? I think that could keep request threads from parking while the RC thread runs ddwaf_destroy. The window looks quite small though, so happy to leave it as is if you think it's not worth it. What do you think?
| // WAF context closed concurrently between the isWafContextClosed() check and context | ||
| // creation; skip | ||
| // (APPSEC-69085). raspRuleEval() was already counted above, so don't also count | ||
| // raspRuleSkipped() here - that counter is reserved for calls that never attempted eval. |
There was a problem hiding this comment.
Tiny nit: it looks like this comment change might not be related to the fix, and the reflow seems to have left creation; skip on its own line before // (APPSEC-69085). Would you mind either reverting it or re-wrapping it as a single paragraph? Feel free to ignore if I'm misreading it.
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
This branch has not been deployed
2 participants
What Does This Do
Stops native context creation from racing with the remote-config thread destroying the previous WAF handle.
StampedLock, and the remote-config thread holds the write lock while it closes the replacedWafHandle. Handle destruction therefore waits for any in-flightddwaf_context_initon that handle.CtxAndAddressessnapshot before a swap re-reads the current one and retries, so a context is never created from a retired handle.AppSecRequestContext.getWafContextIfReadyinstead of locking. The method declines (and the caller falls back togetOrCreateWafContext) while the context is missing, closed, or the RASP/WAF metrics that call needs have not been created yet. Existing contexts own their native ruleset, so they keep evaluating after their handle is replaced.Motivation
A customer JVM on musl crashed with
SIGSEGV(si_codeSI_KERNEL,si_addr 0x0) inddwaf_destroy, called fromWAFModule.initOrUpdateWafHandleon the remote-config thread.The native
initWafContextin libddwaf-java reads the handle pointer without takingWafHandle's lock or checking whether it is still online, whileWafHandle.close()destroys it. When a request creates its context while remote config replaces the ruleset, it can either:shared_ptrafterddwaf_destroyreleased the last reference, so the ruleset is destroyed a second time when that context closes; orddwaf::wafthat has already been freed.Both corrupt the heap. musl's allocator aborts on that corruption with
hlt, which the kernel reports exactly as the signal in the crash report.The race reproduces on v1.62.0 (libsqreen 17.3.0) and on current master, on x86_64 musl with JDK 25. An
LD_PRELOADtracer on the libddwaf entry points showed every crash was a context creation overlapping a destroy of the same handle. With this change, 17.35 M traced context creations across 225 K handle replacements showed no such overlap, on both libsqreen 17.3.0 and 17.5.0.Additional Notes
synchronized (reqCtx)ingetOrCreateWafContext. The shared lock is only taken on a request's first WAF call, and on its first RASP call when metrics are enabled.StampedLockinstead ofReentrantReadWriteLockavoids per-thread read-hold bookkeeping, and it does not pin virtual-thread carriers.WafContext(WafHandle)should take the handle's own read lock.WAFModuleSpecificationchanges are expectation-only: explicitgetWafContextIfReadycounts, and the context reuse made explicit. The new behaviour is covered by JUnit 5 tests inWAFModuleHandleReloadRaceTestandAppSecRequestContextWafContextRaceTest.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-70090
🤖 Generated with Claude Code