Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[William A Rowe Jr]

On Wed, Mar 16, 2016 at 3:58 AM, Stefan Eissing <
stefan.eiss...@greenbytes.de> wrote:

> Hmm, I can tell you for certain that modules encountering HTTP/2
> connections
> in 2.4 do not find the filters as they may expect.
>

Right, that's by design.  Existing protocol modules assume a particular
stack
for a 'default' newly created connection.  If that connection is configured
as h2
then all bets are off, but this wouldn't be the case, especially on an
upgrade
of one subversion to another (e.g. 2.4.18 to 2.4.20).

A content module that assumes anything about the filter stack can run into
troubles using mod_http2, or mod_ftp, or lots of other examples, but these
protocol modules are making assumptions about 2.4.x based on what the
server was already doing.  Let's not break those assumptions until 2.6.0
when lots of assumptions go out the door.

Bill

Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[Graham Leggett]

On 16 Mar 2016, at 5:57 AM, William A Rowe Jr  wrote:

> My concern is that this can't and shouldn't change on 2.4.x.
> 
> I love the concept and it is correct, however there are enough modules relying
> on the fact that they must remove the http protocol filters that changing the
> default behavior is effectively breaking binary ABI.
> 
> Thoughts?

Hmm…removing a filter that doesn’t exist isn’t an error, but relying on a side 
effect of a filter being there and now suddenly it is not there any more 
certainly will be. You’re right on the ABI.

There are two approaches that leap to my mind, one is provide an optional 
function that signals “don’t add http filters on non http connections”, the 
second is to keep this change on v2.5+ and use version check to remove filters 
on any code that is intended to run correctly on v2.4.

Regards,
Graham
—

Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[Nick Kew]

On Tue, 2016-03-15 at 22:57 -0500, William A Rowe Jr wrote:
> My concern is that this can't and shouldn't change on 2.4.x.

> Thoughts?

If someone is independently using a different protocol,
or has just tweaked HTTP handling for their own reasons,
they could be using hacks/workarounds this'll break.
We've seen similar cases in the lifetime of 2.4 where PHP
introduced its own workarounds that our bugfixes then broke.

Basic lesson: any such patch must be switchable and default
to off.

On the other hand, mod_h2 being marked as pre-stable means
we haven't promised stability to anyone who's hacked it.
So mod_h2 could safely deal with protocol filters.  Or by
extension could explicitly set a flag to trigger a core
change as per this patch.

Does this merit that level of hack?

-- 
Nick Kew

Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[Stefan Eissing]

Hmm, I can tell you for certain that modules encountering HTTP/2 connections
in 2.4 do not find the filters as they may expect.

So far, I have not heard of any breakage related to this. Which is not to say
it does not exist. 

AFAIK the patch will only affect connections where ap_get_protocol() returns
something else than the default. So far, this is only true for h2 connections,
which is not enabled by default. 

So, I do not see the need for worries here.

-Stefan

> Am 16.03.2016 um 09:34 schrieb Ruediger Pluem :
> 
> 
> 
> On 03/16/2016 04:57 AM, William A Rowe Jr wrote:
>> My concern is that this can't and shouldn't change on 2.4.x.
>> 
>> I love the concept and it is correct, however there are enough modules 
>> relying
>> on the fact that they must remove the http protocol filters that changing the
>> default behavior is effectively breaking binary ABI.
>> 
>> Thoughts?
> 
> Agreed.
> 
> Regards
> 
> Rüdiger
> 

Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[Ruediger Pluem]

On 03/16/2016 04:57 AM, William A Rowe Jr wrote:
> My concern is that this can't and shouldn't change on 2.4.x.
> 
> I love the concept and it is correct, however there are enough modules relying
> on the fact that they must remove the http protocol filters that changing the
> default behavior is effectively breaking binary ABI.
> 
> Thoughts?

Agreed.

Regards

Rüdiger

Re: [Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[William A Rowe Jr]

My concern is that this can't and shouldn't change on 2.4.x.

I love the concept and it is correct, however there are enough modules
relying
on the fact that they must remove the http protocol filters that changing
the
default behavior is effectively breaking binary ABI.

Thoughts?

Cheers,

Bill

On Mon, Mar 14, 2016 at 7:22 PM, Graham Leggett  wrote:

> Hi all,
>
> The following patch fixes the problem where the HTTP1 filters are added on
> non HTTP1 requests.
>
> The filters are now only added if ap_get_protocol() returns
> AP_PROTOCOL_HTTP1.
>
> Regards,
> Graham
> --
>
> Index: modules/http/http_core.c
> ===
> --- modules/http/http_core.c(revision 1734998)
> +++ modules/http/http_core.c(working copy)
> @@ -34,6 +34,8 @@
>
>  #include "mod_core.h"
>
> +module AP_MODULE_DECLARE_DATA http_module;
> +
>  /* Handles for core filters */
>  AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
>  AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
> @@ -48,6 +50,10 @@
>   */
>  static int async_mpm = 0;
>
> +typedef struct {
> +int is_http;
> +} http_conn_conf;
> +
>  static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
>const char *arg)
>  {
> @@ -242,9 +248,29 @@
>  return OK;
>  }
>
> +static int http_pre_connection(conn_rec *c, void *csd)
> +{
> +const char *protocol = ap_get_protocol(c);
> +
> +http_conn_conf *cconf = apr_pcalloc(c->pool, sizeof(*cconf));
> +
> +if (!protocol || !strcmp(AP_PROTOCOL_HTTP1, protocol)) {
> +cconf->is_http = 1;
> +}
> +
> +ap_set_module_config(c->conn_config, _module, cconf);
> +
> +return OK;
> +}
> +
>  static int ap_process_http_connection(conn_rec *c)
>  {
> -if (async_mpm && !c->clogging_input_filters) {
> +http_conn_conf *cconf = ap_get_module_config(c->conn_config,
> _module);
> +
> +if (!cconf || !cconf->is_http) {
> +return DECLINED;
> +}
> +else if (async_mpm && !c->clogging_input_filters) {
>  return ap_process_http_async_connection(c);
>  }
>  else {
> @@ -254,7 +280,6 @@
>
>  static int http_create_request(request_rec *r)
>  {
> -/* FIXME: we must only add these filters if we are an HTTP request */
>  if (!r->main && !r->prev) {
>  ap_add_output_filter_handle(ap_byterange_filter_handle,
>  NULL, r, r->connection);
> @@ -295,6 +320,7 @@
>  static void register_hooks(apr_pool_t *p)
>  {
>  ap_hook_post_config(http_post_config, NULL, NULL, APR_HOOK_MIDDLE);
> +ap_hook_pre_connection(http_pre_connection,NULL,NULL,
> APR_HOOK_REALLY_LAST);
>  ap_hook_process_connection(ap_process_http_connection, NULL, NULL,
> APR_HOOK_REALLY_LAST);
>  ap_hook_map_to_storage(ap_send_http_trace,NULL,NULL,APR_HOOK_MIDDLE);
>
>

[Patch] Ensure HTTP1 filters are only added on HTTP1 requests

[Graham Leggett]

Hi all,

The following patch fixes the problem where the HTTP1 filters are added on non 
HTTP1 requests.

The filters are now only added if ap_get_protocol() returns AP_PROTOCOL_HTTP1.

Regards,
Graham
--

Index: modules/http/http_core.c
===
--- modules/http/http_core.c(revision 1734998)
+++ modules/http/http_core.c(working copy)
@@ -34,6 +34,8 @@
 
 #include "mod_core.h"
 
+module AP_MODULE_DECLARE_DATA http_module;
+
 /* Handles for core filters */
 AP_DECLARE_DATA ap_filter_rec_t *ap_http_input_filter_handle;
 AP_DECLARE_DATA ap_filter_rec_t *ap_http_header_filter_handle;
@@ -48,6 +50,10 @@
  */
 static int async_mpm = 0;
 
+typedef struct {
+int is_http;
+} http_conn_conf;
+
 static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
   const char *arg)
 {
@@ -242,9 +248,29 @@
 return OK;
 }
 
+static int http_pre_connection(conn_rec *c, void *csd)
+{
+const char *protocol = ap_get_protocol(c);
+
+http_conn_conf *cconf = apr_pcalloc(c->pool, sizeof(*cconf));
+
+if (!protocol || !strcmp(AP_PROTOCOL_HTTP1, protocol)) {
+cconf->is_http = 1;
+}
+
+ap_set_module_config(c->conn_config, _module, cconf);
+
+return OK;
+}
+
 static int ap_process_http_connection(conn_rec *c)
 {
-if (async_mpm && !c->clogging_input_filters) {
+http_conn_conf *cconf = ap_get_module_config(c->conn_config, _module);
+
+if (!cconf || !cconf->is_http) {
+return DECLINED;
+}
+else if (async_mpm && !c->clogging_input_filters) {
 return ap_process_http_async_connection(c);
 }
 else {
@@ -254,7 +280,6 @@
 
 static int http_create_request(request_rec *r)
 {
-/* FIXME: we must only add these filters if we are an HTTP request */
 if (!r->main && !r->prev) {
 ap_add_output_filter_handle(ap_byterange_filter_handle,
 NULL, r, r->connection);
@@ -295,6 +320,7 @@
 static void register_hooks(apr_pool_t *p)
 {
 ap_hook_post_config(http_post_config, NULL, NULL, APR_HOOK_MIDDLE);
+ap_hook_pre_connection(http_pre_connection,NULL,NULL, 
APR_HOOK_REALLY_LAST);
 ap_hook_process_connection(ap_process_http_connection, NULL, NULL,
APR_HOOK_REALLY_LAST);
 ap_hook_map_to_storage(ap_send_http_trace,NULL,NULL,APR_HOOK_MIDDLE);