Re: svn commit: r1585609 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
Should I go with this ?
On Fri, Apr 17, 2015 at 11:54 AM, Yann Ylavic wrote:
>
> How about handling the case in the Require directive parser itself
> (add_authz_provider)?
>
> Something like:
>
> Index: modules/aaa/mod_authz_core.c
> ===
> --- modules/aaa/mod_authz_core.c(revision 1674046)
> +++ modules/aaa/mod_authz_core.c(working copy)
> @@ -393,7 +393,13 @@ static const char *add_authz_provider(cmd_parms *c
> section->negate = 1;
> }
>
> -section->provider_args = args;
> +if (args && (args[0] == '"' || args[0] == '\'')
> + && (args[strlen(args) - 1] == args[0])) {
> +section->provider_args = ap_getword_conf(cmd->pool, &args);
> +}
> +else {
> +section->provider_args = args;
> +}
>
> /* lookup and cache the actual provider now */
> section->provider = ap_lookup_provider(AUTHZ_PROVIDER_GROUP,
> @@ -425,7 +431,7 @@ static const char *add_authz_provider(cmd_parms *c
> AUTHZ_PROVIDER_NAME_NOTE,
> apr_pool_cleanup_null,
> cmd->temp_pool);
> -err = section->provider->parse_require_line(cmd, args,
> +err = section->provider->parse_require_line(cmd,
> section->provider_args,
>
> §ion->provider_parsed_args);
>
> if (err)
> @@ -1069,16 +1075,6 @@ static const char *expr_parse_config(cmd_parms *cm
> const char *expr_err = NULL;
> struct require_expr_info *info = apr_pcalloc(cmd->pool, sizeof(*info));
>
> -/* if the expression happens to be surrounded by quotes, skip them */
> -if (require_line[0] == '"') {
> -apr_size_t len = strlen(require_line);
> -
> -if (require_line[len-1] == '"')
> -require_line = apr_pstrndup(cmd->temp_pool,
> -require_line + 1,
> -len - 2);
> -}
> -
> apr_pool_userdata_setn(info, REQUIRE_EXPR_NOTE, apr_pool_cleanup_null,
>cmd->temp_pool);
> info->expr = ap_expr_parse_cmd(cmd, require_line, 0, &expr_err,
> --
>
> (the second hunk is a revert of this commit).
Re: svn commit: r1585609 - /httpd/httpd/trunk/modules/aaa/mod_authz_core.c
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-37kdqtkrruzb5hh...@mail.gmail.com%3E
On Fri, Apr 17, 2015 at 10:45 PM, Christophe JAILLET
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
>> 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
Re: Balancer manager
+1 There are also some neat-o features I added in my notes during ACNA to stick into the balancer manager, too... I plan to get around to them in days. -- Daniel Ruggeri On 4/24/2015 7:52 AM, Jim Jagielski wrote: > Right now, the balancer manager allows for a member to be > disabled/stopped, but it cannot *remove* that member... > Seems to me that that would be good, especially since > we could always re-use that slot. > > Comments?
Re: ATS: [VOTE] Release APR 1.5.2
I'm very sorry, that was sent from my phone, which was in my pocket... Turning the auto-unlock feature off right now. Thank you. Best regards, Martynas Bendorius On 4/25/15 4:22 PM, Martynas Bendorius wrote: Rarr Best regards, Martynas Bendorius Rarr Best regards, Martynas Bendorius Jeff Trawick rašė Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ Shortcut to CHANGES: http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 autoconf version: 2.69 (same as apr 1.5.1) libtool version: 2.4.2 (same as apr 1.5.1) +/-1 [ ] Release APR 1.5.2 as GA I'll hold the vote open for 72 hours unless something out of the ordinary occurs. Thanks in advance for testing! smime.p7s Description: S/MIME Cryptographic Signature
Re: [VOTE] Release APR 1.5.2
On Sat, Apr 25, 2015 at 9:37 AM, Yann Ylavic wrote: > Hi Jeff, > > shouldn't this be addressed to dev@apr instead? > yes, of course :) > > On Sat, Apr 25, 2015 at 2:13 PM, Jeff Trawick wrote: > > Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > > > > Shortcut to CHANGES: > > http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > > > > autoconf version: 2.69 (same as apr 1.5.1) > > libtool version: 2.4.2 (same as apr 1.5.1) > > > > +/-1 > > [ ] Release APR 1.5.2 as GA > > > > I'll hold the vote open for 72 hours unless something out of the ordinary > > occurs. > > > > Thanks in advance for testing! > > > -- Born in Roswell... married an alien... http://emptyhammock.com/
Re: [VOTE] Release APR 1.5.2
Hi Jeff, shouldn't this be addressed to dev@apr instead? On Sat, Apr 25, 2015 at 2:13 PM, Jeff Trawick wrote: > Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > > Shortcut to CHANGES: > http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > > autoconf version: 2.69 (same as apr 1.5.1) > libtool version: 2.4.2 (same as apr 1.5.1) > > +/-1 > [ ] Release APR 1.5.2 as GA > > I'll hold the vote open for 72 hours unless something out of the ordinary > occurs. > > Thanks in advance for testing! >
ATS: [VOTE] Release APR 1.5.2
:$ :-SS ,bbbhjb 098!('':-& c
Best regards,
Martynas Bendorius
Jeff Trawick rašė
>Tarballs/zipfiles are at http://apr.apache.org/dev/dist/
>
>Shortcut to CHANGES:
>http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2
>
>autoconf version: 2.69 (same as apr 1.5.1)
>libtool version: 2.4.2 (same as apr 1.5.1)
>
>+/-1
>[ ] Release APR 1.5.2 as GA
>
>I'll hold the vote open for 72 hours unless something out of the ordinary
>occurs.
>
>Thanks in advance for testing!
>
ATS: [VOTE] Release APR 1.5.2
Ef Best regards,Na:-& Martynas Bendorius Jeff Trawick rašė > Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > > Shortcut to CHANGES: > http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > > autoconf version: 2.69 (same as apr 1.5.1) > libtool version: 2.4.2 (same as apr 1.5.1) > > +/-1 > [ ] Release APR 1.5.2 as GA > > I'll hold the vote open for 72 hours unless something out of the ordinary > occurs. > > Thanks in advance for testing!
ATS: [VOTE] Release APR 1.5.2
Rarr Best regards, Martynas Bendorius Jeff Trawick rašė >Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > >Shortcut to CHANGES: >http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > >autoconf version: 2.69 (same as apr 1.5.1) >libtool version: 2.4.2 (same as apr 1.5.1) > >+/-1 >[ ] Release APR 1.5.2 as GA > >I'll hold the vote open for 72 hours unless something out of the ordinary >occurs. > >Thanks in advance for testing! >
ATS: [VOTE] Release APR 1.5.2
Si Best regards, Martynas Bendorius Jeff Trawick rašė > Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > > Shortcut to CHANGES: > http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > > autoconf version: 2.69 (same as apr 1.5.1) > libtool version: 2.4.2 (same as apr 1.5.1) > > +/-1 > [ ] Release APR 1.5.2 as GA > > I'll hold the vote open for 72 hours unless something out of the ordinary > occurs. > > Thanks in advance for testing!
ATS: [VOTE] Release APR 1.5.2
Best regards, Martynas Bendorius Jeff Trawick rašė >Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ > >Shortcut to CHANGES: >http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 > >autoconf version: 2.69 (same as apr 1.5.1) >libtool version: 2.4.2 (same as apr 1.5.1) > >+/-1 >[ ] Release APR 1.5.2 as GA > >I'll hold the vote open for 72 hours unless something out of the ordinary >occurs. > >Thanks in advance for testing! >
[VOTE] Release APR 1.5.2
Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ Shortcut to CHANGES: http://apr.apache.org/dev/dist/CHANGES-APR-1.5.2 autoconf version: 2.69 (same as apr 1.5.1) libtool version: 2.4.2 (same as apr 1.5.1) +/-1 [ ] Release APR 1.5.2 as GA I'll hold the vote open for 72 hours unless something out of the ordinary occurs. Thanks in advance for testing!
Re: ALPN patch comments
On 22.04.2015 18:54, Jim Jagielski wrote: >> For me the time seems right to rip NPN out of trunk and only backport >> the ALPN code to 2.4. >> > > I'd be +1 for that. So, to get one step further, and since there were no explicit objections to removing NPN support so far (or arguments for keeping it, FWIW), I went ahead and took a stab at this with r1676004. Only tested in terms of "compiles both w/ and w/o HAVE_TLS_ALPN", so it certainly needs more eyes before a backport proposal could be made. There's also a "TODO: we should have a mod_ssl configuration parameter" in ssl_engine_kernel.c which I'm unsure to what it refers. Kaspar
