You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2022/09/02 22:53:40 UTC

[GitHub] [tomcat] malaysf opened a new pull request, #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

malaysf opened a new pull request, #550:
URL: https://github.com/apache/tomcat/pull/550

   Fixing a bug where a connection would be kept alive without swallowing the input from the request stream.
   
   When a request comes in with a 100 Continue expectation, and the server immediately responds with a 200 status without reading the request body at all, the next request on the connection will fail because Tomcat cannot parse the HTTP verb. The first request left Http11Processor in a state where it kept the connection alive but did not discard the request body, thus reading the next request began at the wrong place in the stream.
   
   The new unit test replicates the issue and is addressed by the code changes to keep the keepAlive and input buffer swallow state in sync with each other.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

markt-asf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1240278052

   > Are you referring to the call to `inputBuffer.setSwallowInput(true);` in `Http11Processor.ack`? That does appear to be unnecessary now.
   
   Yes. That was the one.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] malaysf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

malaysf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1238525462

   @markt-asf That does work and is a much simpler solution! It also doesn't break TestSwallowAbortedUploads (which I now understand why).
   
   I will update my PR with that change. What is the other call that you think could be removed?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

markt-asf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1238513982

   I think you can just remove the `inputBuffer.setSwallowInput(false)` in `Http11Processor.prepareExpectation()`. If that is correct, I think there is at least one other call that can also be removed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] markt-asf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

markt-asf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1239284952

   I suspect the call in `Http11Processor.ack()` can also be removed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] markt-asf merged pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

markt-asf merged PR #550:
URL: https://github.com/apache/tomcat/pull/550


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] malaysf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

malaysf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1238435761

   The TestSwallowAbortedUploads test fails so I need to look into that.
   
   We ran into this issue in production and were able to reproduce with curl or postman requests to an API that returns 200 without ever reading the request body.
   
   I don't think that the core logic is wrong in `Http11Processor.ack`, in this situation `ack` is never called since the request body input stream is never read from, which triggers the `ack`.
   
   When a request is started, `Http11Processor.prepareExpectation` sets `inputBuffer.setSwallowInput(false);` but `keepAlive` is still set to true. If `keepAlive` is true, then the inputBuffer's swallow state must be set to true as well to ensure that the request stream is advanced to the beginning of the next request, regardless of whether the servlet consumed all the bytes.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] malaysf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

malaysf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1244022378

   @markt-asf Thanks for reviewing and merging. Could this please also be included in 9.0.x?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] rmaucher commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

rmaucher commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1237008899

   A request with an expectation is not supposed to be performed the same way as an identical request without it. So you have to be careful and I don't think your patch is correct.
   Looking at the code in Http11Processor.ack, it looks to me that it should work so I don't really understand yet.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] malaysf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

malaysf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1241479251

   I've included that change too. Please let me know if there's anything else that should be done. Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[GitHub] [tomcat] malaysf commented on pull request #550: Http11Processor's keep alive state and input buffer's swallow state must be synchronized

Posted by GitBox <gi...@apache.org>.

malaysf commented on PR #550:
URL: https://github.com/apache/tomcat/pull/550#issuecomment-1239848521

   Are you referring to the call to `inputBuffer.setSwallowInput(true);` in `Http11Processor.ack`? That does appear to be unnecessary now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org