Fix RUM injection for offset response writes#11949
Fix RUM injection for offset response writes#11949gh-worker-dd-mergequeue-cf854d[bot] merged 6 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Commit d95beb4 Reproduces the regression standalone. The following commits fix the regression |
|
@codex review |
There was a problem hiding this comment.
More details
The PR correctly fixes a critical offset-handling bug in RUM injection that corrupted HTML responses when servlets wrote from non-zero array offsets. The code had mixed absolute array indices with slice-relative indices; fixes convert all calculations to handle offsets properly. Three focused test cases and a Thymeleaf smoke test validate the fix and lock in real-world scenarios.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit ea2c639 · What is Autotest? · Any feedback? Reach out in #autotest
🟢 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea2c639e49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
amarziali
left a comment
There was a problem hiding this comment.
Thanks for the fix and the tests. The changes sounds good. One optional suggestion to make it harder to reintroduce this bug: every site in the bulk-write branch now recomputes off + … by hand, and the original bug was exactly a "did I mean len or off+len" slip at one of these. Could we introduce a single final int end = off + len; at the top of the branch and express all bounds as the half-open range [off, end)? Happy to merge as-is if you'd rather not churn it. Same applies to the char version in InjectingPipeWriter.
| command.add(javaPath()) | ||
| command.addAll(defaultJavaProperties) | ||
| command.addAll((String[]) [ | ||
| "-Ddd.rum.enabled=true", |
There was a problem hiding this comment.
Can we have a specific smoke test for RUM and keep the existing without?
|
|
||
| if (len > bulkWriteThreshold) { | ||
| // A match that started in the previous write precedes every match wholly in this array. | ||
| if (matchingPos > 0 && arrayCompletesPendingMatch(array, off, len)) { |
There was a problem hiding this comment.
what about havingfinal int end = off + len; since off + len is quite a lot calculated in all the class (same for InjectingPipeWriter
| } | ||
| bytesToWrite = len - idx; | ||
| downstream.write(array, off + idx, bytesToWrite); | ||
| bytesToWrite = off + len - idx; |
There was a problem hiding this comment.
that would translate to end - idx
Can be overridden no worries |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
Jira: RUMS-6106
What changed
InjectingPipeWriterandInjectingPipeOutputStream.Why
The bulk-write paths mixed absolute array indexes with indexes relative to
len. When a servlet or template stack wrote a response from a non-zero array offset, six characters around the</head>marker could be moved across the injected RUM snippet. This corrupted otherwise valid response markup and could also corrupt dynamic inline JavaScript or rendered URLs.The fix preserves the original response exactly, injects the RUM snippet once, and keeps original response-size accounting unchanged.
Validation
</head>integrity assertion and passes afterward../gradlew :dd-java-agent:agent-bootstrap:test./gradlew :dd-smoke-tests:spring-boot-3.3-webmvc:test./gradlew :dd-java-agent:agent-bootstrap:spotlessCheck :dd-smoke-tests:spring-boot-3.3-webmvc:spotlessCheckThe local smoke build excluded the unrelated CICS 9.1 instrumentation aggregate because
public.dhe.ibm.comwas not resolvable; the Spring Boot 3.3 WebMVC module and its smoke tests completed successfully.