This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 10.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.0.x by this push:
new dd875f01e2 Fix BZ 66023 - improve handling of HTTP upgrade with
request body
dd875f01e2 is described below
commit dd875f01e2e57602a7d180e43fcda8b14d5c60d3
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Apr 29 11:17:39 2022 +0100
Fix BZ 66023 - improve handling of HTTP upgrade with request body
https://bz.apache.org/bugzilla/show_bug.cgi?id=66023
The previous fix (BZ 65726) made the saved request body available to the
request object but didn't update the processor. This patch extends the
previous fix to make the request and processor objects consistent. It
addresses the issue identified in 66023 and should address any
additional, related issues. If a use case has been missed, the fix
should be as simple as an update to the SavedRequestStreamInputBuffer.
---
java/org/apache/coyote/http2/Stream.java | 97 ++++++++++++++++++++--
test/org/apache/coyote/http2/Http2TestBase.java | 43 ++++++++--
.../coyote/http2/TestHttp2UpgradeHandler.java | 49 +++++++++--
webapps/docs/changelog.xml | 4 +
4 files changed, 171 insertions(+), 22 deletions(-)
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 03eaf43e9b..d1074c4cff 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -34,6 +34,7 @@ import org.apache.coyote.Request;
import org.apache.coyote.Response;
import org.apache.coyote.http11.HttpOutputBuffer;
import org.apache.coyote.http11.OutputFilter;
+import org.apache.coyote.http11.filters.SavedRequestInputFilter;
import org.apache.coyote.http2.HpackDecoder.HeaderEmitter;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
@@ -99,12 +100,13 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
if (coyoteRequest == null) {
// HTTP/2 new request
this.coyoteRequest = new Request();
- this.inputBuffer = new StreamInputBuffer();
+ this.inputBuffer = new StandardStreamInputBuffer();
this.coyoteRequest.setInputBuffer(inputBuffer);
} else {
// HTTP/2 Push or HTTP/1.1 upgrade
this.coyoteRequest = coyoteRequest;
- this.inputBuffer = null;
+ this.inputBuffer = new SavedRequestStreamInputBuffer(
+ (SavedRequestInputFilter) coyoteRequest.getInputBuffer());
// Headers have been read by this point
state.receivedStartOfHeaders();
if (HTTP_UPGRADE_STREAM.equals(identifier)) {
@@ -1024,7 +1026,27 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
- class StreamInputBuffer implements InputBuffer {
+ abstract class StreamInputBuffer implements InputBuffer {
+
+ abstract void receiveReset();
+
+ abstract void swallowUnread() throws IOException;
+
+ abstract void notifyEof();
+
+ abstract ByteBuffer getInBuffer();
+
+ abstract void onDataAvailable() throws IOException;
+
+ abstract boolean isReadyForRead();
+
+ abstract boolean isRequestBodyFullyRead();
+
+ abstract void insertReplayedBody(ByteChunk body);
+ }
+
+
+ class StandardStreamInputBuffer extends StreamInputBuffer {
/* Two buffers are required to avoid various multi-threading issues.
* These issues arise from the fact that the Stream (or the
@@ -1197,7 +1219,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
- private final ByteBuffer getInBuffer() {
+ final ByteBuffer getInBuffer() {
ensureBuffersExist();
return inBuffer;
}
@@ -1224,7 +1246,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
- private final void receiveReset() {
+ final void receiveReset() {
if (inBuffer != null) {
synchronized (inBuffer) {
resetReceived = true;
@@ -1233,7 +1255,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
}
- private final void notifyEof() {
+ final void notifyEof() {
if (inBuffer != null) {
synchronized (inBuffer) {
inBuffer.notifyAll();
@@ -1241,7 +1263,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
}
- private final void swallowUnread() throws IOException {
+ final void swallowUnread() throws IOException {
synchronized (this) {
closed = true;
}
@@ -1267,4 +1289,65 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
}
}
+
+
+ class SavedRequestStreamInputBuffer extends StreamInputBuffer {
+
+ private final SavedRequestInputFilter inputFilter;
+
+ SavedRequestStreamInputBuffer(SavedRequestInputFilter inputFilter) {
+ this.inputFilter = inputFilter;
+ }
+
+
+ @Override
+ public int doRead(ApplicationBufferHandler handler) throws IOException
{
+ return inputFilter.doRead(handler);
+ }
+
+ @Override
+ public int available() {
+ return inputFilter.available();
+ }
+
+ @Override
+ void receiveReset() {
+ // NO-OP
+ }
+
+ @Override
+ void swallowUnread() throws IOException {
+ // NO-OP
+ }
+
+ @Override
+ void notifyEof() {
+ // NO-OP
+ }
+
+ @Override
+ ByteBuffer getInBuffer() {
+ return null;
+ }
+
+ @Override
+ void onDataAvailable() throws IOException {
+ // NO-OP
+ }
+
+ @Override
+ boolean isReadyForRead() {
+ return true;
+ }
+
+ @Override
+ boolean isRequestBodyFullyRead() {
+ return inputFilter.isFinished();
+ }
+
+ @Override
+ void insertReplayedBody(ByteChunk body) {
+ // NO-OP
+ }
+ }
}
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java
b/test/org/apache/coyote/http2/Http2TestBase.java
index 6073ecbe87..acf2d91b37 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -20,6 +20,7 @@ import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
+import java.io.InputStreamReader;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.Socket;
@@ -1381,20 +1382,44 @@ public abstract class Http2TestBase extends
TomcatBaseTest {
protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
// Request bodies are unusal with GET but not illegal
+ doPost(req, resp);
+ }
+
+ @Override
+ protected void doPost(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
long total = 0;
long read = 0;
- byte[] buffer = new byte[1024];
- try (InputStream is = req.getInputStream()) {
- while ((read = is.read(buffer)) > 0) {
- total += read;
+
+ if ("true".equals(req.getParameter("useReader"))) {
+ char[] buffer = new char[1024];
+
+ try (InputStream is = req.getInputStream();
+ InputStreamReader reader = new InputStreamReader(is,
StandardCharsets.UTF_8);) {
+ while ((read = reader.read(buffer)) > 0) {
+ total += read;
+ }
}
- }
- resp.setContentType("text/plain");
- resp.setCharacterEncoding("UTF-8");
- PrintWriter pw = resp.getWriter();
- pw.print("Total bytes read from request body [" + total + "]");
+ resp.setContentType("text/plain");
+ resp.setCharacterEncoding("UTF-8");
+ PrintWriter pw = resp.getWriter();
+ pw.print("Total chars read from request body [" + total + "]");
+
+ } else {
+ byte[] buffer = new byte[1024];
+ try (InputStream is = req.getInputStream()) {
+ while ((read = is.read(buffer)) > 0) {
+ total += read;
+ }
+ }
+
+ resp.setContentType("text/plain");
+ resp.setCharacterEncoding("UTF-8");
+ PrintWriter pw = resp.getWriter();
+ pw.print("Total bytes read from request body [" + total + "]");
+ }
}
}
diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
index ebdb14ac02..e818205b2d 100644
--- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
+++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
@@ -72,18 +72,54 @@ public class TestHttp2UpgradeHandler extends Http2TestBase {
@Test
- public void testUpgradeWithRequestBody() throws Exception {
- doTestUpgradeWithRequestBody(false);
+ public void testUpgradeWithRequestBodyGet() throws Exception {
+ doTestUpgradeWithRequestBody(false, false, false);
}
@Test
- public void testUpgradeWithRequestBodyTooBig() throws Exception {
- doTestUpgradeWithRequestBody(true);
+ public void testUpgradeWithRequestBodyGetTooBig() throws Exception {
+ doTestUpgradeWithRequestBody(false, false, true);
}
- private void doTestUpgradeWithRequestBody(boolean tooBig) throws Exception
{
+ @Test
+ public void testUpgradeWithRequestBodyPost() throws Exception {
+ doTestUpgradeWithRequestBody(true, false, false);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyPostTooBig() throws Exception {
+ doTestUpgradeWithRequestBody(true, false, true);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyGetReader() throws Exception {
+ doTestUpgradeWithRequestBody(false, true, false);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyGetReaderTooBig() throws Exception {
+ doTestUpgradeWithRequestBody(false, true, true);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyPostReader() throws Exception {
+ doTestUpgradeWithRequestBody(true, true, false);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyPostReaderTooBig() throws Exception {
+ doTestUpgradeWithRequestBody(true, true, true);
+ }
+
+
+ private void doTestUpgradeWithRequestBody(boolean usePost, boolean
useReader, boolean tooBig) throws Exception {
enableHttp2();
Tomcat tomcat = getTomcatInstance();
@@ -100,7 +136,8 @@ public class TestHttp2UpgradeHandler extends Http2TestBase {
openClientConnection();
- byte[] upgradeRequest = ("GET / HTTP/1.1\r\n" +
+ byte[] upgradeRequest = ((usePost ? "POST" : "GET") +
+ " /" + (useReader ? "?useReader=true " : " ") + "HTTP/1.1\r\n"
+
"Host: localhost:" + getPort() + "\r\n" +
"Content-Length: 18\r\n" +
"Connection: Upgrade,HTTP2-Settings\r\n" +
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 9dba4aa423..d555408ca4 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -143,6 +143,10 @@
of the OS that uses kernel 5.10 or later. Thanks to Christopher Gual
for
the research into this issue. (markt)
</fix>
+ <fix>
+ <bug>66023</bug>: Improve the fix for <bug>65726</bug> and support HTTP
+ upgrade with a request body for a wider set of use cases. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
