You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Michael Osipov <mi...@apache.org> on 2019/10/09 15:58:02 UTC
Possible bugs in Http11Processor
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Michael Osipov <mi...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Michael Osipov <mi...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Mark Thomas <ma...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Michael Osipov <mi...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Mark Thomas <ma...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Mark Thomas <ma...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Possible bugs in Http11Processor
Posted by Michael Osipov <mi...@apache.org>.
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org