Read agent jar resources through the retained jar handle - #12426
Read agent jar resources through the retained jar handle#12426claponcet wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
claponcet
added
type: bug fix
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
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. |
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-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The retained jar handle supplies indexed resources. The old lookup path still supplies resources that the retained jar does not contain.
🤖 Datadog Autotest · Commit c57654d · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| InputStream is = findResourceAsStream(name); | ||
| if (null == is) { | ||
| is = super.getResourceAsStream(name); | ||
| } |
There was a problem hiding this comment.
Here it seems we are flipping the resolution order. Is that a safe change?
There was a problem hiding this comment.
Yes, this is intentional for indexed agent resources so their bytes always match the classes loaded from the retained jar, rather than being shadowed by a parent or replacement jar. Unindexed resources still delegate normally; b6d3bec336 adds tests for both the parent-collision case and manifest fallback.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
Automated review findings (see inline comments).
| return super.getResource(name); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This fix only patches getResourceAsStream(). getResource()/findResource() still build a pathname-based jar: URL that has the same live-jar-replacement vulnerability. A caller doing classLoader.getResource(name).openStream() after the agent jar is replaced/deleted on disk would still hit the original bug (stale/foreign content or an IOException) since only the getResourceAsStream path was patched. Worth confirming this is intentionally out of scope, or fixing both paths.
There was a problem hiding this comment.
Good point—this remains intentionally out of scope. The reported AppSec resource and other bundled AppSec resources use getResourceAsStream(), which now reads indexed entries from the retained jar.
Fixing getResource() requires a retained-jar URL implementation and changes to BootstrapProxy-first resolution, with broader compatibility implications.
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
findResourceAsStream duplicates the entryName/getJarEntry lookup already in findResource() just above/below it in this file. Consider extracting a shared private JarEntry resourceJarEntry(String name) used by both, so a future change to jar-entry resolution (e.g. AgentJarIndex versioning or case-insensitive lookup) doesn't need to be applied in two places and risk silent divergence.
There was a problem hiding this comment.
Good suggestion—done in b6d3bec336. Both getResourceAsStream() and findResource() now use the shared findResourceEntry() helper for index resolution and jar-entry lookup.
| * Reads a resource we ship from the agent jar, or returns {@code null} if the agent jar does not | ||
| * contain it. Note {@link BootstrapProxy} is backed by the very same jar, so consulting this | ||
| * before delegating cannot shadow a different resource of the same name. | ||
| */ |
There was a problem hiding this comment.
This javadoc states BootstrapProxy is "backed by the very same jar", but that's only true by convention today (both call sites happen to pass the same URL) — BootstrapProxy is explicitly designed to hold multiple jars. If a future change adds a second BootstrapProxy.addBootstrapResource(differentJarUrl) call, findResourceAsStream's own-jar-first ordering could silently shadow a resource BootstrapProxy would otherwise have served, contradicting this documented guarantee. Consider noting this is an invariant maintained by callers, not enforced here.
There was a problem hiding this comment.
Agreed—updated in b6d3bec336. The Javadoc now states that production initially registers the same agent jar URL, while BootstrapProxy may contain additional URLs, and documents the intentional precedence for indexed agent resources.
| // deleting a file that is still open is rejected on Windows, so the scenario cannot arise there | ||
| assumeFalse(System.getProperty("os.name").toLowerCase(Locale.ROOT).contains("win")); | ||
|
|
||
| File jar = new File(tempDir, "testjar-jdk8"); |
There was a problem hiding this comment.
The two new regression tests duplicate identical jar-copy-and-construct-loader setup boilerplate. Consider extracting a shared private DatadogClassLoader loadCopyOfTestJar(File tempDir) helper so a future change to the test fixture path or the constructor signature only needs updating in one place.
There was a problem hiding this comment.
I considered this, but kept the small setup explicit because the jar pathname and the point where the loader retains its handle are central to each test’s lifecycle. The tests diverge immediately afterward, while the less scenario-specific jar-writing and byte-reading operations are shared through helpers.
| InputStream is = findResourceAsStream(name); | ||
| if (null == is) { | ||
| is = super.getResourceAsStream(name); | ||
| } | ||
| return is; | ||
| } |
There was a problem hiding this comment.
findResourceAsStream returns null both when the agent jar does not contain the
resource and when JarFile.getInputStream throws an IOException (line 100-103).
In the second case, getResourceAsStream (line 79-81) falls through to
super.getResourceAsStream, which resolves the resource through getResource() and
can reopen the jar by pathname. If that pathname now holds a replacement build, this
would serve the replacement's copy of the resource - the exact cross-build mixing this
change is meant to prevent.
This requires a narrow double condition (entry present in the retained handle's index,
but its read independently throwing IOException), so I'd call it non-blocking rather
than a blocker, but worth a comment or a follow-up: e.g. treating a caught IOException
here as "resource not found, and not exposed to the fallback either" (return a marker
distinct from null, or rethrow) would close the gap.
There was a problem hiding this comment.
Good catch—fixed in b6d3bec336. We now resolve the JarEntry first: a missing entry delegates normally, while an IOException opening an owned entry is logged and returns null without falling back to a potentially different jar.
|
Just a note on the framing in the PR description: APPSEC-69906's own root-cause analysis is flagged as "weak-moderate confidence" and points at classloading isolation/timing during early Tomcat/Catalina bootstrap, not necessarily a live jar replacement on disk - the ticket itself says reproduction against a live Tomcat |
4 participants
What Does This Do
DatadogClassLoadernow reads indexed agent resources from theJarFilehandle it opened at construction time — the same source class loading already reads from — instead of resolving the agent jar by pathname on every read. Resources not owned by the retained jar, including the manifest which is deliberately excluded from the index, continue through the previous delegation path unchanged.If the retained jar owns an entry but opening it fails, the lookup is not delegated: doing so could serve the same entry from a different agent jar now present at the original pathname.
Motivation
A JVM can outlive the
dd-java-agent.jarpathname it started with. For example, package upgrades and configuration management commonly replace the jar on disk while the JVM keeps running. Class loading continues through the retained jar handle, but resource loading went through thejar:file:...!/entryURL returned byfindResource(), which re-resolves the pathname on every read.Once the jar at that pathname has been replaced or removed, that read can either:
ClassLoader.getResourceAsStream()turns theIOExceptioninto a silentnull, so an unreadable resource is indistinguishable from an absent one; orappsec/native_libs/.../libddwaf.so, where a native library could be loaded against JNI bindings from another build.APPSEC-69906 recorded the same symptom for
default_config.json: AppSec receivednulland reportedjava.io.IOException: Resource default_config.json not found, leaving the WAF with no ruleset and no signal that it was unprotected. The available evidence does not establish that live jar replacement caused that incident; this change hardens the classloader against that concrete failure mode and enforces that indexed resource streams come from the same jar handle as agent classes.Resources whose first read happens late are the most exposed to pathname changes. Most agent resources (
InstrumenterIndex,KnownTypesIndex,ClassFileLocators, and the system-classloader reads) resolve during startup. Two late readers are AppSec's default ruleset and the WAF native library, both deferred to the remote-config poller thread underENABLED_INACTIVE.Additional Notes
For indexed agent resources,
getResourceAsStream()now intentionally gives the retained agent jar precedence overBootstrapProxyand the parent loader. In normal agent wiring,BootstrapProxyis initially registered with the same agent jar URL and the parent is the bootstrap or platform loader. In CLI wiring the parent may be an application loader, so an application resource with the same logical name as an indexed agent resource will no longer take precedence.Across a built shadow jar's 17528 entries there are 15 cases where a module-prefixed entry also has a literal copy at the un-prefixed path. All are
META-INF/MANIFEST.MF, whichAgentJarIndex.IndexGeneratorexcludes from the index, so they take the unchanged fallback path. Every product resource (default_config.json,native_libs/**,third_party_libraries.json,metricconfigs.txt,appsec.version, and the blocking templates) resolves to exactly the entryfindResource()would have built a URL for.Fallback is preserved when the retained jar does not own the requested resource or the index maps it to an entry the retained jar does not contain. It is deliberately not used after an
IOExceptionopening an entry that the retained jar does contain, because the fallback could serve that entry from a replacement build.Deliberately out of scope, since each needs a jar swap inside the startup window and all three want the same
URLStreamHandlerwork:getResource()/getResources()still return pathname-resolved URLs, so a caller that opens the URL itself can still read a replaced jar.ClassFileLocatorsreads class bytes viaBootstrapProxywithout going throughDatadogClassLoader.components/native-loaderlocates viagetResource()thenopenStream(); it has no non-test consumers today.A resource present only in the new jar can still fall through to
BootstrapProxyand be served from the replacement. The guarantee is that an entry present in the retained jar is never read from the replacement, not that the replacement can never be consulted.Follow-up worth doing independently of this change:
AppSecConfigServiceImpl.init()failing on the post-startup remote-config path throwsAbortStartupException, which is caught as a plainRuntimeExceptionand rate-limit-logged hours after startup, leavingdefaultConfigActivatedfalse. Any cause of that failure is currently silent.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueJira ticket: APPSEC-69906
🤖 Generated with Claude Code