Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JAX-RS filter NPE for empty entities #1909

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

msdousti
Copy link
Collaborator

@msdousti msdousti commented Sep 22, 2024

In JAX-RS, when Logbook is registered as a response filter, and the response entity is empty, an NPE is thrown.
Same seems to be the case for request filter and an empty request.

Description

The issue was reported to us but marked as not reproducible (see below). However, I could reproduce it here:
https://github.com/msdousti/quarkus-logbook

If you use install this branch to your local Maven, it works as expected. However, if you change logbook-bom.version in the above repo to 3.9.0 (current Logbook version), and run the test:

curl -iI http://localhost:8080/hello
HTTP/1.1 500 Internal Server Error
content-type: text/plain; charset=utf-8

In the server logs:

2024-09-22 02:13:20,426 TRACE [org.zal.log.Logbook] (executor-thread-1) Incoming Request: f0ce7f5363945e86
Remote: localhost:8080
HEAD http://localhost:8080/hello HTTP/1.1
Accept: */*
Host: localhost:8080
User-Agent: curl/8.5.0

2024-09-22 02:13:20,433 TRACE [org.zal.log.Logbook] (executor-thread-1) Outgoing Response: f0ce7f5363945e86
Duration: 11 ms
HTTP/1.1 200 OK
2024-09-22 02:13:20,437 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-1) HTTP Request to /hello failed, error id: e5624c9d-1e32-48e8-b1b3-56183b37467c-1: java.lang.NullPointerException: Cannot invoke "java.io.OutputStream.close()" because "this.original" is null
        at org.zalando.logbook.jaxrs.TeeOutputStream.close(TeeOutputStream.java:39)
        at io.quarkus.resteasy.runtime.standalone.VertxHttpResponse.finish(VertxHttpResponse.java:145)
        at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.dispatch(VertxRequestHandler.java:174)
...

Root Cause

  1. LogbookServerFilter#filter calls response.expose().

  2. expose calls state.buffer(context)

  3. buffer is implemented as follows:

    @Override
    public State buffer(final ContainerResponseContext context) {
    final TeeOutputStream stream = new TeeOutputStream(context.getEntityStream());
    context.setEntityStream(stream);
    return new Buffering(stream);
    }

  4. When the response has no entity, then context.getEntityStream() is null. As such, TeeOutputStream will have a null stream (called "original"). When original.close is called, an NPE is thrown.

The exact same issue happens for Request Filters, when request.expose is called.

Fix

The fix is to call expose methods conditionally, only when the request/response entity exists.

The fix is to annotate origin in TeeOutputStream as nullable, and then handle nullability everywhere. Thanks to @kasmarian for the suggestion!

TeeOutputStream(@Nullable final OutputStream original) {
    this.original = original;
}

Test

The existing tests use ResourceConfig (example), and they don't seem to suffer from this issue.

Quarkus does not seem to support ResourceConfig, so in my Quarkus project, I used a @Provider (see LogbookFilterSetup).

I wasn't able to write an automated test to cover this, but the existing tests plus cover for all branches already. Suggestions are welcome.

Motivation and Context

Addresses #1384

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@msdousti
Copy link
Collaborator Author

msdousti commented Nov 3, 2024

👍

@kasmarian
Copy link
Member

@lukasniemeier-zalando @ChristianLohmann could you have a look?

@lukasniemeier-zalando lukasniemeier-zalando merged commit f2183b4 into main Nov 5, 2024
4 checks passed
@lukasniemeier-zalando lukasniemeier-zalando deleted the fix-jaxrs-issue branch November 5, 2024 13:06
@msdousti msdousti restored the fix-jaxrs-issue branch November 10, 2024 20:52
@msdousti msdousti deleted the fix-jaxrs-issue branch November 10, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants