NPE for body-less requests

[chmllr]

Hello Brian,

I think I've found a bug in accesslog. Consider the following code:

Request request = newRequest();
request.cookies = someCookies;
request.path = somePath;
makeRequest(request);

This code works in 1.2.4 without any problems. However, with accesslog it fails with an NPE, because of the following code in AccessLogPlugin.java:

if (_shouldLogPost)
{
    line = StringUtils.replaceOnce(line, "%post", request.params.get("body"));
}

The problem we have here, is that this code tries to extract the body from the request, which leads to an NPE in Play, because Play assumes, that body is always an InputStream(). In my sample code the request is body-less. If you look at the definition of newRequest() in Play, you'll see that the body will be set to null, and the request's type is GET (so it shouldn't be relevant for the code snippet from accesslog).

If I add the following line

request.body = new ByteArrayInputStream(new byte[0]);

before the last line in the first code snippet, the code works without any problems again.

I suspect, that body is only initialized for POST-requests. However, you only check whether POST-requests should be logged, but you don't check whether the request is a POST request as well.

What do you think?

PS: if you agree, I could provide a patch.

[briannesbitt]

Could very well be. Let me look into it and get back to you.

Sent from my iPad

On Apr 22, 2012, at 1:55 PM, "Christian Müller"
[email protected]
wrote:

Hello Brian,

I think I've found a bug in accesslog. Consider the following code:

Request request = newRequest();
request.cookies = someCookies;
request.path = somePath;
makeRequest(request);

This code works in 1.2.4 without any problems. However, with accesslog it fails with an NPE, because of the following code in AccessLogPlugin.java:

if (_shouldLogPost)
{
line = StringUtils.replaceOnce(line, "%post", request.params.get("body"));
}

The problem we have here, is that this code tries to extract the body from the request, which leads to an NPE in Play, because Play assumes, that body is always an InputStream(). In my sample code the request is body-less. If you look at the definition of newRequest() in Play, you'll see that the body will be set to null, and the request's type is GET (so it shouldn't be relevant for the code snippet from accesslog).

If I add the following line

request.body = new ByteArrayInputStream(new byte[0]);

before the last line in the first code snippet, the code works without any problems again.

I suspect, that body is only initialized for POST-requests. However, you only check whether POST-requests should be logged, but you don't check whether the request is a POST request as well.

What do you think?

PS: if you agree, I could provide a patch.

---

Reply to this email directly or view it on GitHub:
#5

[briannesbitt]

The request type should be checked. Not sure about the NPE since the StringUtils.replaceOnce() method allows any parameter to be null. Do you have a stacktrace? Maybe its happening in the .get() call ? https://github.com/playframework/play/blob/master/framework/src/play/mvc/Scope.java#L433

I did commit this, but I want to better understand where the NPE is coming from before pushing to the playframework modules repo.

cb1344d

[chmllr]

Do you have a stacktrace? Maybe its happening in the .get() call ?

I can tell you exactly where it comes from. At the line 386 of Scope.java we have:

_mergeWith(dataParser.parse(request.body));

Remember, request.body is initialized with null in newRequest(). Then, in TextParser.parse(), line 18:

while ((b = is.read()) != -1) ...

Here, the request.body is used as an InputStream parameter is. So, calling read()on it leads to the NPE. In accesslog, the NPE is coming from the internal call caused by this line:

String body = request.params.get("body");

I hope I could help.

Btw, your new commit would fix the problem for me, however, it is not clear, whether we can make an implication POST-request => body is never null. So, I'd wrap the new line 135 in your commit with a try-catch.