Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

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

Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

Yann Ylavic
Hi Luca,

On Mon, Sep 18, 2017 at 7:08 PM,  <[hidden email]> wrote:

>
> Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1808746&r1=1808745&r2=1808746&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Mon Sep 18 17:08:54 2017
> @@ -2035,7 +2035,10 @@ static char *lookup_variable(char *var,
>
>              case 'S':
>                  if (!strcmp(var, "HTTP_HOST")) {
> -                    result = lookup_header("Host", ctx);
> +                    /* Skip the 'Vary: Host' header combination
> +                     * as indicated in rfc7231 section-7.1.4
> +                     */
> +                    result = apr_table_get(ctx->r->headers_in, "Host");

Why not address this in lookup_header directly?
Looks like there several call sites...

Most seem to use a fixed name (other than "Host"), but the one around
line 1879 is:

            if (!strncasecmp(var, "HTTP", 4)) {
                result = lookup_header(var+5, ctx);
            }

so possibly also a candidate.

This might be enough though, to avoid spurious cycles spent in lookup_header().


Regards,
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

Luca Toscano
Hi Yann,

2017-09-18 19:28 GMT+02:00 Yann Ylavic <[hidden email]>:
Hi Luca,

On Mon, Sep 18, 2017 at 7:08 PM,  <[hidden email]> wrote:
>
> Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1808746&r1=1808745&r2=1808746&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Mon Sep 18 17:08:54 2017
> @@ -2035,7 +2035,10 @@ static char *lookup_variable(char *var,
>
>              case 'S':
>                  if (!strcmp(var, "HTTP_HOST")) {
> -                    result = lookup_header("Host", ctx);
> +                    /* Skip the 'Vary: Host' header combination
> +                     * as indicated in rfc7231 section-7.1.4
> +                     */
> +                    result = apr_table_get(ctx->r->headers_in, "Host");

Why not address this in lookup_header directly?
Looks like there several call sites...

I missed the HTTP:Host combination, I originally thought to make the strcmp comparison only when explicitly needed rather than in all cases in which lookup_header was called.. 
 

Most seem to use a fixed name (other than "Host"), but the one around
line 1879 is:

            if (!strncasecmp(var, "HTTP", 4)) {
                result = lookup_header(var+5, ctx);
            }

so possibly also a candidate.

This might be enough though, to avoid spurious cycles spent in lookup_header().

Something like the following? What do you think?

Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c (revision 1808842)
+++ modules/mappers/mod_rewrite.c (working copy)
@@ -1877,7 +1877,14 @@
             const char *path;

             if (!strncasecmp(var, "HTTP", 4)) {
-                result = lookup_header(var+5, ctx);
+                /* Skip the 'Vary: Host' header combination
+                 * as indicated in rfc7231 section-7.1.4
+                 */
+                if (!strcmp(var+5, "Host")) {
+                    result = apr_table_get(ctx->r->headers_in, "Host");
+                } else {
+                    result = lookup_header(var+5, ctx);
+                }
             }
             else if (!strncasecmp(var, "LA-U", 4)) {
                 if (ctx->uri && subreq_ok(r)) {
Index: server/util_expr_eval.c
===================================================================
--- server/util_expr_eval.c (revision 1808842)
+++ server/util_expr_eval.c (working copy)
@@ -1044,7 +1044,12 @@
         t = ctx->r->headers_in;
     else {                          /* req, http */
         t = ctx->r->headers_in;
-        add_vary(ctx, arg);
+        /* Skip the 'Vary: Host' header combination
+         * as indicated in rfc7231 section-7.1.4
+         */
+        if (strcmp(arg, "Host")){
+            add_vary(ctx, arg);
+        }
     }
     return apr_table_get(t, arg);
 } 



Thanks for the review!

Luca
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

Yann Ylavic
Hi Luca,

On Tue, Sep 19, 2017 at 11:02 AM, Luca Toscano <[hidden email]> wrote:
>
> Something like the following? What do you think?

After a quick look, I think we should put the test in looker_header, like:

Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c       (revision 1808823)
+++ modules/mappers/mod_rewrite.c       (working copy)
@@ -1796,7 +1796,7 @@ static const char *lookup_header(const char *name,
 {
     const char *val = apr_table_get(ctx->r->headers_in, name);

-    if (val) {
+    if (val && strcasecmp(name, "Host") != 0) {
         ctx->vary_this = ctx->vary_this
                          ? apr_pstrcat(ctx->r->pool, ctx->vary_this, ", ",
                                        name, NULL)
--

The call sites either have the name to be really checked (e.g. var+5),
or they use string literals (e.g. "Host", "Accept", ...) thus the
compiler is able to inline and optimize in or out the test by itself.


> Index: server/util_expr_eval.c
> ===================================================================
> --- server/util_expr_eval.c (revision 1808842)
> +++ server/util_expr_eval.c (working copy)
> @@ -1044,7 +1044,12 @@
>          t = ctx->r->headers_in;
>      else {                          /* req, http */
>          t = ctx->r->headers_in;
> -        add_vary(ctx, arg);
> +        /* Skip the 'Vary: Host' header combination
> +         * as indicated in rfc7231 section-7.1.4
> +         */
> +        if (strcmp(arg, "Host")){
> +            add_vary(ctx, arg);
> +        }
>      }
>      return apr_table_get(t, arg);
>  }

This one looks good.


Regards,
Yann.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

Yann Ylavic
On Tue, Sep 19, 2017 at 12:13 PM, Yann Ylavic <[hidden email]> wrote:

>
> On Tue, Sep 19, 2017 at 11:02 AM, Luca Toscano <[hidden email]> wrote:
>
>> Index: server/util_expr_eval.c
>> ===================================================================
>> --- server/util_expr_eval.c (revision 1808842)
>> +++ server/util_expr_eval.c (working copy)
>> @@ -1044,7 +1044,12 @@
>>          t = ctx->r->headers_in;
>>      else {                          /* req, http */
>>          t = ctx->r->headers_in;
>> -        add_vary(ctx, arg);
>> +        /* Skip the 'Vary: Host' header combination
>> +         * as indicated in rfc7231 section-7.1.4
>> +         */
>> +        if (strcmp(arg, "Host")){
>> +            add_vary(ctx, arg);
>> +        }
>>      }
>>      return apr_table_get(t, arg);
>>  }
>
> This one looks good.

Well, headers' names are case insensitive, so strcasecmp() above ;)

>
>
> Regards,
> Yann.
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1808746 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_rewrite.c server/util_expr_eval.c

Luca Toscano
In reply to this post by Yann Ylavic


2017-09-19 12:13 GMT+02:00 Yann Ylavic <[hidden email]>:
Hi Luca,

On Tue, Sep 19, 2017 at 11:02 AM, Luca Toscano <[hidden email]> wrote:
>
> Something like the following? What do you think?

After a quick look, I think we should put the test in looker_header, like:

Index: modules/mappers/mod_rewrite.c
===================================================================
--- modules/mappers/mod_rewrite.c       (revision 1808823)
+++ modules/mappers/mod_rewrite.c       (working copy)
@@ -1796,7 +1796,7 @@ static const char *lookup_header(const char *name,
 {
     const char *val = apr_table_get(ctx->r->headers_in, name);

-    if (val) {
+    if (val && strcasecmp(name, "Host") != 0) {
         ctx->vary_this = ctx->vary_this
                          ? apr_pstrcat(ctx->r->pool, ctx->vary_this, ", ",
                                        name, NULL)
--

The call sites either have the name to be really checked (e.g. var+5),
or they use string literals (e.g. "Host", "Accept", ...) thus the
compiler is able to inline and optimize in or out the test by itself.


> Index: server/util_expr_eval.c
> ===================================================================
> --- server/util_expr_eval.c (revision 1808842)
> +++ server/util_expr_eval.c (working copy)
> @@ -1044,7 +1044,12 @@
>          t = ctx->r->headers_in;
>      else {                          /* req, http */
>          t = ctx->r->headers_in;
> -        add_vary(ctx, arg);
> +        /* Skip the 'Vary: Host' header combination
> +         * as indicated in rfc7231 section-7.1.4
> +         */
> +        if (strcmp(arg, "Host")){
> +            add_vary(ctx, arg);
> +        }
>      }
>      return apr_table_get(t, arg);
>  }

This one looks good.



Thanks,

Luca