Stop repeated NoSuchFieldError in Jackson 2.16 IAST interner lookup (quick fix)

[dougqh]

What Does This Do

Stops the Jackson 2.16 IAST instrumentation from throwing and reporting a NoSuchFieldError on every parsed JSON field name when the package-private Jackson fields it reads (UTF8StreamJsonParser._symbols, ByteQuadsCanonicalizer._interner) are missing.

Each field read has its own Latch (a small one-way, call-site-wide latch added here in internal-api):

• JsonParser216Helper latches the _symbols read, and ByteQuadsCanonicalizer216Helper latches the _interner read.
• The first NoSuchFieldError for each field is rethrown, so the instrumentation exception handler still reports it. This is not an exactly-once guarantee: threads that race the first failure each rethrow, so a failure is reported at least once, bounded by concurrency. The aim is to stop throwing forever, not to report exactly once.
• After that the failure is remembered and the read returns true ("field names are interned") without throwing.
• A classpath missing only one of the fields keeps using the other.

Adds Latch (plain flag, hint semantics; see its Javadoc) with LatchTest and LatchBenchmark, and extends JsonParser216HelperTest (JUnit 5): interned names, non-interned names, a simulated missing _interner, a simulated missing _symbols, and a second class loader that must rethrow its own first failure (child-first class loader that renames the field in the bytecode).

Design decision for IAST review

When the fields are missing we cannot tell whether names are interned, so we answer "interned". NameAdvice then records the current field name via setCurrentName but does not taint the name string (taintName is skipped). This is a deliberate trade-off:

• Jackson interns field names by default, so "interned" is the likely answer.
• Tainting an interned String taints the one shared instance, so every occurrence of that name (across requests) would look tainted: a false-positive risk. Answering "not interned" would do exactly that.
• The cost is a possible false negative: on such classpaths an attacker-controlled JSON key reaching a sink is not detected. Field values are still attributed to their field name.
• Today on those classpaths NameAdvice aborts before either setCurrentName or taintName, so neither name tracking nor name taint works. This change adds current-name tracking and does not add name taint.

The trade-off is also documented in the JsonParser216Helper Javadoc. I'd like IAST to confirm that "assume interned" is the right default.

Motivation

Instrumentation telemetry shows NoSuchFieldError from JsonParserInstrumentation$NameAdvice (about 2.9k events in 3 days, roughly 93% from one service on tracer 1.66.0, the rest a long tail on 1.56 to 1.63). I checked jackson-core 2.16.0 through 2.21.x: _symbols and _interner exist in all of them, and the StreamWriteConstraints gate first matches in 2.16.0 together with _interner. So a stock Jackson 2.16+ cannot produce this error, which points at a mixed or repackaged Jackson on the customer classpath.

Additional Notes

• Why a latch per field. A missing field fails the same way for every receiver at that site, so a single one-way flag per site is enough (no per-class state). One latch per field, rather than one shared flag, means a missing _interner does not switch off the _symbols read, and each field's first failure is reported separately, with a different top frame identifying which field is missing.

• API. The helpers call handleNoSuchField(target, read) inside apply, then tryApplyOrNull / tryApplyOrDefault(symbols, Boolean.TRUE) at the call site. handleNoSuchField latches on NoSuchFieldError and rethrows it, so the first failure is still reported; it takes a plain java.util.function.Function because a field read throws nothing checked. Latch has no built-in default: a null result means nothing is available (skipped, or no value), and the caller chooses the fallback. For _interner the boolean is computed inside the latch, so the result is never null; a null _interner is data ("not interned") and must not be confused with a skipped call.

• The failing call itself. The call that hits the failure is aborted by the advice's exception suppression before setCurrentName, so that one field name is not tracked and a value read right after it may be attributed to no name, or to the previous one. Later calls are not affected. This is also in the JsonParser216Helper Javadoc.

• Flag is plain, not volatile. The previous version used a static volatile boolean. Latch uses a plain flag on purpose: a stale read only costs another failure, and a thread always sees its own write, so each thread pays for at most one extra failure after its own first. This is a per-field-name path, so the read cost matters. LatchBenchmark, added here, measures this exact shape: a volatile flag costs about 0.5 ns more than a plain one on the working path at depth 0 (4.04 against 3.55 ns), and Latch matches a hand-rolled plain flag (see the benchmark note below).

• Benchmark. LatchBenchmark (JMH; a real NoSuchFieldError is built at setup, as in ClassLatchBenchmark). One run on Zulu 17.0.7 (HotSpot), MacBook M1, single thread, 5 forks, on a laptop with normal background activity (load about 4); the full table is in the benchmark's Javadoc. JDK 8 and x86 are not measured. The numbers were measured on the commit before the hook was renamed apply and the public methods tryApply*; that rename is naming only.

  Arm	depth 0	depth 50	Allocation (0 / 50)
  status quo (read and catch every call)	3,489 ns	5,100 ns	768 B / 2,128 B
  Latch, skipped	2.18 ns	28.2 ns	0
  hand-rolled plain flag, skipped	2.18 ns	27.7 ns	0
  hand-rolled volatile flag, skipped	2.44 ns	29.6 ns	0
  working path, direct read	3.35 ns	33.3 ns	0
  working path, Latch	3.53 ns	31.3 ns	0
  working path, plain flag	3.55 ns	39.4 ns (±6%)	0
  working path, volatile flag	4.04 ns	32.1 ns	0

  A latched skip is about 1,600 times cheaper than the status quo at depth 0 and about 180 times at depth 50, and allocates nothing. Latch is as cheap as a hand-rolled plain flag, so the abstraction costs nothing measurable, and a volatile flag costs more (about 0.5 ns on the working path and about 0.3 ns when skipping, at depth 0). At depth 50 only the skipped arms are reliable (28 to 30 ns, consistent across forks): the working-path arms span 31 to 39 ns, and the plain flag, which is the same logic as Latch, came out slowest with one fork at 28.9M against 24.2M to 24.8M ops/s for the others, so I read that as JIT and recursion noise, not a difference between the designs.

• Null _symbols. If the _symbols read ever returned null, the answer is now "assume interned" where the old code would have thrown an NPE. _symbols is never null in stock Jackson.

• Class loaders and lambdas. The helpers are injected into the application class loader, now reference datadog.trace.util.Latch from internal-api, and contain two small lambdas (parser -> parser._symbols, s -> s._interner != null); muzzle passes across the Jackson versions it checks.

• The design of Latch and its per-class sibling is tracked in APMLP-1894 (the sibling is in Skip repeated AbstractMethodError from JDBC getClientInfo #12702).

• The root cause on the affected classpath is not confirmed: the telemetry error.message is empty, so the missing field name is unknown. This change bounds the cost of the failure and does not fix the classpath.

• The failure simulation is synthetic; I have not seen the customer's real classpath.

• Only NoSuchFieldError is handled. Other linkage errors still propagate as before.

• Existing Groovy IAST tests in the module pass locally.

Contributor Checklist

• Format the title according to the contribution guidelines
• Assign the type: and (comp: or inst:) labels in addition to any other useful labels
• Avoid using close, fix, or any linking keywords when referencing an issue
  Use solves instead, and assign the PR milestone to the issue
• Update the CODEOWNERS file on source file addition, migration, or deletion
• Update public documentation with any new configuration flags or behaviors
• Once approved, use merge queue to merge the PR

Jira ticket: APMLP-1894

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bb4e5331ef

ℹ️ 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".

[dd-octo-sts]

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.07 s	13.96 s	[-0.0%; +1.7%] (no difference)
startup:insecure-bank:tracing:Agent	13.01 s	13.12 s	[-1.8%; +0.2%] (no difference)
startup:petclinic:appsec:Agent	17.25 s	17.06 s	[+0.2%; +2.0%] (maybe worse)
startup:petclinic:iast:Agent	16.98 s	16.94 s	[-0.7%; +1.1%] (no difference)
startup:petclinic:profiling:Agent	16.56 s	16.28 s	[-2.9%; +6.3%] (no difference)
startup:petclinic:sca:Agent	17.07 s	16.96 s	[-0.4%; +1.7%] (no difference)
startup:petclinic:tracing:Agent	16.19 s	16.18 s	[-0.9%; +1.1%] (no difference)

Commit: 05bfd5a0 · CI Pipeline · Benchmarking Platform UI

---

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

[jandro996]

non-blocking: The IAST design decision (unavailable fields => NameAdvice calls setCurrentName but skips taintName) is only asserted at the helper level (fetchInterner returns true). Nothing checks the actual advice behaviour, which is what the PR description asks IAST to sign off on. Consider a small test (e.g. a Groovy JsonParserInstrumentationTest case) using the missing-field loader that verifies taintString is called for values with the field name and not for the name itself. Otherwise a later change to NameAdvice could silently flip the trade-off.

[dougqh]

The trade-off is pinned from both sides today: test json parsing (tainted but field names) goes through the fetchInterner() == true branch and asserts no name taint (0 * _), so flipping that branch would fail it, and JsonParser216HelperTest asserts that a missing _symbols or _interner yields true, which routes into that same branch.

A true end-to-end test would need the agent to instrument classes from a child-first loader in that harness, which I haven't verified works. Since it's non-blocking, I'd leave it as a follow-up if IAST wants it.

Reply written by Claude (Claude Code) on behalf of dougqh.

[jandro996]

non-blocking: The Javadoc and PR text say the first failure is reported "once". Since fieldsUnavailable is only set here in the catch, threads racing before the first failure completes each rethrow, so it is really "once per racing thread". The bounded cost is fine; I would just reword the Javadoc to "at least once, bounded by concurrency" (the Codex bot already raised the race, so not flagging it separately).

[dougqh]

Yes, agreed, I'll fix that. I'm not aiming for an exactly once guarantee.
I just want to prevent constantly throwing Errors forever.

[dougqh]

Reworded in 092eaf0: both helpers' Javadoc now say a failure is reported at least once, bounded by concurrency, and the PR description matches.

This also covers the Codex comment above about making the transition atomic: the goal is to stop repeated throws, and a thread that races the first failure pays at most one extra, so I've left the transition non-atomic.

Reply written by Claude (Claude Code) on behalf of dougqh.

[jandro996]

nit: Only a missing _interner is simulated. The _symbols branch goes through the same catch, but a parametrized case would prove it. Also, a second MissingInternerClassLoader instance is not checked to rethrow once, which is what "per class loader" claims.

[dougqh]

Added both: a _symbols case (rethrowsFirstMissingSymbolsThenAssumesInterned) and eachClassLoaderRethrowsItsOwnFirstFailure, which runs two separate loaders with the same missing field. Both use one shared helper, assertRethrowsOnceThenAssumesInterned(className, field), rather than a parametrized test. The loader is now MissingFieldClassLoader, so it can rename either field.

Reply written by Claude (Claude Code) on behalf of dougqh.

[jandro996]

nit: On the rethrown first call, NameAdvice aborts before setCurrentName, so the first field name after the failure is not tracked and its value is attributed to a stale or null name. Harmless, but worth a line in the Javadoc next to the trade-off note.

[dougqh]

Added to the JsonParser216Helper Javadoc, next to the trade-off note: the call that hits the failure is aborted by the advice's exception suppression before setCurrentName, so that one field name isn't tracked and the value after it may be attributed to no name or the previous one. Later calls aren't affected. The PR description has the same note.

Reply written by Claude (Claude Code) on behalf of dougqh.

[dougqh]

I think I'm going to create a generic guard around this similar to what I'm doing for AbstractMethodError.
Until I get to that, I've put this PR back to draft mode.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9f470827b

ℹ️ 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".

[datadog-prod-us1-5]

The new anonymous latch subclasses are not injected into application class loaders, so helper initialization can fail and silently disable Jackson 2.16+ IAST name tracking.

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit e9f4708 · @DataDog review to ask questions}