Skip to content

fix(undertow): null-check context key in UndertowActiveHandlers to prevent NPE#17559

Open
divitsinghall wants to merge 1 commit intoopen-telemetry:mainfrom
divitsinghall:fix/undertow-active-handlers-npe
Open

fix(undertow): null-check context key in UndertowActiveHandlers to prevent NPE#17559
divitsinghall wants to merge 1 commit intoopen-telemetry:mainfrom
divitsinghall:fix/undertow-active-handlers-npe

Conversation

@divitsinghall
Copy link
Copy Markdown

Summary

Fixes a possible NPE in UndertowActiveHandlers reported in #16128.

context.get(CONTEXT_KEY) can return null if init() was never called
for that context (e.g. in rare race conditions or atypical request flows).
Both increment() and decrementAndGet() previously dereferenced the
result without a null check, causing an NPE that failed real HTTP requests.

Occurs roughly once or twice per tens of millions of requests — rare but
impactful since it drops legitimate traffic.

Changes

  • increment(): no-op when counter is absent (counter was never initialized)
  • decrementAndGet(): returns 1 when counter is absent, so callers that
    check == 0 to end a span will safely skip that path

Tests

Added UndertowActiveHandlersTest covering:

  • increment() and decrementAndGet() on an uninitialized context (the NPE paths)
  • Normal initialized counter behavior
  • Verification that uninitialized increment does not affect other contexts

Fixes #16128

@divitsinghall divitsinghall requested a review from a team as a code owner April 5, 2026 07:05
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Apr 5, 2026

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: divitsinghall / name: divitsinghall (30428f6)

@laurit
Copy link
Copy Markdown
Contributor

laurit commented Apr 6, 2026

@divitsinghall were you able to reproduce the issue? Or have you managed to come up with an explanation why the counter is missing from the context?

@divitsinghall
Copy link
Copy Markdown
Author

@divitsinghall were you able to reproduce the issue? Or have you managed to come up with an explanation why the counter is missing from the context?

Thanks for reviewing!
The counter is missing from the context when increment() or decrementAndGet() is called on a context that never had init() called on it. init() is what stores the AtomicInteger under CONTEXT_KEY so if that path is skipped (e.g. a request enters through an atypical code path that bypasses the normal handler setup), context.get(CONTEXT_KEY) returns null and both methods NPE on the dereference.
The reporter confirmed it happens roughly once or twice per tens of millions of requests, which suggests it's a rare but real code path where the context doesn't go through the standard initialization. I wasn't able to reproduce it locally either since it requires specific runtime conditions, but the NPE is clear from the code: context.get() is not guaranteed to return non-null if init() was never called for that context instance.

@laurit
Copy link
Copy Markdown
Contributor

laurit commented Apr 7, 2026

The counter is missing from the context when increment() or decrementAndGet() is called on a context that never had init() called on it.

This is exactly what I'm interested in. Which code path passes context without the counter to these methods? I guess we can add the null check even if we don't understand why this happens, though it would be nice if we did. Perhaps the real bug is somewhere else.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment makes it look like context being not initialized is somehow expected

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced of the usefulness of this test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps -1 would be better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible NPE In Undertow Instrumentation

2 participants