Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c

[William A Rowe Jr]

On Fri, Nov 20, 2015 at 3:13 AM, Bert Huijben  wrote:

>
> > -Original Message-
> > From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> > Sent: vrijdag 20 november 2015 10:11
> > To: dev@httpd.apache.org
> > Subject: Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
> >
> > +1 for lowercasing and pls backport, since it just arrived as is in
> 2.4.x,
> i think.
>
> > > -apr_table_setn(r->headers_out, "Connection",
> "upgrade");
> > > +apr_table_setn(r->headers_out, "Connection",
> "Upgrade");
>
> As the 'Connection' header technically refers to the 'Upgrade' header I
> would paint both sheds in the same color.


It does, yes,  That's part of my motivation, the Key (field title) is
generally
capitalized, the values are generally lowercase, and in this case upgrade
does appear twice, so it helps slightly to distinguish the two.

Referring  back it has appeared both ways in the specs, but RFC 7230
has buried RFC 2817 :)

http://tools.ietf.org/html/rfc7230#page-57

My inclination is obvious but I don't feel strongly enough to patch this
myself, will let the others do so.  For reference, we actually followed
the RFC 2817 conventions in 2.2...

ssl_engine_io.c:#define CONNECTION_HEADER "Connection: Upgrade"
ssl_engine_kernel.c:apr_table_setn(r->err_headers_out,
"Connection", "Upgrade");

and it now appears both ways in the code.  /shrug

ssl_engine_kernel.c:#define CONNECTION_HEADER "Connection: Upgrade"
ssl_engine_kernel.c:apr_table_setn(r->err_headers_out,
"Connection", "Upgrade");
ssl_engine_kernel.c: * connection, and this isn't a subrequest, send an
Upgrade
ssl_engine_kernel.c:apr_table_mergen(r->headers_out, "Connection",
"upgrade");

Re: H2 stream dependencies

[Stefan Eissing]

Bert,

interesting and nice to see the progress. You probably could use priorities for 
ordering, especially when using the stream dependencies. I am not certain, 
however, if this will give you totally deterministic behavior. If stream B 
depends on A, usually A will be sent out before B. However, should stream A 
become suspended - e.g. unable to progress, either because there is no data 
available which some other thread needs to produce, or because the flow control 
window has not been updated in time - the server will start sending data for B.

The state of the implementation is:
2.4.17: fully implemented priority handling on sending stream data, as 
implemented by the nghttp2 library
2.4.18: additionally, streams are scheduled for execution by priority. Makes a 
difference when the number of available worker is less than the stream queue 
length, e.g. when der server is under load or only few workers are available 
(in prefork, for certain).

Another way to influence the ordering is for the client to play with the stream 
window sizes. If you give stream A 2^31-1 and stream B 0 until you have A, then 
update B, it may also do what you want. If both streams produce response 
bodies...flow only applies to DATA...

Cheers,

  Stefan

> Am 20.11.2015 um 00:17 schrieb Bert Huijben :
> 
> Hi All (and Stefan in particular),
>  
> As already noted I’m trying to make Subversion work over http/2 via the 
> Apache Serf library. Today I made a few huge steps forward and got most of 
> the Subversion tests working over h2. (Just +- 60 failures left of the +- 
> 2000 tests)
>  
> One particular problem that remains is that we have some code that assumes 
> some related responses are delivered in strict order. This code worked for 
> many releases, so there is not much I can change on the information needed, 
> without adding features that only work in future Subversion versions.
>  
> In theory I should be able to get exactly the behavior I want by adding 
> priority information to my h2 requests… but I don’t know if httpd is ready to 
> process that information in a way that would help us.
> (Currently I just ignore all this in my implementation… as will at least some 
> other implementations)
>  
> Can somebody give me an update on what the current status of priority 
> handling in httpd is? (trunk vs 2.4.17)
>  
> Thanks,
> Bert

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Stefan Eissing]

Yes, but I interpret that as the header fields, not any tokens that refer to 
header names. 

> Am 20.11.2015 um 17:04 schrieb Jim Jagielski :
> 
> Ugg... I *just* noticed:
> 
>However, header field names MUST be converted to lowercase
>prior to their encoding in HTTP/2. A request or response
>containing uppercase header field names MUST be treated as
>malformed 
> 
> 
> 
>> On Nov 20, 2015, at 10:44 AM, Stefan Eissing  
>> wrote:
>> 
>> Most of this is discussed here: 
>> https://httpwg.github.io/specs/rfc7540.html#HttpHeaders
>> 
>> Basically HTTP/2 defines its own connection properties, so several things 
>> which are announced/controlled
>> by Connection: and other headers do not apply to HTTP/2 connections. So, the 
>> "Connection:" header itself
>> was obsoleted.
>> 
>> mod_http2 has to translate between both worlds a bit and is doing so 
>> probably incompletely so now, so
>> the discussion about this is very useful.
>> 
>> There are two conversions for mod_http2 to make:
>> 1. request headers (H2 -> H1): 
>> 2. response headers (H1 -> H2):
>> 
>> For 1. certain headers should not arrive at all. If they do, we can either 
>> ignore or generate an error and deny the request. For example "Connection: 
>> ..." should never arrive. Same for "Transfer-Encoding: ". The current 
>> implementation ignores. One could argue for deny.
>> For 2. certain headers we cannot send out and we need to take proper actions 
>> for that. For example a "TE: deflate" we cannot process like this. Here more 
>> checks need to be added.
>> 
>> //Stefan
>> 
>>> Am 20.11.2015 um 16:25 schrieb Yann Ylavic :
>>> 
>>> On Fri, Nov 20, 2015 at 2:58 PM,   wrote:
 Author: icing
 Date: Fri Nov 20 13:58:32 2015
 New Revision: 1715363
 
 URL: http://svn.apache.org/viewvc?rev=1715363=rev
 Log:
 incoming trailers passed into chunked request bodies, outgoing trailers 
 not supported yet
 
 Modified:
>>> []
  httpd/httpd/trunk/modules/http2/h2_util.c
>>> []
 
 
 Modified: httpd/httpd/trunk/modules/http2/h2_util.c
 URL: 
 http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1715363=1715362=1715363=diff
 ==
 --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
 +++ httpd/httpd/trunk/modules/http2/h2_util.c Fri Nov 20 13:58:32 2015
>>> []
 @@ -879,3 +894,94 @@ h2_ngheader *h2_util_ngheader_make_req(a
   return ngh;
 }
 
 +/***
 + * header HTTP/1 <-> HTTP/2 conversions
 + 
 **/
 +
 +
 +typedef struct {
 +const char *name;
 +size_t len;
 +} literal;
 +
 +#define H2_DEF_LITERAL(n)   { (n), (sizeof(n)-1) }
 +#define H2_ALEN(a)  (sizeof(a)/sizeof((a)[0]))
 +#define H2_LIT_ARGS(a)  (a),H2_ALEN(a)
 +
 +static literal IgnoredRequestHeaders[] = {
 +H2_DEF_LITERAL("host"),
 +H2_DEF_LITERAL("expect"),
 +H2_DEF_LITERAL("upgrade"),
 +H2_DEF_LITERAL("connection"),
 +H2_DEF_LITERAL("keep-alive"),
 +H2_DEF_LITERAL("http2-settings"),
 +H2_DEF_LITERAL("proxy-connection"),
 +H2_DEF_LITERAL("transfer-encoding"),
 +};
>>> 
>>> Shouldn't we also include all the tokens from the Connection header?
>>> (obviously not feasible with this only blacklist)
>>> 
>>> 
 +static literal IgnoredRequestTrailers[] = { /* Ignore, see rfc7230, ch. 
 4.1.2 */
 +H2_DEF_LITERAL("te"),
 +H2_DEF_LITERAL("host"),
 +H2_DEF_LITERAL("range"),
 +H2_DEF_LITERAL("cookie"),
 +H2_DEF_LITERAL("expect"),
 +H2_DEF_LITERAL("pragma"),
 +H2_DEF_LITERAL("max-forwards"),
 +H2_DEF_LITERAL("cache-control"),
 +H2_DEF_LITERAL("authorization"),
 +H2_DEF_LITERAL("content-length"),
 +H2_DEF_LITERAL("proxy-authorization"),
 +};
>>> 
>>> No notion of announced trailers in HTTP2, with eg. the request header
>>> "Trailer: token1, token2, ..." (any non-announced trailer would be
>>> ignored), something we could (always/optionally) enforce?
>>> 
 +static literal IgnoredResponseTrailers[] = {
 +H2_DEF_LITERAL("age"),
 +H2_DEF_LITERAL("date"),
 +H2_DEF_LITERAL("vary"),
 +H2_DEF_LITERAL("cookie"),
 +H2_DEF_LITERAL("expires"),
 +H2_DEF_LITERAL("warning"),
 +H2_DEF_LITERAL("location"),
 +H2_DEF_LITERAL("retry-after"),
 +H2_DEF_LITERAL("cache-control"),
 +H2_DEF_LITERAL("www-authenticate"),
 +H2_DEF_LITERAL("proxy-authenticate"),
 +};
>>> 
>>> Likewise, shouldn't we check whether the request associated to the
>>> response contains a "TE: 

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Yann Ylavic]

On Fri, Nov 20, 2015 at 5:04 PM, Jim Jagielski  wrote:
> Ugg... I *just* noticed:
>
> However, header field names MUST be converted to lowercase
> prior to their encoding in HTTP/2. A request or response
> containing uppercase header field names MUST be treated as
> malformed

Gain some cycles (client side), to the detriment of existing apps...
This won't help H1->H2, at least.

Re: svn commit: r1715375 - /httpd/httpd/trunk/CHANGES

[Yann Ylavic]

On Fri, Nov 20, 2015 at 4:24 PM,   wrote:
> Author: icing
> Date: Fri Nov 20 15:24:21 2015
> New Revision: 1715375
>
> URL: http://svn.apache.org/viewvc?rev=1715375=rev
> Log:
> updated CHANGES
>
> Modified:
> httpd/httpd/trunk/CHANGES
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1715375=1715374=1715375=diff
> ==
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Nov 20 15:24:21 2015
> @@ -4,6 +4,24 @@ Changes with Apache 2.5.0
>*) mod_ssl: For the "SSLStaplingReturnResponderErrors off" case, make sure
>   to only staple responses with certificate status "good". [Kaspar Brand]
>
> +  *) mod_http2: incoming trailers (headers after request body) are properly
> + forwarded to the processing engine. [Stefan Eissing]
> +
> +  *) mod_http2: new directive 'H2Push' to en-/disable HTTP/2 server
> + pushes a server/virtual host. Pushes are initiated by the presence
> + of 'Link:' headers with relation 'preload' on a response. [Stefan 
> Eissing]
> +
> +  *) mod_http2: write performance of http2 improved for larger resources,
> + especially static files. [Stefan Eissing]
> +
> +  *) core: if the first HTTP/1.1 request on a connection goes to a server 
> that
> + prefers different protocols, these protocols are announced in a Upgrade:
> + header on the response, mentioning the preferred protocols.
> + [Stefan Eissing]
> +
> +  *) mod_http2: new directive 'H2ModernTLSOnly' to enforce security
> + requirements of RFC 7540 on TLS connections. [Stefan Eissing]
> +

This should rather be removed from trunk/CHANGES once backported to
2.4.x/CHANGES (and added when committed to trunk ;).


>*) mod_http2: new directives 'H2TLSWarmUpSize' and 'H2TLSCoolDownSecs'
>   to control TLS record sizes during connection lifetime.
>
> @@ -12,8 +30,7 @@ Changes with Apache 2.5.0
>
>*) core: add ap_get_protocol_upgrades() to retrieve the list of protocols
>   that a client could possibly upgrade to. Use in first request on a
> - connection to announce protocol choices.
> - [Stefan Eissing]
> + connection to announce protocol choices. [Stefan Eissing]

Likewise.

>
>*) mod_http2: reworked deallocation on connection shutdown and worker
>   abort. Separate parent pool for all workers. worker threads are joined
> @@ -29,8 +46,7 @@ Changes with Apache 2.5.0
>   Detailed examination of renegotiation is only done when these do not
>   match.
>   Renegotiation is 403ed when a master connection is present. Exact reason
> - is given additionally in a request note.
> - [Stefan Eissing]
> + is given additionally in a request note. [Stefan Eissing]

Likewise.

Thanks,
Yann.

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Stefan Eissing]

Most of this is discussed here: 
https://httpwg.github.io/specs/rfc7540.html#HttpHeaders

Basically HTTP/2 defines its own connection properties, so several things which 
are announced/controlled
by Connection: and other headers do not apply to HTTP/2 connections. So, the 
"Connection:" header itself
was obsoleted.

mod_http2 has to translate between both worlds a bit and is doing so probably 
incompletely so now, so
the discussion about this is very useful.

There are two conversions for mod_http2 to make:
1. request headers (H2 -> H1): 
2. response headers (H1 -> H2):

For 1. certain headers should not arrive at all. If they do, we can either 
ignore or generate an error and deny the request. For example "Connection: ..." 
should never arrive. Same for "Transfer-Encoding: ". The current implementation 
ignores. One could argue for deny.
For 2. certain headers we cannot send out and we need to take proper actions 
for that. For example a "TE: deflate" we cannot process like this. Here more 
checks need to be added.

//Stefan

> Am 20.11.2015 um 16:25 schrieb Yann Ylavic :
> 
> On Fri, Nov 20, 2015 at 2:58 PM,   wrote:
>> Author: icing
>> Date: Fri Nov 20 13:58:32 2015
>> New Revision: 1715363
>> 
>> URL: http://svn.apache.org/viewvc?rev=1715363=rev
>> Log:
>> incoming trailers passed into chunked request bodies, outgoing trailers not 
>> supported yet
>> 
>> Modified:
> []
>>httpd/httpd/trunk/modules/http2/h2_util.c
> []
>> 
>> 
>> Modified: httpd/httpd/trunk/modules/http2/h2_util.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1715363=1715362=1715363=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_util.c Fri Nov 20 13:58:32 2015
> []
>> @@ -879,3 +894,94 @@ h2_ngheader *h2_util_ngheader_make_req(a
>> return ngh;
>> }
>> 
>> +/***
>> + * header HTTP/1 <-> HTTP/2 conversions
>> + 
>> **/
>> +
>> +
>> +typedef struct {
>> +const char *name;
>> +size_t len;
>> +} literal;
>> +
>> +#define H2_DEF_LITERAL(n)   { (n), (sizeof(n)-1) }
>> +#define H2_ALEN(a)  (sizeof(a)/sizeof((a)[0]))
>> +#define H2_LIT_ARGS(a)  (a),H2_ALEN(a)
>> +
>> +static literal IgnoredRequestHeaders[] = {
>> +H2_DEF_LITERAL("host"),
>> +H2_DEF_LITERAL("expect"),
>> +H2_DEF_LITERAL("upgrade"),
>> +H2_DEF_LITERAL("connection"),
>> +H2_DEF_LITERAL("keep-alive"),
>> +H2_DEF_LITERAL("http2-settings"),
>> +H2_DEF_LITERAL("proxy-connection"),
>> +H2_DEF_LITERAL("transfer-encoding"),
>> +};
> 
> Shouldn't we also include all the tokens from the Connection header?
> (obviously not feasible with this only blacklist)
> 
> 
>> +static literal IgnoredRequestTrailers[] = { /* Ignore, see rfc7230, ch. 
>> 4.1.2 */
>> +H2_DEF_LITERAL("te"),
>> +H2_DEF_LITERAL("host"),
>> +H2_DEF_LITERAL("range"),
>> +H2_DEF_LITERAL("cookie"),
>> +H2_DEF_LITERAL("expect"),
>> +H2_DEF_LITERAL("pragma"),
>> +H2_DEF_LITERAL("max-forwards"),
>> +H2_DEF_LITERAL("cache-control"),
>> +H2_DEF_LITERAL("authorization"),
>> +H2_DEF_LITERAL("content-length"),
>> +H2_DEF_LITERAL("proxy-authorization"),
>> +};
> 
> No notion of announced trailers in HTTP2, with eg. the request header
> "Trailer: token1, token2, ..." (any non-announced trailer would be
> ignored), something we could (always/optionally) enforce?
> 
>> +static literal IgnoredResponseTrailers[] = {
>> +H2_DEF_LITERAL("age"),
>> +H2_DEF_LITERAL("date"),
>> +H2_DEF_LITERAL("vary"),
>> +H2_DEF_LITERAL("cookie"),
>> +H2_DEF_LITERAL("expires"),
>> +H2_DEF_LITERAL("warning"),
>> +H2_DEF_LITERAL("location"),
>> +H2_DEF_LITERAL("retry-after"),
>> +H2_DEF_LITERAL("cache-control"),
>> +H2_DEF_LITERAL("www-authenticate"),
>> +H2_DEF_LITERAL("proxy-authenticate"),
>> +};
> 
> Likewise, shouldn't we check whether the request associated to the
> response contains a "TE: trailers" or otherwise use no trailer?
> 
> 
> I'm a bit confused about what is included-in/overwritten-by the HTTP2
> framing and what remains from HTTP1 RFCs/requirements.
> 
> 
> Regards,
> Yann.

Re: svn commit: r1715375 - /httpd/httpd/trunk/CHANGES

[Stefan Eissing]

Updated. 

Sorry, misunderstood the "delta" intention of the file.

> Am 20.11.2015 um 16:38 schrieb Yann Ylavic :
> 
> On Fri, Nov 20, 2015 at 4:24 PM,   wrote:
>> Author: icing
>> Date: Fri Nov 20 15:24:21 2015
>> New Revision: 1715375
>> 
>> URL: http://svn.apache.org/viewvc?rev=1715375=rev
>> Log:
>> updated CHANGES
>> 
>> Modified:
>>httpd/httpd/trunk/CHANGES
>> 
>> Modified: httpd/httpd/trunk/CHANGES
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1715375=1715374=1715375=diff
>> ==
>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Nov 20 15:24:21 2015
>> @@ -4,6 +4,24 @@ Changes with Apache 2.5.0
>>   *) mod_ssl: For the "SSLStaplingReturnResponderErrors off" case, make sure
>>  to only staple responses with certificate status "good". [Kaspar Brand]
>> 
>> +  *) mod_http2: incoming trailers (headers after request body) are properly
>> + forwarded to the processing engine. [Stefan Eissing]
>> +
>> +  *) mod_http2: new directive 'H2Push' to en-/disable HTTP/2 server
>> + pushes a server/virtual host. Pushes are initiated by the presence
>> + of 'Link:' headers with relation 'preload' on a response. [Stefan 
>> Eissing]
>> +
>> +  *) mod_http2: write performance of http2 improved for larger resources,
>> + especially static files. [Stefan Eissing]
>> +
>> +  *) core: if the first HTTP/1.1 request on a connection goes to a server 
>> that
>> + prefers different protocols, these protocols are announced in a 
>> Upgrade:
>> + header on the response, mentioning the preferred protocols.
>> + [Stefan Eissing]
>> +
>> +  *) mod_http2: new directive 'H2ModernTLSOnly' to enforce security
>> + requirements of RFC 7540 on TLS connections. [Stefan Eissing]
>> +
> 
> This should rather be removed from trunk/CHANGES once backported to
> 2.4.x/CHANGES (and added when committed to trunk ;).
> 
> 
>>   *) mod_http2: new directives 'H2TLSWarmUpSize' and 'H2TLSCoolDownSecs'
>>  to control TLS record sizes during connection lifetime.
>> 
>> @@ -12,8 +30,7 @@ Changes with Apache 2.5.0
>> 
>>   *) core: add ap_get_protocol_upgrades() to retrieve the list of protocols
>>  that a client could possibly upgrade to. Use in first request on a
>> - connection to announce protocol choices.
>> - [Stefan Eissing]
>> + connection to announce protocol choices. [Stefan Eissing]
> 
> Likewise.
> 
>> 
>>   *) mod_http2: reworked deallocation on connection shutdown and worker
>>  abort. Separate parent pool for all workers. worker threads are joined
>> @@ -29,8 +46,7 @@ Changes with Apache 2.5.0
>>  Detailed examination of renegotiation is only done when these do not
>>  match.
>>  Renegotiation is 403ed when a master connection is present. Exact reason
>> - is given additionally in a request note.
>> - [Stefan Eissing]
>> + is given additionally in a request note. [Stefan Eissing]
> 
> Likewise.
> 
> Thanks,
> Yann.

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Jim Jagielski]

Ugg... I *just* noticed:

However, header field names MUST be converted to lowercase
prior to their encoding in HTTP/2. A request or response
containing uppercase header field names MUST be treated as
malformed 



> On Nov 20, 2015, at 10:44 AM, Stefan Eissing  
> wrote:
> 
> Most of this is discussed here: 
> https://httpwg.github.io/specs/rfc7540.html#HttpHeaders
> 
> Basically HTTP/2 defines its own connection properties, so several things 
> which are announced/controlled
> by Connection: and other headers do not apply to HTTP/2 connections. So, the 
> "Connection:" header itself
> was obsoleted.
> 
> mod_http2 has to translate between both worlds a bit and is doing so probably 
> incompletely so now, so
> the discussion about this is very useful.
> 
> There are two conversions for mod_http2 to make:
> 1. request headers (H2 -> H1): 
> 2. response headers (H1 -> H2):
> 
> For 1. certain headers should not arrive at all. If they do, we can either 
> ignore or generate an error and deny the request. For example "Connection: 
> ..." should never arrive. Same for "Transfer-Encoding: ". The current 
> implementation ignores. One could argue for deny.
> For 2. certain headers we cannot send out and we need to take proper actions 
> for that. For example a "TE: deflate" we cannot process like this. Here more 
> checks need to be added.
> 
> //Stefan
> 
>> Am 20.11.2015 um 16:25 schrieb Yann Ylavic :
>> 
>> On Fri, Nov 20, 2015 at 2:58 PM,   wrote:
>>> Author: icing
>>> Date: Fri Nov 20 13:58:32 2015
>>> New Revision: 1715363
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1715363=rev
>>> Log:
>>> incoming trailers passed into chunked request bodies, outgoing trailers not 
>>> supported yet
>>> 
>>> Modified:
>> []
>>>   httpd/httpd/trunk/modules/http2/h2_util.c
>> []
>>> 
>>> 
>>> Modified: httpd/httpd/trunk/modules/http2/h2_util.c
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1715363=1715362=1715363=diff
>>> ==
>>> --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_util.c Fri Nov 20 13:58:32 2015
>> []
>>> @@ -879,3 +894,94 @@ h2_ngheader *h2_util_ngheader_make_req(a
>>>return ngh;
>>> }
>>> 
>>> +/***
>>> + * header HTTP/1 <-> HTTP/2 conversions
>>> + 
>>> **/
>>> +
>>> +
>>> +typedef struct {
>>> +const char *name;
>>> +size_t len;
>>> +} literal;
>>> +
>>> +#define H2_DEF_LITERAL(n)   { (n), (sizeof(n)-1) }
>>> +#define H2_ALEN(a)  (sizeof(a)/sizeof((a)[0]))
>>> +#define H2_LIT_ARGS(a)  (a),H2_ALEN(a)
>>> +
>>> +static literal IgnoredRequestHeaders[] = {
>>> +H2_DEF_LITERAL("host"),
>>> +H2_DEF_LITERAL("expect"),
>>> +H2_DEF_LITERAL("upgrade"),
>>> +H2_DEF_LITERAL("connection"),
>>> +H2_DEF_LITERAL("keep-alive"),
>>> +H2_DEF_LITERAL("http2-settings"),
>>> +H2_DEF_LITERAL("proxy-connection"),
>>> +H2_DEF_LITERAL("transfer-encoding"),
>>> +};
>> 
>> Shouldn't we also include all the tokens from the Connection header?
>> (obviously not feasible with this only blacklist)
>> 
>> 
>>> +static literal IgnoredRequestTrailers[] = { /* Ignore, see rfc7230, ch. 
>>> 4.1.2 */
>>> +H2_DEF_LITERAL("te"),
>>> +H2_DEF_LITERAL("host"),
>>> +H2_DEF_LITERAL("range"),
>>> +H2_DEF_LITERAL("cookie"),
>>> +H2_DEF_LITERAL("expect"),
>>> +H2_DEF_LITERAL("pragma"),
>>> +H2_DEF_LITERAL("max-forwards"),
>>> +H2_DEF_LITERAL("cache-control"),
>>> +H2_DEF_LITERAL("authorization"),
>>> +H2_DEF_LITERAL("content-length"),
>>> +H2_DEF_LITERAL("proxy-authorization"),
>>> +};
>> 
>> No notion of announced trailers in HTTP2, with eg. the request header
>> "Trailer: token1, token2, ..." (any non-announced trailer would be
>> ignored), something we could (always/optionally) enforce?
>> 
>>> +static literal IgnoredResponseTrailers[] = {
>>> +H2_DEF_LITERAL("age"),
>>> +H2_DEF_LITERAL("date"),
>>> +H2_DEF_LITERAL("vary"),
>>> +H2_DEF_LITERAL("cookie"),
>>> +H2_DEF_LITERAL("expires"),
>>> +H2_DEF_LITERAL("warning"),
>>> +H2_DEF_LITERAL("location"),
>>> +H2_DEF_LITERAL("retry-after"),
>>> +H2_DEF_LITERAL("cache-control"),
>>> +H2_DEF_LITERAL("www-authenticate"),
>>> +H2_DEF_LITERAL("proxy-authenticate"),
>>> +};
>> 
>> Likewise, shouldn't we check whether the request associated to the
>> response contains a "TE: trailers" or otherwise use no trailer?
>> 
>> 
>> I'm a bit confused about what is included-in/overwritten-by the HTTP2
>> framing and what remains from HTTP1 RFCs/requirements.
>> 
>> 
>> Regards,
>> Yann.
> 

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Jim Jagielski]

> On Nov 20, 2015, at 11:25 AM, Yann Ylavic  wrote:
> 
> On Fri, Nov 20, 2015 at 5:04 PM, Jim Jagielski  wrote:
>> Ugg... I *just* noticed:
>> 
>>However, header field names MUST be converted to lowercase
>>prior to their encoding in HTTP/2. A request or response
>>containing uppercase header field names MUST be treated as
>>malformed
> 
> Gain some cycles (client side), to the detriment of existing apps...
> This won't help H1->H2, at least.

Yeah... This just seemed coincidental considering the other
thread about the tokens themselves.

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Yann Ylavic]

On Fri, Nov 20, 2015 at 2:58 PM,   wrote:
> Author: icing
> Date: Fri Nov 20 13:58:32 2015
> New Revision: 1715363
>
> URL: http://svn.apache.org/viewvc?rev=1715363=rev
> Log:
> incoming trailers passed into chunked request bodies, outgoing trailers not 
> supported yet
>
> Modified:
[]
> httpd/httpd/trunk/modules/http2/h2_util.c
[]
>
>
> Modified: httpd/httpd/trunk/modules/http2/h2_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1715363=1715362=1715363=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_util.c Fri Nov 20 13:58:32 2015
[]
> @@ -879,3 +894,94 @@ h2_ngheader *h2_util_ngheader_make_req(a
>  return ngh;
>  }
>
> +/***
> + * header HTTP/1 <-> HTTP/2 conversions
> + 
> **/
> +
> +
> +typedef struct {
> +const char *name;
> +size_t len;
> +} literal;
> +
> +#define H2_DEF_LITERAL(n)   { (n), (sizeof(n)-1) }
> +#define H2_ALEN(a)  (sizeof(a)/sizeof((a)[0]))
> +#define H2_LIT_ARGS(a)  (a),H2_ALEN(a)
> +
> +static literal IgnoredRequestHeaders[] = {
> +H2_DEF_LITERAL("host"),
> +H2_DEF_LITERAL("expect"),
> +H2_DEF_LITERAL("upgrade"),
> +H2_DEF_LITERAL("connection"),
> +H2_DEF_LITERAL("keep-alive"),
> +H2_DEF_LITERAL("http2-settings"),
> +H2_DEF_LITERAL("proxy-connection"),
> +H2_DEF_LITERAL("transfer-encoding"),
> +};

Shouldn't we also include all the tokens from the Connection header?
(obviously not feasible with this only blacklist)


> +static literal IgnoredRequestTrailers[] = { /* Ignore, see rfc7230, ch. 
> 4.1.2 */
> +H2_DEF_LITERAL("te"),
> +H2_DEF_LITERAL("host"),
> +H2_DEF_LITERAL("range"),
> +H2_DEF_LITERAL("cookie"),
> +H2_DEF_LITERAL("expect"),
> +H2_DEF_LITERAL("pragma"),
> +H2_DEF_LITERAL("max-forwards"),
> +H2_DEF_LITERAL("cache-control"),
> +H2_DEF_LITERAL("authorization"),
> +H2_DEF_LITERAL("content-length"),
> +H2_DEF_LITERAL("proxy-authorization"),
> +};

No notion of announced trailers in HTTP2, with eg. the request header
"Trailer: token1, token2, ..." (any non-announced trailer would be
ignored), something we could (always/optionally) enforce?

> +static literal IgnoredResponseTrailers[] = {
> +H2_DEF_LITERAL("age"),
> +H2_DEF_LITERAL("date"),
> +H2_DEF_LITERAL("vary"),
> +H2_DEF_LITERAL("cookie"),
> +H2_DEF_LITERAL("expires"),
> +H2_DEF_LITERAL("warning"),
> +H2_DEF_LITERAL("location"),
> +H2_DEF_LITERAL("retry-after"),
> +H2_DEF_LITERAL("cache-control"),
> +H2_DEF_LITERAL("www-authenticate"),
> +H2_DEF_LITERAL("proxy-authenticate"),
> +};

Likewise, shouldn't we check whether the request associated to the
response contains a "TE: trailers" or otherwise use no trailer?


I'm a bit confused about what is included-in/overwritten-by the HTTP2
framing and what remains from HTTP1 RFCs/requirements.


Regards,
Yann.

Re: svn commit: r1715375 - /httpd/httpd/trunk/CHANGES

[Yann Ylavic]

No pb ;)
The rationale is (I think) that next httpd (2.6/3) CHANGES shouldn't
contain the ones from the previous versions.

On Fri, Nov 20, 2015 at 4:45 PM, Stefan Eissing
 wrote:
> Updated.
>
> Sorry, misunderstood the "delta" intention of the file.
>
>> Am 20.11.2015 um 16:38 schrieb Yann Ylavic :
>>
>> On Fri, Nov 20, 2015 at 4:24 PM,   wrote:
>>> Author: icing
>>> Date: Fri Nov 20 15:24:21 2015
>>> New Revision: 1715375
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1715375=rev
>>> Log:
>>> updated CHANGES
>>>
>>> Modified:
>>>httpd/httpd/trunk/CHANGES
>>>
>>> Modified: httpd/httpd/trunk/CHANGES
>>> URL: 
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1715375=1715374=1715375=diff
>>> ==
>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Fri Nov 20 15:24:21 2015
>>> @@ -4,6 +4,24 @@ Changes with Apache 2.5.0
>>>   *) mod_ssl: For the "SSLStaplingReturnResponderErrors off" case, make sure
>>>  to only staple responses with certificate status "good". [Kaspar Brand]
>>>
>>> +  *) mod_http2: incoming trailers (headers after request body) are properly
>>> + forwarded to the processing engine. [Stefan Eissing]
>>> +
>>> +  *) mod_http2: new directive 'H2Push' to en-/disable HTTP/2 server
>>> + pushes a server/virtual host. Pushes are initiated by the presence
>>> + of 'Link:' headers with relation 'preload' on a response. [Stefan 
>>> Eissing]
>>> +
>>> +  *) mod_http2: write performance of http2 improved for larger resources,
>>> + especially static files. [Stefan Eissing]
>>> +
>>> +  *) core: if the first HTTP/1.1 request on a connection goes to a server 
>>> that
>>> + prefers different protocols, these protocols are announced in a 
>>> Upgrade:
>>> + header on the response, mentioning the preferred protocols.
>>> + [Stefan Eissing]
>>> +
>>> +  *) mod_http2: new directive 'H2ModernTLSOnly' to enforce security
>>> + requirements of RFC 7540 on TLS connections. [Stefan Eissing]
>>> +
>>
>> This should rather be removed from trunk/CHANGES once backported to
>> 2.4.x/CHANGES (and added when committed to trunk ;).
>>
>>
>>>   *) mod_http2: new directives 'H2TLSWarmUpSize' and 'H2TLSCoolDownSecs'
>>>  to control TLS record sizes during connection lifetime.
>>>
>>> @@ -12,8 +30,7 @@ Changes with Apache 2.5.0
>>>
>>>   *) core: add ap_get_protocol_upgrades() to retrieve the list of protocols
>>>  that a client could possibly upgrade to. Use in first request on a
>>> - connection to announce protocol choices.
>>> - [Stefan Eissing]
>>> + connection to announce protocol choices. [Stefan Eissing]
>>
>> Likewise.
>>
>>>
>>>   *) mod_http2: reworked deallocation on connection shutdown and worker
>>>  abort. Separate parent pool for all workers. worker threads are joined
>>> @@ -29,8 +46,7 @@ Changes with Apache 2.5.0
>>>  Detailed examination of renegotiation is only done when these do not
>>>  match.
>>>  Renegotiation is 403ed when a master connection is present. Exact 
>>> reason
>>> - is given additionally in a request note.
>>> - [Stefan Eissing]
>>> + is given additionally in a request note. [Stefan Eissing]
>>
>> Likewise.
>>
>> Thanks,
>> Yann.
>

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Yann Ylavic]

Hmm, "header field names" usually refers to header names.

On Fri, Nov 20, 2015 at 5:12 PM, Stefan Eissing
 wrote:
> Yes, but I interpret that as the header fields, not any tokens that refer to 
> header names.
>
>> Am 20.11.2015 um 17:04 schrieb Jim Jagielski :
>>
>> Ugg... I *just* noticed:
>>
>>However, header field names MUST be converted to lowercase
>>prior to their encoding in HTTP/2. A request or response
>>containing uppercase header field names MUST be treated as
>>malformed
>>
>>
>>
>>> On Nov 20, 2015, at 10:44 AM, Stefan Eissing  
>>> wrote:
>>>
>>> Most of this is discussed here: 
>>> https://httpwg.github.io/specs/rfc7540.html#HttpHeaders
>>>
>>> Basically HTTP/2 defines its own connection properties, so several things 
>>> which are announced/controlled
>>> by Connection: and other headers do not apply to HTTP/2 connections. So, 
>>> the "Connection:" header itself
>>> was obsoleted.
>>>
>>> mod_http2 has to translate between both worlds a bit and is doing so 
>>> probably incompletely so now, so
>>> the discussion about this is very useful.
>>>
>>> There are two conversions for mod_http2 to make:
>>> 1. request headers (H2 -> H1):
>>> 2. response headers (H1 -> H2):
>>>
>>> For 1. certain headers should not arrive at all. If they do, we can either 
>>> ignore or generate an error and deny the request. For example "Connection: 
>>> ..." should never arrive. Same for "Transfer-Encoding: ". The current 
>>> implementation ignores. One could argue for deny.
>>> For 2. certain headers we cannot send out and we need to take proper 
>>> actions for that. For example a "TE: deflate" we cannot process like this. 
>>> Here more checks need to be added.
>>>
>>> //Stefan
>>>
 Am 20.11.2015 um 16:25 schrieb Yann Ylavic :

 On Fri, Nov 20, 2015 at 2:58 PM,   wrote:
> Author: icing
> Date: Fri Nov 20 13:58:32 2015
> New Revision: 1715363
>
> URL: http://svn.apache.org/viewvc?rev=1715363=rev
> Log:
> incoming trailers passed into chunked request bodies, outgoing trailers 
> not supported yet
>
> Modified:
 []
>  httpd/httpd/trunk/modules/http2/h2_util.c
 []
>
>
> Modified: httpd/httpd/trunk/modules/http2/h2_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_util.c?rev=1715363=1715362=1715363=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_util.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_util.c Fri Nov 20 13:58:32 2015
 []
> @@ -879,3 +894,94 @@ h2_ngheader *h2_util_ngheader_make_req(a
>   return ngh;
> }
>
> +/***
> + * header HTTP/1 <-> HTTP/2 conversions
> + 
> **/
> +
> +
> +typedef struct {
> +const char *name;
> +size_t len;
> +} literal;
> +
> +#define H2_DEF_LITERAL(n)   { (n), (sizeof(n)-1) }
> +#define H2_ALEN(a)  (sizeof(a)/sizeof((a)[0]))
> +#define H2_LIT_ARGS(a)  (a),H2_ALEN(a)
> +
> +static literal IgnoredRequestHeaders[] = {
> +H2_DEF_LITERAL("host"),
> +H2_DEF_LITERAL("expect"),
> +H2_DEF_LITERAL("upgrade"),
> +H2_DEF_LITERAL("connection"),
> +H2_DEF_LITERAL("keep-alive"),
> +H2_DEF_LITERAL("http2-settings"),
> +H2_DEF_LITERAL("proxy-connection"),
> +H2_DEF_LITERAL("transfer-encoding"),
> +};

 Shouldn't we also include all the tokens from the Connection header?
 (obviously not feasible with this only blacklist)


> +static literal IgnoredRequestTrailers[] = { /* Ignore, see rfc7230, ch. 
> 4.1.2 */
> +H2_DEF_LITERAL("te"),
> +H2_DEF_LITERAL("host"),
> +H2_DEF_LITERAL("range"),
> +H2_DEF_LITERAL("cookie"),
> +H2_DEF_LITERAL("expect"),
> +H2_DEF_LITERAL("pragma"),
> +H2_DEF_LITERAL("max-forwards"),
> +H2_DEF_LITERAL("cache-control"),
> +H2_DEF_LITERAL("authorization"),
> +H2_DEF_LITERAL("content-length"),
> +H2_DEF_LITERAL("proxy-authorization"),
> +};

 No notion of announced trailers in HTTP2, with eg. the request header
 "Trailer: token1, token2, ..." (any non-announced trailer would be
 ignored), something we could (always/optionally) enforce?

> +static literal IgnoredResponseTrailers[] = {
> +H2_DEF_LITERAL("age"),
> +H2_DEF_LITERAL("date"),
> +H2_DEF_LITERAL("vary"),
> +H2_DEF_LITERAL("cookie"),
> +H2_DEF_LITERAL("expires"),
> +H2_DEF_LITERAL("warning"),
> +H2_DEF_LITERAL("location"),
> +H2_DEF_LITERAL("retry-after"),

Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Yann Ylavic]

On Fri, Nov 20, 2015 at 4:44 PM, Stefan Eissing
 wrote:
> Most of this is discussed here: 
> https://httpwg.github.io/specs/rfc7540.html#HttpHeaders
>
> Basically HTTP/2 defines its own connection properties, so several things 
> which are announced/controlled
> by Connection: and other headers do not apply to HTTP/2 connections. So, the 
> "Connection:" header itself
> was obsoleted.
>
> mod_http2 has to translate between both worlds a bit and is doing so probably 
> incompletely so now, so
> the discussion about this is very useful.
>
> There are two conversions for mod_http2 to make:
> 1. request headers (H2 -> H1):
> 2. response headers (H1 -> H2):
>
> For 1. certain headers should not arrive at all. If they do, we can either 
> ignore or generate an error and deny the request. For example "Connection: 
> ..." should never arrive. Same for "Transfer-Encoding: ". The current 
> implementation ignores. One could argue for deny.
> For 2. certain headers we cannot send out and we need to take proper actions 
> for that. For example a "TE: deflate" we cannot process like this. Here more 
> checks need to be added.

OK, thanks for the explanation/pointer.

In rfc7540.html#rfc.section.8.1.2, looks like "TE: trailers" is still
meaningful, and "Trailer:" is also in the examples.
Should we ignore the trailers unless explicitely specified?

I really need to have a closer look at rfc7540...

Re: strncasecmp

[William A Rowe Jr]

I'm +1 if you are suggesting an ascii-only implementation, and by all means
introduce both an ap_ and apr_ flavor at the same time.

E.g. apr[r]_ascii_str[n]casecmp().

All characters <'A' || >'Z' && <'a || >'z' should be compared by identity.

I'm not sure which OS/X implementation you are referring to, though...
http://www.opensource.apple.com/source/Libc/Libc-1044.40.1/string/FreeBSD/strcasecmp.c

That implementation reminds us that we shouldn't be trusting these local
code page implementations for comparing defined ascii tokens, and many
of our comparisons in httpd are for such purposes.

But since a non-locale based ascii implementation comparing
tolower(x)-tolower(y) devolves to chrtable[x]-chrtable[y], where
tolower is inline, and if we intend to preserve the defined <0 == >0
behavior of the K definition, you do need to fold.

Yes the switch/case is ugly, but far more efficient for a larger set of
values/tokens.  One optimization would be iterating the pre-sorted
list of tokens in a binary fashion with a near skip pointer or returning
the elt of that item, but that would quickly become as crufty, subject
to bugs and unmaintainable as the usual switch/case solution.



On Fri, Nov 20, 2015 at 11:17 AM, Jim Jagielski  wrote:

> We use str[n]casecmp quite a bit. The rub is that some
> platforms use a sensible implementation (such as OSX) which
> uses a upper-lowercase map and is v. fast, and others
> use a brain-dead version which does an actual tolower() of
> each char in the string as it tests. We actually try to
> handle this in many cases by doing a switch/case test on the
> 1st char to fast path the strncasecmp, resulting in ugly code.
>
> This is crazy.
>
> I propose a ap_strncasecmp/ap_strcasecmp which we should use.
> Ideally, it would be in apr but no need to wait for that
> to happen :)
>
> Unless people have heartburn about this, I will add next week.
>

Re: strncasecmp

[William A Rowe Jr]

Pay special attention to;

The *strncasecmp*() function shall compare, *while ignoring differences in
case*, not more than *n* bytes from the string pointed to by *s1* to the
string pointed to by *s2*.

In the POSIX locale, *strcasecmp*() and *strncasecmp*() shall *behave as if
the strings had been converted to lowercase* and then a byte comparison
performed. The results are unspecified in other locales.
http://pubs.opengroup.org/onlinepubs/009695399/functions/strcasecmp.html

E.g. 0x5B-0x60 sort before alpha.

If you don't want to honor posix semantics, don't abuse the "strcmp"
concept, but rather name this "streq" or something otherwise less confusing
:-)

On Fri, Nov 20, 2015 at 12:22 PM, William A Rowe Jr 
wrote:

> I'm +1 if you are suggesting an ascii-only implementation, and by all means
> introduce both an ap_ and apr_ flavor at the same time.
>
> E.g. apr[r]_ascii_str[n]casecmp().
>
> All characters <'A' || >'Z' && <'a || >'z' should be compared by identity.
>
> I'm not sure which OS/X implementation you are referring to, though...
>
> http://www.opensource.apple.com/source/Libc/Libc-1044.40.1/string/FreeBSD/strcasecmp.c
>
> That implementation reminds us that we shouldn't be trusting these local
> code page implementations for comparing defined ascii tokens, and many
> of our comparisons in httpd are for such purposes.
>
> But since a non-locale based ascii implementation comparing
> tolower(x)-tolower(y) devolves to chrtable[x]-chrtable[y], where
> tolower is inline, and if we intend to preserve the defined <0 == >0
> behavior of the K definition, you do need to fold.
>
> Yes the switch/case is ugly, but far more efficient for a larger set of
> values/tokens.  One optimization would be iterating the pre-sorted
> list of tokens in a binary fashion with a near skip pointer or returning
> the elt of that item, but that would quickly become as crufty, subject
> to bugs and unmaintainable as the usual switch/case solution.
>
>
>
> On Fri, Nov 20, 2015 at 11:17 AM, Jim Jagielski  wrote:
>
>> We use str[n]casecmp quite a bit. The rub is that some
>> platforms use a sensible implementation (such as OSX) which
>> uses a upper-lowercase map and is v. fast, and others
>> use a brain-dead version which does an actual tolower() of
>> each char in the string as it tests. We actually try to
>> handle this in many cases by doing a switch/case test on the
>> 1st char to fast path the strncasecmp, resulting in ugly code.
>>
>> This is crazy.
>>
>> I propose a ap_strncasecmp/ap_strcasecmp which we should use.
>> Ideally, it would be in apr but no need to wait for that
>> to happen :)
>>
>> Unless people have heartburn about this, I will add next week.
>>
>
>

strncasecmp

[Jim Jagielski]

We use str[n]casecmp quite a bit. The rub is that some
platforms use a sensible implementation (such as OSX) which
uses a upper-lowercase map and is v. fast, and others
use a brain-dead version which does an actual tolower() of
each char in the string as it tests. We actually try to
handle this in many cases by doing a switch/case test on the
1st char to fast path the strncasecmp, resulting in ugly code.

This is crazy.

I propose a ap_strncasecmp/ap_strcasecmp which we should use.
Ideally, it would be in apr but no need to wait for that
to happen :)

Unless people have heartburn about this, I will add next week.

RE: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2: h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h h2_util.c h2_util.h

[Bert Huijben]

> -Original Message-
> From: Jim Jagielski [mailto:j...@jagunet.com]
> Sent: vrijdag 20 november 2015 17:04
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1715363 - in /httpd/httpd/trunk/modules/http2:
> h2_request.c h2_response.h h2_session.c h2_stream.c h2_stream.h
> h2_util.c h2_util.h
> 
> Ugg... I *just* noticed:
> 
>   However, header field names MUST be converted to lowercase
>   prior to their encoding in HTTP/2. A request or response
>   containing uppercase header field names MUST be treated as
>   malformed

Nghttp2 handles this just fine.

My early tests with serf showed that some servers didn't enforce the
validation rule, while others (nghttp2 based) did.

Bert

Re: strncasecmp

[Christophe JAILLET]

+1


Le 20/11/2015 18:17, Jim Jagielski a écrit :

We use str[n]casecmp quite a bit. The rub is that some
platforms use a sensible implementation (such as OSX) which
uses a upper-lowercase map and is v. fast, and others
use a brain-dead version which does an actual tolower() of
each char in the string as it tests. We actually try to
handle this in many cases by doing a switch/case test on the
1st char to fast path the strncasecmp, resulting in ugly code.

This is crazy.

I propose a ap_strncasecmp/ap_strcasecmp which we should use.
Ideally, it would be in apr but no need to wait for that
to happen :)

Unless people have heartburn about this, I will add next week.

RE: H2 stream dependencies

[Bert Huijben]

> -Original Message-
> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Sent: vrijdag 20 november 2015 10:26
> To: dev@httpd.apache.org
> Subject: Re: H2 stream dependencies
> 
> Bert,
> 
> interesting and nice to see the progress. You probably could use priorities 
> for
> ordering, especially when using the stream dependencies. I am not certain,
> however, if this will give you totally deterministic behavior. If stream B
> depends on A, usually A will be sent out before B. However, should stream A
> become suspended - e.g. unable to progress, either because there is no data
> available which some other thread needs to produce, or because the flow
> control window has not been updated in time - the server will start sending
> data for B.
> 
> The state of the implementation is:
> 2.4.17: fully implemented priority handling on sending stream data, as
> implemented by the nghttp2 library
> 2.4.18: additionally, streams are scheduled for execution by priority. Makes a
> difference when the number of available worker is less than the stream
> queue length, e.g. when der server is under load or only few workers are
> available (in prefork, for certain).
> 
> Another way to influence the ordering is for the client to play with the
> stream window sizes. If you give stream A 2^31-1 and stream B 0 until you
> have A, then update B, it may also do what you want. If both streams
> produce response bodies...flow only applies to DATA...

I'm currently thinking about combining both approaches... Sending the 
dependencies and (just to be sure) also keep the window low on such a stream. 
In that case only a very small cache should be able to handle the delaying in 
case (somehow) the dependency doesn't do the right thing.

In the specific case of Subversion there shouldn't be many cases where output 
is delayed, so I hope this will just work for us. But once we see h2 proxies, 
I'm guessing we will really need the window handling as fallback.



In a few of the remaining test failures I see httpd spinning at 100% CPU 
(automatically cooling down after some timout; perhaps if the connection is 
closed). I haven't investigated those, but it could be an httpd issue. This 
usually triggers other test failures for me as I limit the amount of workers 
and threads while testing. I'll follow-up when I know more.

Bert

Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c

[Marion & Christophe JAILLET]

That was I first thought too.

When I looked at r1715255, I noticed:
 const char *conn = apr_table_get(r->headers_in, "Connection");
 if (ap_find_token(r->pool, conn, "upgrade")) {
and looked for inconsistencies.

When digging deeper, I found that "Connection Upgrade" was used in most 
cases in httpd. That's why I reverted.


The only places where I found the lowercase version are:
  core.c:5366:   if (ap_find_token(r->pool, conn, "upgrade")) {
  ssl_engine_kernel.c:1344:  apr_table_mergen(r->headers_out, 
"Connection", "upgrade");

  http2: in different places

So, for consistency, it's maybe these places that should be updated???


In any case, I don't think that it would avoid some case folding.
For "Connection upgrade", the only places I've found that process it is 
the one given above (core.c:5366)
In this case, taking advantage of a lower case version would require to 
tweak ap_find_token.
This would, IMHO, add complexity to the code and would be worse for the 
common case (i.e. if what we have does not match what we test, we would 
test twice)



If having upgrade in lower case makes it way, other places should also 
be looked at:

Upgrade: WebSocketvswebsocket

CJ


Le 20/11/2015 00:24, William A Rowe Jr a écrit :
It wouldn't hurt to change this though, the tokens are generally 
represented in lowercase, and this could avoid case folding I suppose.


How often do we see value tokens as upper case from httpd? Let's be 
consistent although it isn't strictly required.




On Thu, Nov 19, 2015 at 3:54 PM, > wrote:


Author: jailletc36
Date: Thu Nov 19 21:54:48 2015
New Revision: 1715294

URL: http://svn.apache.org/viewvc?rev=1715294=rev
Log:
Revert r1715289 (Connection header field should use "upgrade"
instead of "Upgrade")

This is case-insensitive, so no need for such a change.

Modified:
httpd/httpd/trunk/server/core.c

Modified: httpd/httpd/trunk/server/core.c
URL:

http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1715294=1715293=1715294=diff

==
--- httpd/httpd/trunk/server/core.c (original)
+++ httpd/httpd/trunk/server/core.c Thu Nov 19 21:54:48 2015
@@ -5379,7 +5379,7 @@ static int core_upgrade_handler(request_
 /* Let the client know what we are upgrading
to. */
 apr_table_clear(r->headers_out);
 apr_table_setn(r->headers_out, "Upgrade",
protocol);
-apr_table_setn(r->headers_out, "Connection",
"upgrade");
+apr_table_setn(r->headers_out, "Connection",
"Upgrade");

 r->status = HTTP_SWITCHING_PROTOCOLS;
 r->status_line = ap_get_status_line(r->status);
@@ -5404,7 +5404,7 @@ static int core_upgrade_handler(request_
 if (upgrades && upgrades->nelts > 0) {
 char *protocols = apr_array_pstrcat(r->pool,
upgrades, ',');
 apr_table_setn(r->headers_out, "Upgrade", protocols);
-apr_table_setn(r->headers_out, "Connection", "upgrade");
+apr_table_setn(r->headers_out, "Connection", "Upgrade");
 }
 }

Re: strncasecmp

[Yann Ylavic]

+1

On Fri, Nov 20, 2015 at 6:17 PM, Jim Jagielski  wrote:
> We use str[n]casecmp quite a bit. The rub is that some
> platforms use a sensible implementation (such as OSX) which
> uses a upper-lowercase map and is v. fast, and others
> use a brain-dead version which does an actual tolower() of
> each char in the string as it tests. We actually try to
> handle this in many cases by doing a switch/case test on the
> 1st char to fast path the strncasecmp, resulting in ugly code.
>
> This is crazy.
>
> I propose a ap_strncasecmp/ap_strcasecmp which we should use.
> Ideally, it would be in apr but no need to wait for that
> to happen :)
>
> Unless people have heartburn about this, I will add next week.

Re: strncasecmp

[Jim Jagielski]

Implemented in r1715401

If people want to nit-pick about naming and wish to
rename it to something else, be my guest.

> On Nov 20, 2015, at 1:03 PM, Yann Ylavic  wrote:
> 
> +1
> 
> On Fri, Nov 20, 2015 at 6:17 PM, Jim Jagielski  wrote:
>> We use str[n]casecmp quite a bit. The rub is that some
>> platforms use a sensible implementation (such as OSX) which
>> uses a upper-lowercase map and is v. fast, and others
>> use a brain-dead version which does an actual tolower() of
>> each char in the string as it tests. We actually try to
>> handle this in many cases by doing a switch/case test on the
>> 1st char to fast path the strncasecmp, resulting in ugly code.
>> 
>> This is crazy.
>> 
>> I propose a ap_strncasecmp/ap_strcasecmp which we should use.
>> Ideally, it would be in apr but no need to wait for that
>> to happen :)
>> 
>> Unless people have heartburn about this, I will add next week.

Re: strncasecmp

[William A Rowe Jr]

At first glance this seems to meet the POSIX definition in the POSIX
context.  Just want to be careful about using this for non-posix
applications and your doxygen seems to cover this.  Looks good to me.
Implemented in r1715401

If people want to nit-pick about naming and wish to
rename it to something else, be my guest.

> On Nov 20, 2015, at 1:03 PM, Yann Ylavic  wrote:
>
> +1
>
> On Fri, Nov 20, 2015 at 6:17 PM, Jim Jagielski  wrote:
>> We use str[n]casecmp quite a bit. The rub is that some
>> platforms use a sensible implementation (such as OSX) which
>> uses a upper-lowercase map and is v. fast, and others
>> use a brain-dead version which does an actual tolower() of
>> each char in the string as it tests. We actually try to
>> handle this in many cases by doing a switch/case test on the
>> 1st char to fast path the strncasecmp, resulting in ugly code.
>>
>> This is crazy.
>>
>> I propose a ap_strncasecmp/ap_strcasecmp which we should use.
>> Ideally, it would be in apr but no need to wait for that
>> to happen :)
>>
>> Unless people have heartburn about this, I will add next week.

Re: strncasecmp

[Christophe JAILLET]

Le 20/11/2015 18:17, Jim Jagielski a écrit :

Ideally, it would be in apr

+1
This could also be even more interesting, because of apr_table_ functions.

CJ

Re: svn commit: r1715401 - in /httpd/httpd/trunk: include/ap_mmn.h include/httpd.h server/util.c

[Yann Ylavic]

On Fri, Nov 20, 2015 at 7:49 PM,   wrote:
> Author: jim
> Date: Fri Nov 20 18:49:38 2015
> New Revision: 1715401
>
> URL: http://svn.apache.org/viewvc?rev=1715401=rev
> Log:
> Provide our own impl of str[n]casecmp()
>
> This simply provides it. Next step is to change all uses of
> str[n]casecmp to ap_str[n]casecmp and *then* remove those silly
> logic paths where we check the 1st char of a string before
> we do the strcasecmp (since this is no longer expensive).
>
> Modified:
[]
> httpd/httpd/trunk/server/util.c
[]
> +AP_DECLARE(int) ap_strncasecmp(const char *s1, const char *s2, apr_size_t n)
> +{
> +const unsigned char *ps1 = (const unsigned char *) s1;
> +const unsigned char *ps2 = (const unsigned char *) s2;
> +if (n) {
> +do {
> +if (ucharmap[*ps1] != ucharmap[*ps2++]) {
> +return (ucharmap[*ps1] - ucharmap[*--ps2]);
> +}
> +if (*ps1++ == '\0') {
> +/* we know both end here */
> +return (0);
> +}
> +} while (!--n);

while (--n) probably.

> +}
> +return (0);
> +}
>
>

Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c

[Stefan Eissing]

+1 for lowercasing and pls backport, since it just arrived as is in 2.4.x, i 
think.

> Am 20.11.2015 um 00:24 schrieb William A Rowe Jr :
> 
> It wouldn't hurt to change this though, the tokens are generally represented 
> in lowercase, and this could avoid case folding I suppose.
> 
> How often do we see value tokens as upper case from httpd?  Let's be 
> consistent although it isn't strictly required.
> 
> 
> 
> On Thu, Nov 19, 2015 at 3:54 PM,  wrote:
> Author: jailletc36
> Date: Thu Nov 19 21:54:48 2015
> New Revision: 1715294
> 
> URL: http://svn.apache.org/viewvc?rev=1715294=rev
> Log:
> Revert r1715289 (Connection header field should use "upgrade" instead of 
> "Upgrade")
> 
> This is case-insensitive, so no need for such a change.
> 
> Modified:
> httpd/httpd/trunk/server/core.c
> 
> Modified: httpd/httpd/trunk/server/core.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1715294=1715293=1715294=diff
> ==
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Thu Nov 19 21:54:48 2015
> @@ -5379,7 +5379,7 @@ static int core_upgrade_handler(request_
>  /* Let the client know what we are upgrading to. */
>  apr_table_clear(r->headers_out);
>  apr_table_setn(r->headers_out, "Upgrade", protocol);
> -apr_table_setn(r->headers_out, "Connection", "upgrade");
> +apr_table_setn(r->headers_out, "Connection", "Upgrade");
> 
>  r->status = HTTP_SWITCHING_PROTOCOLS;
>  r->status_line = ap_get_status_line(r->status);
> @@ -5404,7 +5404,7 @@ static int core_upgrade_handler(request_
>  if (upgrades && upgrades->nelts > 0) {
>  char *protocols = apr_array_pstrcat(r->pool, upgrades, ',');
>  apr_table_setn(r->headers_out, "Upgrade", protocols);
> -apr_table_setn(r->headers_out, "Connection", "upgrade");
> +apr_table_setn(r->headers_out, "Connection", "Upgrade");
>  }
>  }
> 
> 
> 
> 

RE: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c

[Bert Huijben]

> -Original Message-
> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Sent: vrijdag 20 november 2015 10:11
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1715294 - /httpd/httpd/trunk/server/core.c
> 
> +1 for lowercasing and pls backport, since it just arrived as is in 2.4.x,
i think.



> >  apr_table_clear(r->headers_out);
> >  apr_table_setn(r->headers_out, "Upgrade",
protocol);
> > -apr_table_setn(r->headers_out, "Connection",
"upgrade");
> > +apr_table_setn(r->headers_out, "Connection",
"Upgrade");

As the 'Connection' header technically refers to the 'Upgrade' header I
would paint both sheds in the same color.

Either would be fine by me.

Bert

Re: svn commit: r1715401 - in /httpd/httpd/trunk: include/ap_mmn.h include/httpd.h server/util.c

[Yann Ylavic]

Alternatively, a single/simpler while (n--){} would work too.


On Fri, Nov 20, 2015 at 11:17 PM, Yann Ylavic  wrote:
> On Fri, Nov 20, 2015 at 7:49 PM,   wrote:
>> Author: jim
>> Date: Fri Nov 20 18:49:38 2015
>> New Revision: 1715401
>>
>> URL: http://svn.apache.org/viewvc?rev=1715401=rev
>> Log:
>> Provide our own impl of str[n]casecmp()
>>
>> This simply provides it. Next step is to change all uses of
>> str[n]casecmp to ap_str[n]casecmp and *then* remove those silly
>> logic paths where we check the 1st char of a string before
>> we do the strcasecmp (since this is no longer expensive).
>>
>> Modified:
> []
>> httpd/httpd/trunk/server/util.c
> []
>> +AP_DECLARE(int) ap_strncasecmp(const char *s1, const char *s2, apr_size_t n)
>> +{
>> +const unsigned char *ps1 = (const unsigned char *) s1;
>> +const unsigned char *ps2 = (const unsigned char *) s2;
>> +if (n) {
>> +do {
>> +if (ucharmap[*ps1] != ucharmap[*ps2++]) {
>> +return (ucharmap[*ps1] - ucharmap[*--ps2]);
>> +}
>> +if (*ps1++ == '\0') {
>> +/* we know both end here */
>> +return (0);
>> +}
>> +} while (!--n);
>
> while (--n) probably.
>
>> +}
>> +return (0);
>> +}
>>
>>