On 15/09/2020 17:35, ma...@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 6c21ee5 Fix possible concurrency issue
> 6c21ee5 is described below
>
> commit 6c21ee5a57e6a6fa9c780d4a8f857d5c3a0b9a35
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Tue Sep 15 17:34:50 2020 +0100
>
> Fix possible concurrency issue
Hi,
This fix is a little on the speculative side.
I've been watching the intermittent CI failures for the last month or so
and there is evidence (mainly in failed WebSocket tests) of something
going wrong in HTTP request parsing.
Today, I saw a failure of once of the TestCoyoteAdapterRequestFuzzing
tests with the following stack trace:
15-Sep-2020 15:15:13.301 INFO [http-nio-127.0.0.1-auto-8-exec-1]
org.apache.coyote.http11.Http11Processor.service Error parsing HTTP
request header
Note: further occurrences of HTTP request parsing errors will be logged
at DEBUG level.
java.lang.IllegalArgumentException: Request header is too large
at
org.apache.coyote.http11.Http11InputBuffer.fill(Http11InputBuffer.java:777)
at
org.apache.coyote.http11.Http11InputBuffer.parseHeader(Http11InputBuffer.java:938)
at
org.apache.coyote.http11.Http11InputBuffer.parseHeaders(Http11InputBuffer.java:589)
at
org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:284)
at
org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
at
org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:868)
at
org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1590)
at
org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at
org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
at java.lang.Thread.run(Thread.java:748)
Given the request, that should not be possible. The only explanation I
can come up with is that the recycling of the InputBuffer was not fully
visible to the http-nio-127.0.0.1-auto-8-exec-1 thread.
This seems a little far-fetched to me but I can't come up with a better
theory. With this theory in mind a looked at the Http11InputBuffer and
if my understanding is correct:
- there is a theoretical concurrency issue
- the patch below fixes it
I do intend to back-port this as my understanding is that the issue is
(theoretically) correct. I'll continue to keep an eye on the CI results
to see if there is any noticeable impact. That said, as I can't
reproduce this and impact is going to have be inferred.
Mark
> ---
> java/org/apache/coyote/http11/Http11InputBuffer.java | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java
> b/java/org/apache/coyote/http11/Http11InputBuffer.java
> index ab0d1c6..555e3ad 100644
> --- a/java/org/apache/coyote/http11/Http11InputBuffer.java
> +++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
> @@ -71,7 +71,7 @@ public class Http11InputBuffer implements InputBuffer,
> ApplicationBufferHandler
> /**
> * State.
> */
> - private boolean parsingHeader;
> + private volatile boolean parsingHeader;
>
>
> /**
> @@ -130,7 +130,7 @@ public class Http11InputBuffer implements InputBuffer,
> ApplicationBufferHandler
> */
> private byte prevChr = 0;
> private byte chr = 0;
> - private boolean parsingRequestLine;
> + private volatile boolean parsingRequestLine;
> private int parsingRequestLinePhase = 0;
> private boolean parsingRequestLineEol = false;
> private int parsingRequestLineStart = 0;
> @@ -266,18 +266,22 @@ public class Http11InputBuffer implements InputBuffer,
> ApplicationBufferHandler
>
> byteBuffer.limit(0).position(0);
> lastActiveFilter = -1;
> - parsingHeader = true;
> swallowInput = true;
>
> chr = 0;
> prevChr = 0;
> headerParsePos = HeaderParsePosition.HEADER_START;
> - parsingRequestLine = true;
> parsingRequestLinePhase = 0;
> parsingRequestLineEol = false;
> parsingRequestLineStart = 0;
> parsingRequestLineQPos = -1;
> headerData.recycle();
> + // Recycled last because they are volatile
> + // All variables visible to this thread are guaranteed to be visible
> to
> + // any other thread once that thread reads the same volatile. The
> first
> + // action when parsing input data is to read one of these volatiles.
> + parsingRequestLine = true;
> + parsingHeader = true;
> }
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
