Possible bugs in Http11Processor

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Possible bugs in Http11Processor

Michael Osipov
Folks,

while working on an improvement for Http11Processor I have noticed there
constructs:

> if ((contentEncodingMB != null)
>    (contentEncodingMB.indexOf("gzip") != -1))


> if (connectionValue != null)
>     foundUpgrade = connectionValue.toLowerCase(Locale.ENGLISH).contains("upgrade");

> if (findBytes(connectionValueBC, Constants.CLOSE_BYTES) != -1) {
>     keepAlive = false;
> } else if (findBytes(connectionValueBC,
>     Constants.KEEPALIVE_BYTES) != -1) {

and on likely other spots. I believe they are wrong. A simple curl test:

> $ curl http://md11gxtc:8080/ -I --verbose   -H "Connection: close2"
> * Uses proxy env variable NO_PROXY == 'localhost .siemens.net .siemens.com .siemens.de'
> *   Trying 147.54.67.8:8080...
> * TCP_NODELAY set
> * Connected to md11gxtc (147.54.67.8) port 8080 (#0)
>> HEAD / HTTP/1.1
>> Host: md11gxtc:8080
>> User-Agent: curl/7.66.0
>> Accept: */*
>> Connection: close2
>>
> * Mark bundle as not supporting multiuse
> < HTTP/1.1 404
> HTTP/1.1 404
> < Content-Type: text/html;charset=utf-8
> Content-Type: text/html;charset=utf-8
> < Content-Language: en
> Content-Language: en
> < Transfer-Encoding: chunked
> Transfer-Encoding: chunked
> < Date: Wed, 09 Oct 2019 15:54:49 GMT
> Date: Wed, 09 Oct 2019 15:54:49 GMT
> < Connection: close
> Connection: close
>
> <
> * Closing connection 0

I don't expect the server to respond with "Connection: close" when I
send a non-sense value "close2".

Here w/o the header:

> $ curl http://md11gxtc:8080/ -I --verbose
> * Uses proxy env variable NO_PROXY == 'localhost .siemens.net .siemens.com .siemens.de'
> *   Trying 147.54.67.8:8080...
> * TCP_NODELAY set
> * Connected to md11gxtc (147.54.67.8) port 8080 (#0)
>> HEAD / HTTP/1.1
>> Host: md11gxtc:8080
>> User-Agent: curl/7.66.0
>> Accept: */*
>>
> * Mark bundle as not supporting multiuse
> < HTTP/1.1 404
> HTTP/1.1 404
> < Content-Type: text/html;charset=utf-8
> Content-Type: text/html;charset=utf-8
> < Content-Language: en
> Content-Language: en
> < Transfer-Encoding: chunked
> Transfer-Encoding: chunked
> < Date: Wed, 09 Oct 2019 15:56:31 GMT
> Date: Wed, 09 Oct 2019 15:56:31 GMT
>
> <
> * Connection #0 to host md11gxtc left intact

Tests performed with 8.5.x/4a9f854a67bc5cece8fa83278ac5449c4b1f54d9.

WDYT?

Michael

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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

Michael Osipov
Just found another bug:
>     private static boolean isConnectionClose(MimeHeaders headers) {
>         MessageBytes connection = headers.getValue(Constants.CONNECTION);
>         if (connection == null) {
>             return false;
>         }
>         return connection.equals(Constants.CLOSE);
>     }


RFC 7230, section 6.1. says:
> Connection options are case-insensitive.

Michael

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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

markt
In reply to this post by Michael Osipov
On 09/10/2019 16:58, Michael Osipov wrote:
> Folks,
>
> while working on an improvement for Http11Processor I have noticed there
> constructs:
>
>> if ((contentEncodingMB != null)
>>    (contentEncodingMB.indexOf("gzip") != -1))

The parsing of this is much tighter on input. For output I think this is
a reasonable trade-off. The worst that will happen is that Tomcat won't
compress something it might have been able to.

>> if (connectionValue != null)
>>     foundUpgrade =
>> connectionValue.toLowerCase(Locale.ENGLISH).contains("upgrade");
>
>> if (findBytes(connectionValueBC, Constants.CLOSE_BYTES) != -1) {
>>     keepAlive = false;
>> } else if (findBytes(connectionValueBC,
>>     Constants.KEEPALIVE_BYTES) != -1) {
>
> and on likely other spots. I believe they are wrong.

Yes, but. Is the cost of parsing that header (and any similar headers)
fully worth the benefit? The header parser is reasonably efficient so it
might be OK.

I'd suggest creating a BZ issue for this.

Mark


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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

markt
In reply to this post by Michael Osipov


On 09/10/2019 17:49, Michael Osipov wrote:

> Just found another bug:
>>     private static boolean isConnectionClose(MimeHeaders headers) {
>>         MessageBytes connection = headers.getValue(Constants.CONNECTION);
>>         if (connection == null) {
>>             return false;
>>         }
>>         return connection.equals(Constants.CLOSE);
>>     }
>
>
> RFC 7230, section 6.1. says:
>> Connection options are case-insensitive.

-> bugzilla.

Mark


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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

Michael Osipov
In reply to this post by markt
Am 2019-10-09 um 19:08 schrieb Mark Thomas:

> On 09/10/2019 16:58, Michael Osipov wrote:
>> Folks,
>>
>> while working on an improvement for Http11Processor I have noticed there
>> constructs:
>>
>>> if ((contentEncodingMB != null)
>>>     (contentEncodingMB.indexOf("gzip") != -1))
>
> The parsing of this is much tighter on input. For output I think this is
> a reasonable trade-off. The worst that will happen is that Tomcat won't
> compress something it might have been able to.

I agree on output, but there is at least on counter example:

>         // Check if browser support gzip encoding
>         MessageBytes acceptEncodingMB =
>             request.getMimeHeaders().getValue("accept-encoding");
>
>         if ((acceptEncodingMB == null)
>             || (acceptEncodingMB.indexOf("gzip") == -1)) {
>             return false;
>         }

It would even accept "Accept-Encoding: figzip" as valid input.

>>> if (findBytes(connectionValueBC, Constants.CLOSE_BYTES) != -1) {
>>>      keepAlive = false;
>>> } else if (findBytes(connectionValueBC,
>>>      Constants.KEEPALIVE_BYTES) != -1) {

In this specific case I said via curl: "Connection: close2" and it still
matched.

>> and on likely other spots. I believe they are wrong.
>
> Yes, but. Is the cost of parsing that header (and any similar headers)
> fully worth the benefit? The header parser is reasonably efficient so it
> might be OK.
>
> I'd suggest creating a BZ issue for this.

Will do!


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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

markt
On 09/10/2019 22:03, Michael Osipov wrote:

> Am 2019-10-09 um 19:08 schrieb Mark Thomas:
>> On 09/10/2019 16:58, Michael Osipov wrote:
>>> Folks,
>>>
>>> while working on an improvement for Http11Processor I have noticed there
>>> constructs:
>>>
>>>> if ((contentEncodingMB != null)
>>>>     (contentEncodingMB.indexOf("gzip") != -1))
>>
>> The parsing of this is much tighter on input. For output I think this is
>> a reasonable trade-off. The worst that will happen is that Tomcat won't
>> compress something it might have been able to.
>
> I agree on output, but there is at least on counter example:
>
>>         // Check if browser support gzip encoding
>>         MessageBytes acceptEncodingMB =
>>             request.getMimeHeaders().getValue("accept-encoding");
>>
>>         if ((acceptEncodingMB == null)
>>             || (acceptEncodingMB.indexOf("gzip") == -1)) {
>>             return false;
>>         }
>
> It would even accept "Accept-Encoding: figzip" as valid input.

That isn't there in 9.0.x. I wonder why the new CompressionConfig class
isn't being used in 8.5.x (that parses this correctly). I have a vague
recollection of backwards compatibility issues with the back-port but it
is worth revisiting that.

Mark


>
>>>> if (findBytes(connectionValueBC, Constants.CLOSE_BYTES) != -1) {
>>>>      keepAlive = false;
>>>> } else if (findBytes(connectionValueBC,
>>>>      Constants.KEEPALIVE_BYTES) != -1) {
>
> In this specific case I said via curl: "Connection: close2" and it still
> matched.
>
>>> and on likely other spots. I believe they are wrong.
>>
>> Yes, but. Is the cost of parsing that header (and any similar headers)
>> fully worth the benefit? The header parser is reasonably efficient so it
>> might be OK.
>>
>> I'd suggest creating a BZ issue for this.
>
> Will do!
>
>
> ---------------------------------------------------------------------
> 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: Possible bugs in Http11Processor

Michael Osipov
Am 2019-10-09 um 23:23 schrieb Mark Thomas:

> On 09/10/2019 22:03, Michael Osipov wrote:
>> Am 2019-10-09 um 19:08 schrieb Mark Thomas:
>>> On 09/10/2019 16:58, Michael Osipov wrote:
>>>> Folks,
>>>>
>>>> while working on an improvement for Http11Processor I have noticed there
>>>> constructs:
>>>>
>>>>> if ((contentEncodingMB != null)
>>>>>      (contentEncodingMB.indexOf("gzip") != -1))
>>>
>>> The parsing of this is much tighter on input. For output I think this is
>>> a reasonable trade-off. The worst that will happen is that Tomcat won't
>>> compress something it might have been able to.
>>
>> I agree on output, but there is at least on counter example:
>>
>>>          // Check if browser support gzip encoding
>>>          MessageBytes acceptEncodingMB =
>>>              request.getMimeHeaders().getValue("accept-encoding");
>>>
>>>          if ((acceptEncodingMB == null)
>>>              || (acceptEncodingMB.indexOf("gzip") == -1)) {
>>>              return false;
>>>          }
>>
>> It would even accept "Accept-Encoding: figzip" as valid input.
>
> That isn't there in 9.0.x. I wonder why the new CompressionConfig class
> isn't being used in 8.5.x (that parses this correctly). I have a vague
> recollection of backwards compatibility issues with the back-port but it
> is worth revisiting that.

Checked CompressionConfig in master:

> if (contentEncodingMB != null &&
>                 (contentEncodingMB.indexOf("gzip") != -1 ||
>                         contentEncodingMB.indexOf("br") != -1)) {
>             return false;
>         }

>         if ((acceptEncodingMB == null) || (acceptEncodingMB.indexOf("gzip") == -1)) {
>             return false;
>         }

don't look any better, do they?

Issue?


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

Reply | Threaded
Open this post in threaded view
|

Re: Possible bugs in Http11Processor

Michael Osipov
Am 2019-10-09 um 23:46 schrieb Michael Osipov:

> Am 2019-10-09 um 23:23 schrieb Mark Thomas:
>> On 09/10/2019 22:03, Michael Osipov wrote:
>>> Am 2019-10-09 um 19:08 schrieb Mark Thomas:
>>>> On 09/10/2019 16:58, Michael Osipov wrote:
>>>>> Folks,
>>>>>
>>>>> while working on an improvement for Http11Processor I have noticed
>>>>> there
>>>>> constructs:
>>>>>
>>>>>> if ((contentEncodingMB != null)
>>>>>>      (contentEncodingMB.indexOf("gzip") != -1))
>>>>
>>>> The parsing of this is much tighter on input. For output I think
>>>> this is
>>>> a reasonable trade-off. The worst that will happen is that Tomcat won't
>>>> compress something it might have been able to.
>>>
>>> I agree on output, but there is at least on counter example:
>>>
>>>>          // Check if browser support gzip encoding
>>>>          MessageBytes acceptEncodingMB =
>>>>              request.getMimeHeaders().getValue("accept-encoding");
>>>>
>>>>          if ((acceptEncodingMB == null)
>>>>              || (acceptEncodingMB.indexOf("gzip") == -1)) {
>>>>              return false;
>>>>          }
>>>
>>> It would even accept "Accept-Encoding: figzip" as valid input.
>>
>> That isn't there in 9.0.x. I wonder why the new CompressionConfig class
>> isn't being used in 8.5.x (that parses this correctly). I have a vague
>> recollection of backwards compatibility issues with the back-port but it
>> is worth revisiting that.
>
> Checked CompressionConfig in master:
>
>> if (contentEncodingMB != null &&
>>                 (contentEncodingMB.indexOf("gzip") != -1 ||
>>                         contentEncodingMB.indexOf("br") != -1)) {
>>             return false;
>>         }
>
>>         if ((acceptEncodingMB == null) ||
>> (acceptEncodingMB.indexOf("gzip") == -1)) {
>>             return false;
>>         }
>
> don't look any better, do they?
>
> Issue?

I guess, I need to spawn an issue anyway because of
https://tools.ietf.org/html/rfc7231#section-3.1.2.1:
"All content-coding values are case-insensitive ..."



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