Symbol database: stop invoking application-overridable reflection during extraction#5946
Conversation
p-datadog
added
the
AI Generated
dd-octo-sts
Bot
added
the
debugger
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0210296 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-25 19:51:16 Comparing candidate commit 0210296 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR hardens Datadog::SymbolDatabase::Extractor so symbol database extraction avoids calling application-overridable reflection methods directly, reducing the risk of side effects, incorrect metadata, or autoload-triggered code execution during passive introspection.
Changes:
- Dispatches core reflection APIs (
name,superclass,included_modules,ancestors,constants,const_get,const_defined?,autoload?, etc.) through cachedUnboundMethods and routes name lookups throughsafe_mod_name. - Updates scope classification (
resolve_scope_type) to walk namespaces segment-by-segment with autoload guards instead ofObject.const_get. - Adds specs covering “hostile” overrides of reflection methods and updates the RBS signature to include the new cached
UnboundMethodconstants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/datadog/symbol_database/extractor.rb |
Uses cached unbound reflection methods + safer scope-type resolution to avoid application overrides/autoload side effects during extraction. |
sig/datadog/symbol_database/extractor.rbs |
Adds RBS declarations for the newly introduced cached UnboundMethod constants. |
spec/datadog/symbol_database/extractor_spec.rb |
Adds regression coverage ensuring extraction is immune to customer-overridden reflection and avoids autoload triggering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
Extraction walks arbitrary application objects via reflection. Calling name/superclass/included_modules/ancestors/class/is_a?/const_get/constants/ autoload? directly executes application code (and, for const_get, can autoload as a side effect): an overridden method can return wrong data, run customer code during a passive introspection pass, or raise. Cache UnboundMethods for these and dispatch them explicitly (the precedent set by MODULE_NAME / safe_mod_name), route every name lookup through safe_mod_name, and replace is_a? checks with Klass === obj. resolve_scope_type now walks the FQN segment by segment with autoload guards instead of Object.const_get, so it classifies CLASS vs MODULE without triggering autoloads.
Cover that build_class_language_specifics, extract_scope_symbols, resolve_scope_type, and extract read real data and avoid side effects when an application overrides name/superclass/included_modules/ancestors/class/is_a?/ const_get/constants, including the autoload-safe classification path.
Wrap the `Class === mod` / `Class === current` ternary conditions in parentheses. Replacing `mod.is_a?(Class) ? ...` with `Class === mod ? ...` introduced complex (binary-operator) ternary conditions, which StandardRB's Style/TernaryParentheses cop requires to be parenthesized. The is_a? form was a method call and did not trigger the cop. This was failing the standard/lint CI check at extractor.rb:920, 925, 954. Verified: `bundle exec standardrb` exits 0 (clean).
f32ae41 to
0210296
Compare
What does this PR do?
Stops symbol database extraction from calling application-overridable reflection methods directly. Extraction now dispatches
name,superclass,included_modules,ancestors,class,is_a?,const_get,constants,class_variables, andautoload?through cached unboundModule/Class/Kernelmethods (the precedent already set byMODULE_NAME/safe_mod_name), routes every name lookup throughsafe_mod_name, and replacesobj.is_a?(Klass)checks withKlass === obj.resolve_scope_typenow walks the fully-qualified name segment by segment with autoload guards instead ofObject.const_get, so it classifies CLASS vs MODULE without triggering autoloads.Motivation:
Extraction introspects arbitrary application objects. Calling these methods directly executes application code during a passive introspection pass: an overridden method can return wrong data, run customer code as a side effect, or raise, and
const_get/Object.const_getcan autoload (loading customer code) while merely classifying a constant.safe_mod_namealready guarded the primary module-name lookup; this extends the same protection to the remaining sites.Change log entry
None.
Additional Notes:
master.BasicObjectis now classified (type: "BasicObject") instead of being dropped, because the real class is read via an unboundKernel#class.How to test the change?
bundle exec rake standard typecheckis clean.spec/datadog/symbol_database/extractor_spec.rb("immunity to customer-overridden reflection") assert each site reads real data and avoids side effects when an application overrides reflection, including the autoload-safe classification path.bundle exec rspec spec/datadog/symbol_database— 372 examples, 0 failures.