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

Uncaught NPE when parsing a header value #574

Closed
nigredo-tori opened this issue Jul 9, 2024 · 8 comments · Fixed by #575
Closed

Uncaught NPE when parsing a header value #574

nigredo-tori opened this issue Jul 9, 2024 · 8 comments · Fixed by #575
Labels
bug Something isn't working
Milestone

Comments

@nigredo-tori
Copy link

nigredo-tori commented Jul 9, 2024

I'm using Akka 10.2.9, but the issue (as in, broken error handling) doesn't seem to have been fixed in Pekko as well. Sorry I don't have a reproduction example at this moment, I'm creating this issue to have something to reference when rolling out a patch that will hopefully give me a clue. 😄

I have a piece of code like this:

`Content-Disposition`.parseFromValueString(headerValue)

I expect it to return an Either. Instead, for some input values it seems to throw the following exception:

java.lang.NullPointerException: Cannot invoke "String.split(String, int)" because "message" is null
	at akka.http.scaladsl.model.ErrorInfo$.fromCompoundString(ErrorInfo.scala:65)
	at akka.http.impl.model.parser.HeaderParser.failure(HeaderParser.scala:77)
	at akka.http.scaladsl.model.HttpHeader$.parse(HttpHeader.scala:92)
	at akka.http.scaladsl.model.headers.ModeledCompanion.parseFromValueString(headers.scala:40)
        <redacted>

It seems that there is an exception raised somewhere inside the parser that doesn't have its message set. Here we do .getMessage, passing null into ErrorInfo.fromCompoundString. It tries to split it, and we get an NPE.

@jrudolph jrudolph added the bug Something isn't working label Jul 9, 2024
@jrudolph
Copy link
Contributor

jrudolph commented Jul 9, 2024

Thanks for the report. HeaderParser.failure tries exactly to convert exceptions into an Either-like error but misses the fact that exception messages can be null. We could either return an empty ErrorInfo or try to be a bit more informative and infer some info from the exception class name.

@pjfanning pjfanning added this to the 1.1.0-M2 milestone Jul 9, 2024
@laglangyue
Copy link
Contributor

image

we should add case

@pjfanning
Copy link
Contributor

image we should add case

@laglangyue would you have time to do a PR?

@laglangyue
Copy link
Contributor

@pjfanning yes, I am designing some tests to reproduce errors and make modifications

@laglangyue
Copy link
Contributor

Although I added a check that can fix it, the message of NPE is not null on Java17.

I dig deep again,
image
image

I hope to identify NPE in advance, so I still need some time to read the code in the parse section

@pjfanning
Copy link
Contributor

pjfanning commented Jul 9, 2024

Use the getMessage if it is not null even if NullPointerException has changed in Java 17. We support Java 8+.
Test cases may not to be handle supporting different behaviour depending on the Java Runtime Version being used.
With StringBasedParserInput, you can change the code that creates instances of StringBasedParserInput to pass in the exception toString instead of getMessage when the getMessage returns null or empty string. Use getMessage when there is a non-empty string (to retain compatibility with existing behaviour).

@pjfanning
Copy link
Contributor

I'd like to see the fix backported to the 1.0.x branch.

@pjfanning
Copy link
Contributor

PRs merged to main and 1.0.x branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants