[tomcat] branch master updated: Fix possible concurrency issue

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch master updated: Fix possible concurrency issue

markt
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 <[hidden email]>
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] branch master updated: Fix possible concurrency issue

markt
On 15/09/2020 17:35, [hidden email] 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 <[hidden email]>
> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [tomcat] branch master updated: Fix possible concurrency issue

markt
On 15/09/2020 17:42, Mark Thomas wrote:

> On 15/09/2020 17:35, [hidden email] 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 <[hidden email]>
>> 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)

<snip/>

> 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: [hidden email]
For additional commands, e-mail: [hidden email]