On 6 November 2013 19:06, Jeff Trawick <traw...@gmail.com> wrote:
> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m....@googlemail.com>
> wrote:
>>
>> On 6 November 2013 14:27, Jeff Trawick <traw...@gmail.com> wrote:
>> > On Wed, Nov 6, 2013 at 8:50 AM, Steve Hay <steve.m....@googlemail.com>
>> > wrote:
>> >>
>> >> On 6 November 2013 13:50, Steve Hay <steve.m....@googlemail.com> wrote:
>> >> > On 6 November 2013 12:27, Jeff Trawick <traw...@gmail.com> wrote:
>> >> >> On Wed, Nov 6, 2013 at 4:02 AM, Steve Hay
>> >> >> <steve.m....@googlemail.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> On 6 November 2013 00:48, Jeff Trawick <traw...@gmail.com> wrote:
>> >> >>> > Back to the httpd24threading branch:
>> >> >>> >
>> >> >>> > * modperl_interp_pool_select() has this notion of phase, which
>> >> >>> > must
>> >> >>> > either
>> >> >>> > be startup or request context.
>> >> >>> > * It thinks it is startup only if the pool passed in is
>> >> >>> > s->process->pconf.
>> >> >>> > * Sometimes it is passed s->process->pool (parent of pconf), such
>> >> >>> > as
>> >> >>> > from
>> >> >>> > perl_parse_require_line().
>> >> >>> > * perl_parse_require_line() can sometimes be called from request
>> >> >>> > context.
>> >> >>> > * When perl_parse_require_line() calls
>> >> >>> > modperl_interp_pool_select(),
>> >> >>> > request
>> >> >>> > context can never be identified because perl_parse_require_line()
>> >> >>> > never
>> >> >>> > passes in r->pool (which I guess would be cmd->pool).
>> >> >>> > * etc.
>> >> >>> >
>> >> >>> > This would seem to be the way to get the right pool to
>> >> >>> > modperl_interp_pool_select().
>> >> >>> >
>> >> >>> > Index: src/modules/perl/modperl_util.c
>> >> >>> >
>> >> >>> > ===================================================================
>> >> >>> > --- src/modules/perl/modperl_util.c (revision 1539040)
>> >> >>> > +++ src/modules/perl/modperl_util.c (working copy)
>> >> >>> > @@ -989,7 +989,7 @@
>> >> >>> > int count;
>> >> >>> > void *key;
>> >> >>> > auth_callback *ab;
>> >> >>> > - MP_dINTERP_POOLa(cmd->server->process->pool, cmd->server);
>> >> >>> > + MP_dINTERP_POOLa(cmd->pool, cmd->server);
>> >> >>> >
>> >> >>> > if (global_authz_providers == NULL) {
>> >> >>> > MP_INTERP_PUTBACK(interp, aTHX);
>> >> >>> >
>> >> >>> > That still doesn't bring happiness (no interpreter returned,
>> >> >>> > resulting
>> >> >>> > in a
>> >> >>> > crash trying to dereference interp).
>> >> >>> >
>> >> >>>
>> >> >>> I'm getting the same crash-on-startup behaviour now myself after a
>> >> >>> fresh rebuild of everything (now using httpd-2.4.6 and
>> >> >>> perl-5.19.5). I
>> >> >>> will look back over the changes made on the threading branch and/or
>> >> >>> my
>> >> >>> merges of them into the httpd24 branch. Hopefully the answer lies
>> >> >>> there somewhere. I'll be very grateful for any help I can get with
>> >> >>> this though -- I didn't do the original work on either of those
>> >> >>> branches...
>> >> >>
>> >> >>
>> >> >> With the "fix" above in place, modperl_init_vhost() seems to be the
>> >> >> next
>> >> >> crucial code. We go down this path:
>> >> >>
>> >> >> if (base_server == s) {
>> >> >> MP_TRACE_i(MP_FUNC, "base server is not vhost, skipping %s",
>> >> >> vhost);
>> >> >> return OK;
>> >> >> }
>> >> >>
>> >> >> and fall through this FIXME in modperl_interp_pool_select():
>> >> >>
>> >> >> if (!scfg->mip) {
>> >> >> /* FIXME: We get here if global "server_rec" ==
>> >> >> s,
>> >> >> scfg->mip
>> >> >> * is not created then. I'm not sure if that's
>> >> >> bug
>> >> >> or
>> >> >> * bad/good design decicision. For now just
>> >> >> return
>> >> >> NULL.
>> >> >> */
>> >> >> return NULL;
>> >> >> }
>> >> >>
>> >> >> (Note: disabling the base_server == s check in modperl_init_vhost()
>> >> >> brings
>> >> >> no happiness either, though perhaps it is a step in the right
>> >> >> direction.)
>> >> >>
>> >> >> This path is new with httpd 2.4; 2.2 didn't have authz_providers.
>> >> >>
>> >> >> This seems to be a whack-a-mole issue. I'd expect that there is
>> >> >> some
>> >> >> easy
>> >> >> way to grab the interpreter for any arbitrary startup path, but I
>> >> >> don't
>> >> >> see
>> >> >> it. Maybe it is worthwhile seeing if we already went through some
>> >> >> paths
>> >> >> where we were able to grab an interpreter.
>> >> >>
>> >> >
>> >> > The last change on the httpd24 branch (r1503193) is what added the
>> >> > FIXME above, but it also made a change in perl_parse_require_line()
>> >> > which I've lost in the course of merging the threading branch in: it
>> >> > made that function tolerant of modperl_interp_pool_select() returning
>> >> > NULL (which is exactly what happens in the FIXME case).
>> >> >
>> >> > If modperl_interp_pool_select() returns NULL then
>> >> > perl_parse_require_line() just returns NULL itself in r1503193, but
>> >> > in
>> >> > httpd24threading I've hidden the use of modperl_interp_pool_select()
>> >> > within the MP_dINTERP_POOLa() macro (as per the general style of
>> >> > changes in the threading branch), but that macro crashes if
>> >> > modperl_interp_pool_select() has returned NULL.
>> >> >
>> >> > The diff below makes that macro tolerant of
>> >> > modperl_interp_pool_select() returning NULL, and makes
>> >> > perl_parse_require_line() tolerant of interp ending up NULL, like it
>> >> > used to be in r1503193.
>> >> >
>> >> > With this diff in place (which includes your earlier change), the
>> >> > server now starts up for me and tests appear to be running as normal.
>> >>
>> >> Oops! In my excitement I forgot the diff!:
>> >>
>> >> Index: src/modules/perl/modperl_interp.h
>> >> ===================================================================
>> >> --- src/modules/perl/modperl_interp.h (revision 1539262)
>> >> +++ src/modules/perl/modperl_interp.h (working copy)
>> >> @@ -68,9 +68,12 @@
>> >> #define MP_INTERP_POOLa(p, s)
>> >> \
>> >> MP_TRACE_i(MP_FUNC, "selecting interp: p=%pp, s=%pp", (p), (s));
>> >> \
>> >> interp = modperl_interp_pool_select((p), (s));
>> >> \
>> >> - MP_TRACE_i(MP_FUNC, " --> got (0x%pp)->refcnt=%d",
>> >> \
>> >> - interp, interp->refcnt);
>> >> \
>> >> - aTHX = interp->perl
>> >> + if (interp) {
>> >> \
>> >> + MP_TRACE_i(MP_FUNC, " --> got (0x%pp)->refcnt=%d",
>> >> \
>> >> + interp, interp->refcnt);
>> >> \
>> >> + aTHX = interp->perl;
>> >> \
>> >> + }
>> >> \
>> >> + NOOP
>> >>
>> >> #define MP_dINTERP_POOLa(p, s)
>> >> \
>> >> MP_dINTERP;
>> >> \
>> >> Index: src/modules/perl/modperl_util.c
>> >> ===================================================================
>> >> --- src/modules/perl/modperl_util.c (revision 1539262)
>> >> +++ src/modules/perl/modperl_util.c (working copy)
>> >> @@ -989,8 +989,11 @@
>> >> int count;
>> >> void *key;
>> >> auth_callback *ab;
>> >> - MP_dINTERP_POOLa(cmd->server->process->pool, cmd->server);
>> >> + MP_dINTERP_POOLa(cmd->pool, cmd->server);
>> >>
>> >> + if (!interp)
>> >> + return ret;
>> >> +
>> >> if (global_authz_providers == NULL) {
>> >> MP_INTERP_PUTBACK(interp, aTHX);
>> >> return ret;
>> >
>> >
>> > I don't think it will ever have an interpreter except when processing a
>> > require from htaccess. Still, it doesn't crash, and I get the same test
>> > results as in my prior post.
>> >
>> > I think this should trigger an error if we get past these checks with no
>> > interpreter:
>> >
>> > if (global_authz_providers == NULL ||
>> > apr_hash_count(global_authz_providers)
>> > == 0) {
>> > /* put back an interpreter if we got one */
>> > return ret; /* still NULL; why not make it obvious? */
>> > }
>> >
>> > apr_pool_userdata_get(&key, AUTHZ_PROVIDER_NAME_NOTE, cmd->temp_pool);
>> > ab = apr_hash_get(global_authz_providers, (char *) key,
>> > APR_HASH_KEY_STRING);
>> > if (ab == NULL || ab->cb2 == NULL) {
>> > /* put back an interpreter if we got one */
>> > return ret; /* still NULL; why not make it obvious? */
>> > }
>> >
>> > if (!interp) {
>> > return "Require handler is not currently supported in this context."
>> > }
>> >
>> > And the ordering issue (create interpreter vs. handle Require parsing)
>> > still
>> > needs to be resolved.
>>
>> I've committed my change to make the macro more robust, plus your
>> change to delay trying to fetch an interpreter -- with an early return
>> if it still fails -- in revisions 1539412 and 1539414. Thanks for your
>> help with this!
>
>
> Cool!
>
> BTW, to return an error from a require line parser, you actually return the
> error string (like a directive parser). Here's an example of one from
> mod_ssl that just ensures there are no arguments:
>
> static const char *ssl_authz_require_ssl_parse(cmd_parms *cmd,
> const char *require_line,
> const void **parsed)
> {
> if (require_line && require_line[0])
> return "'Require ssl' does not take arguments";
>
> return NULL;
> }
>
> So returning the error string in the unhandled case (instead of NULL) would
> probably be good.
Thanks, I hadn't realized that. I thought this was just pseudo-code
when you posted this earlier!
Now done in r1539487.
>
> BUT:
>
>>
>> I don't know what to do about trying to fix it for real (i.e. the
>> ordering problem that you refer to). I'm hoping someone else on the
>> list might be able to help?
>
>
> I'm trying to follow this through from a directive like in the testcase:
>
> PerlAddAuthzProvider my-user TestAPI::access2_24->authz_handler
>
> mod_perl.c has
>
> MP_CMD_SRV_TAKE2("PerlAddAuthzProvider", authz_provider,
> "PerlAddAuthzProvider"),
>
> authz_provider takes exactly 2 arguments (or httpd will trigger an error).
> The first argument (like "my-user") is name and the second argument (like
> "TestAPI::access2_24->authz_handler") is cb in the following call:
>
> modperl_register_auth_provider_name(p, AUTHZ_PROVIDER_GROUP, name,
> AUTHZ_PROVIDER_VERSION, cb, NULL,
> AP_AUTH_INTERNAL_PER_CONF);
>
> in modperl_register_auth_provider(), cb and the following NULL are callback1
> and callback2, and we have
>
> ab->cb1 = callback1;
> ab->cb2 = callback2;
>
> return register_auth_provider(pool, provider_group, provider_name_dup,
> provider_version, ab, type);
>
> (callback2 always NULL for an authz provider)
>
> register_auth_provider(), for an authz provider like this, stores ab (the
> two callbacks, one always NULL), in the global_authz_providers hash then
> calls the httpd API to point to authz_perl_provider for the authz provider
> with this name (e.g., "my-user").
>
> authz_perl_provider is the thing that says call perl_parse_require_line;
> perl_parse_require_line is our new friend that tries real hard to get an
> interpreter in case cb2 is non-NULL, which it never will be, until
> PerlAddAuthzProvider is updated to allow an optional 2nd handler which would
> be called at init to check a require line for errors.
>
> Similarly it would seem that the authn variant, for which a second handler
> would have the task of obtaining a password hash for the realm, also cannot
> be configured.
>
> From the application/script standpoint, I think the drawback of not being
> able to provide a require line parser is that I'd be required to look at it
> a steady state and possibly encounter configuration errors that could have
> been reported at startup (a.k.a. not the end of the world; nothing really
> to parse for some authz providers).
Many thanks for the analysis. I've added a comment summarizing this in
the same revision as above (r1539487).
I'm now intending to make similar changes for the PerlAddAuthnProvider
case to protect that against a future crash if support for the second
handler is ever added there too. Does the following look OK? In
particular, is it correct to be returning AUTH_GENERAL_ERROR, or
should it be AUTH_USER_NOT_FOUND like the existing early returns are?
Index: src/modules/perl/modperl_util.c
===================================================================
--- src/modules/perl/modperl_util.c (revision 1539487)
+++ src/modules/perl/modperl_util.c (working copy)
@@ -1116,52 +1116,67 @@
const char *realm, char **rethash)
{
authn_status ret = AUTH_USER_NOT_FOUND;
- int count;
- SV *rh;
const char *key;
auth_callback *ab;
- MP_dINTERPa(r, NULL, NULL);
- if (global_authn_providers == NULL) {
- MP_INTERP_PUTBACK(interp, aTHX);
- return ret;
+ if (global_authn_providers == NULL ||
+ apr_hash_count(global_authn_providers) == 0)
+ {
+ return AUTH_USER_NOT_FOUND;
}
key = apr_table_get(r->notes, AUTHN_PROVIDER_NAME_NOTE);
ab = apr_hash_get(global_authn_providers, key, APR_HASH_KEY_STRING);
if (ab == NULL || ab->cb2) {
- MP_INTERP_PUTBACK(interp, aTHX);
- return ret;
+ return AUTH_USER_NOT_FOUND;
}
- rh = sv_2mortal(newSVpv("", 0));
{
- dSP;
- ENTER;
- SAVETMPS;
- PUSHMARK(SP);
- XPUSHs(sv_2mortal(modperl_ptr2obj(aTHX_ "Apache2::RequestRec", r)));
- XPUSHs(sv_2mortal(newSVpv(user, 0)));
- XPUSHs(sv_2mortal(newSVpv(realm, 0)));
- XPUSHs(newRV_noinc(rh));
- PUTBACK;
- count = call_sv(ab->cb2, G_SCALAR);
- SPAGAIN;
+ /* PerlAddAuthnProvider currently does not support an optional second
+ * handler, so ab->cb2 should always be NULL above and we
will never get
+ * here. If such support is added in the future then this code will be
+ * reached, but cannot succeed in the absence of an interpreter. The
+ * second handler would be called at init to obtain a password hash for
+ * the realm, but in the current design there is no
interpreter available
+ * at that time.
+ */
+ MP_dINTERPa(r, NULL, NULL);
+ if (!interp) {
+ return AUTH_GENERAL_ERROR;
+ }
- if (count == 1) {
- const char *tmp = SvPV_nolen(rh);
- ret = (authn_status) POPi;
- if (*tmp != '\0') {
- *rethash = apr_pstrdup(r->pool, tmp);
+ {
+ SV* rh = sv_2mortal(newSVpv("", 0));
+ int count;
+ dSP;
+
+ ENTER;
+ SAVETMPS;
+ PUSHMARK(SP);
+ XPUSHs(sv_2mortal(modperl_ptr2obj(aTHX_
"Apache2::RequestRec", r)));
+ XPUSHs(sv_2mortal(newSVpv(user, 0)));
+ XPUSHs(sv_2mortal(newSVpv(realm, 0)));
+ XPUSHs(newRV_noinc(rh));
+ PUTBACK;
+ count = call_sv(ab->cb2, G_SCALAR);
+ SPAGAIN;
+
+ if (count == 1) {
+ const char *tmp = SvPV_nolen(rh);
+ ret = (authn_status) POPi;
+ if (*tmp != '\0') {
+ *rethash = apr_pstrdup(r->pool, tmp);
+ }
}
+
+ PUTBACK;
+ FREETMPS;
+ LEAVE;
}
- PUTBACK;
- FREETMPS;
- LEAVE;
+ MP_INTERP_PUTBACK(interp, aTHX);
}
- MP_INTERP_PUTBACK(interp, aTHX);
return ret;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org
For additional commands, e-mail: dev-h...@perl.apache.org
