See here https://github.com/bclozel/h2c-tomcat/tree/spring-boot-30771 for a sample app demonstrating the issue. (It's a pure Tomcat application - no Spring Boot) Summary: when processing a POST request that asks for an h2c-upgrade, reading the requestbody as an inputstream results in a NullPointerException.
An upgrade with a request body is clearly not supposed to happen with Tomcat. It's actually likely not allowed since after sending the 101 back, the request is supposed to be of the new protocol, so the request "body" will have to be HTTP/2. Clearly that's not possible (if the upgrade does not happen, for example) so it is correct that this is not allowed. The likely solution would be to avoid upgrading when there's a request body.
Also, see https://bz.apache.org/bugzilla/show_bug.cgi?id=65726 which fixed a similar issue in the past.
My reading of RFC 7230 is that when using upgrade the entire request is sent via HTTP/1.1, the upgrade happens and then the response is returned via the upgraded protocol. This is going to require buffering the request body so it is going to be subject to the maxSavePostSize limit. I would have expected this to 'just work' after the fix for bug 65726. I'll take a look at what is going on.
Ok, the spec is not as clear as I would expect. OTOH, upgrading is optional, and this has some resource use and could make some legitimate POSTs to fail (if they are over maxSavePostSize). Is it really worth it ? Shouldn't we simply skip upgrading if there's a request body ?
It doesn't work because, although a SavedRequestInputFilter is set using setInputBuffer on the Request, the available() method uses an action to check the internal buffer, which now goes to the new processor (for HTTP/2 it's a StreamProcessor) while only Http11Processor has the correct behavior for the callback. So some additional "fix" is needed, maybe intercept the callback in Request.action to provide the correct result for available when a SavedRequestInputFilter is set (rather than delegate to the hook as usual). However, I don't like the fix and there might be other similar cases. Since overall this buffering may be very inefficient, I would prefer choosing to not upgrade when a request body is present.
Interesting. I expanded the current upgrade with request body tests to use GET and POST and they still passed. Switching to a Reader triggered the issue because that triggers a call to available() and - as Rémy pointed out - that is where things go wrong. I agree with Rémy's comments on efficiency but I want to see what a patch to fix this would look like before deciding on whether I think we should support this or not. My starting point is that I would like to support it if practical.
I've committed a fix for 10.1.x. I'll give folks a chance to review it before I think about back-porting it.
Fixed in: - 10.1.x for 10.1.0-M15 onwards - 10.0.x for 10.0.21 onwards - 9.0.x for 9.0.63 onwards - 8.5.x for 8.5.79 onwards