httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1585609 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Date Sat, 25 Apr 2015 23:16:10 GMT
I have checked all the calls to ap_expr_parse_cmd() and it seems that
the change in the parser itself is not expected/needed.
Either ap_getword_conf() is implicit (parser called from an AP_TAKEn
or AP_TAKE_ARGV directive), or the caller (AP_RAW_ARGS directive) uses
it explicitly.
Hence that would cause a double unquoting...

There are some special cases though:
- ap_authz_providers as per this commit.
- mod_log_config, mod_log_debug, mod_headers have the expr=... syntax
which does not handle quoted strings;
- mod_rewrite has a special handling of quoted strings, including
RewriteCond expr ... (but it is consistent with all RewriteCond, so
guess it should be kept as is);

The patch proposed in [1] is I think the simplest way to handle the
case for *all* the ap_authz_providers.

Otherwise, we could introduce a new ap_expr_parse_cmd_conf() which
would handle quoted strings and advance the word pointer to the next
one (à la ap_getword_conf() where the word is an expression, either
quoted or until the end of line).
It would then be used by each ap_authz_provider's parser
implementation, and also by mod_{log_config,log_debug,headers} above
for the expr= case.
Something like:

Index: server/util_expr_eval.c
===================================================================
--- server/util_expr_eval.c    (revision 1674695)
+++ server/util_expr_eval.c    (working copy)
@@ -443,20 +443,31 @@ AP_DECLARE(const char *) ap_expr_parse(apr_pool_t
     return NULL;
 }

-AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(const cmd_parms *cmd,
-                                                 const char *expr,
-                                                 unsigned int flags,
-                                                 const char **err,
-                                                 ap_expr_lookup_fn_t
*lookup_fn,
-                                                 int module_index)
+AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_conf(const cmd_parms *cmd,
+                                                const char **line,
+                                                unsigned int flags,
+                                                const char **err,
+                                                ap_expr_lookup_fn_t *lookup_fn,
+                                                int module_index)
 {
+    const char *expr = *line;
+    apr_size_t len = strlen(expr);
     ap_expr_info_t *info = apr_pcalloc(cmd->pool, sizeof(ap_expr_info_t));
+
     info->filename = cmd->directive->filename;
     info->line_number = cmd->directive->line_num;
     info->flags = flags;
     info->module_index = module_index;
+
+    if (len > 1 && (expr[0] == '"' || expr[0] == '\'')
+                && (expr[len - 1] == expr[0])) {
+        expr = ap_getword_conf(cmd->temp_pool, line);
+    }
+    else {
+        *line = expr + len;
+    }
+
     *err = ap_expr_parse(cmd->pool, cmd->temp_pool, info, expr, lookup_fn);
-
     if (*err)
         return NULL;

@@ -463,6 +474,17 @@ AP_DECLARE(const char *) ap_expr_parse(apr_pool_t
     return info;
 }

+AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(const cmd_parms *cmd,
+                                                 const char *expr,
+                                                 unsigned int flags,
+                                                 const char **err,
+                                                 ap_expr_lookup_fn_t
*lookup_fn,
+                                                 int module_index)
+{
+    return ap_expr_parse_cmd_conf(cmd, &expr, flags, err,
+                                  lookup_fn, module_index);
+}
+
 ap_expr_t *ap_expr_make(ap_expr_node_op_e op, const void *a1, const void *a2,
                       ap_expr_parse_ctx_t *ctx)
 {
--

I prefer the patch from [1] though, because it is simpler and more
centralized for the ap_authz_providers case.
We could likewise use ap_getword_conf() explicitely for the expr=
cases, but they do not look critical to me, and "expr=..." (with
quotes around the whole) can still be used.
The only advantage I see for the new function would be if we have more
(and more) needs for it...

[1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201504.mbox/%3CCAKQ1sVPnuJFyCCuK1QkcaQX6y50qjEy-37KDQTkrRUZB5hHoJA@mail.gmail.com%3E

On Fri, Apr 17, 2015 at 10:45 PM, Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Hi,
>
> I would +1 for this solution which is, IMHO, much better.
> However, changing the parser itself would require checking the potential
> impact as it is used in many places.
>
> CJ
>
>
> Le 17/04/2015 13:15, Yann Ylavic a écrit :
>>
>> On Fri, Apr 17, 2015 at 11:54 AM, Yann Ylavic <ylavic.dev@gmail.com>
>> wrote:
>>>
>>> How about handling the case in the Require directive parser itself
>>> (add_authz_provider)?
>>
>> Or even maybe in the expr parser itself:
>>
>> Index: server/util_expr_eval.c
>> ===================================================================
>> --- server/util_expr_eval.c    (revision 1674046)
>> +++ server/util_expr_eval.c    (working copy)
>> @@ -455,6 +455,10 @@ AP_DECLARE(ap_expr_info_t*) ap_expr_parse_cmd_mi(c
>>       info->line_number = cmd->directive->line_num;
>>       info->flags = flags;
>>       info->module_index = module_index;
>> +    if (expr && (expr[0] == '"' || expr[0] == '\'') && (expr[1]
!= '\0')
>> +             && (expr[strlen(expr) - 1] == expr[0])) {
>> +        expr = ap_getword_conf(cmd->temp_pool, &expr);
>> +    }
>>       *err = ap_expr_parse(cmd->pool, cmd->temp_pool, info, expr,
>> lookup_fn);
>>
>>       if (*err)
>> --
>>
>

Mime
View raw message