Re: [tomcat] branch master updated: Fix possible concurrency issue
On 15/09/2020 17:42, Mark Thomas wrote: > 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 >> 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) > 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. Well, that didn't take long. The WebSocket error occurred again after this fix so this isn't the (only?) root cause of that issue. The search for the root cause of the intermittent WebSocket failures continues... Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [tomcat] branch master updated: Fix possible concurrency issue
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 > 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 wh
[tomcat] branch master updated: Fix possible concurrency issue
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 AuthorDate: Tue Sep 15 17:34:50 2020 +0100 Fix possible concurrency issue --- 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
