Add null checks for calls to getAllLoadedClasses - #12236
Conversation
Instrumentation::getAllLoadedClasses can return null slot in the class array in case of class unloading. Make sure that all calls to the method check for null class while iterating over the returned array.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929dbd724a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| final Instrumentation instrumentation, final Set<Class<?>> visited) { | ||
| List<Class<?>> retransforming = new ArrayList<>(); | ||
| for (Class<?> clazz : instrumentation.getAllLoadedClasses()) { | ||
| if (clazz != null) { |
There was a problem hiding this comment.
When getAllLoadedClasses() returns its normal non-null class entries, this condition skips every one, so the rediscovery strategy returns an empty list and AgentInstaller never retransforms already-loaded classes. If the array does contain the null slot this change intends to handle, that slot instead falls through and is dereferenced on the next line, causing a NullPointerException; reverse the condition so only null entries are skipped.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9fdc046 | Docs | View more details | Give us feedback! |
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new rediscovery guard skips every non-null loaded class, so retransformation stops immediately; if a null slot appears, that entry is still dereferenced.
🤖 Datadog Autotest · Commit 929dbd7 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| final Instrumentation instrumentation, final Set<Class<?>> visited) { | ||
| List<Class<?>> retransforming = new ArrayList<>(); | ||
| for (Class<?> clazz : instrumentation.getAllLoadedClasses()) { | ||
| if (clazz != null) { |
There was a problem hiding this comment.
Class rediscovery selects no ordinary loaded classes, disabling required retransformation, while the intended null-slot case can still crash the path.
Assertion details
- Input: Any
getAllLoadedClasses()result containing normal non-null classes; a null slot additionally triggers the dereference failure. - Expected:
Skip null array entries while continuing to evaluate non-null loaded classes for retransformation. - Actual: Every non-null class is skipped. A null class proceeds to
clazz.getClassLoader()and throws a NullPointerException.
| if (clazz != null) { | |
| if (clazz == null) { |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · 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. |
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 5 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (331.545 µs) : 310, 353
. : milestone, 332,
basic (306.23 µs) : 299, 313
. : milestone, 306,
loop (8.982 ms) : 8978, 8986
. : milestone, 8982,
section candidate
noprobe (336.569 µs) : 304, 369
. : milestone, 337,
basic (307.27 µs) : 300, 314
. : milestone, 307,
loop (8.981 ms) : 8976, 8986
. : milestone, 8981,
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
2 participants
What Does This Do
Instrumentation::getAllLoadedClasses can return null slot in the class array in case of class unloading. Make sure that all calls to the method check for null class while iterating over the returned array.
Motivation
Additional Notes
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: [DEBUG-5575, DEBUG-5922]