On Thu, Nov 7, 2013 at 5:03 AM, Jan Kaluža <jkal...@redhat.com> wrote:
> On 11/07/2013 09:53 AM, Steve Hay wrote: > >> On 7 November 2013 06:48, Jan Kaluža <jkal...@redhat.com> wrote: >> >>> On 11/06/2013 11:53 PM, Steve Hay wrote: >>> >>>> >>>> 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). >>>> >>> >>> >>> Can you point me to commit which caused this part of code you are >>> patching >>> below to not work? It was working in httpd24 branch before merging with >>> threading branch, so I would like to check it out. >>> >> >> r1538005. >> >> As explained here: >> >> http://permalink.gmane.org/gmane.comp.apache.mod-perl.devel/10353 >> >> r1503193 made perl_parse_require_line() tolerant of >> modperl_interp_pool_select() returning NULL, so that >> perl_parse_require_line() just returns NULL too in that case, but in >> r1538005 I changed the code that selects the interpreter to use >> MP_dINTERP_POOLa(), which crashes if modperl_interp_pool_select() >> returns NULL. >> >> r1539412/1539414/1539487 restore graceful handling of >> modperl_interp_pool_select() returning NULL in >> perl_parse_require_line(). I'm just thinking that >> perl_get_realm_hash() could do with the same changes since as Jeff >> explained above it seems to suffer the same problem as >> perl_parse_require_line(). >> >> I don't see any tests that cover PerlAddAuthnProvider, though, hence >> the authn case has not actually caused any crashes in testing. Also, >> r1539412 will stop MP_dINTERPa() from crashing if >> modperl_interp_select() returns NULL anyway, so as with the authz >> case, we're now only talking about a possible future crash if >> PerlAddAuthnProvider is ever enhanced to accept the second handler >> [i.e. provide non-NULL cb2 in the >> modperl_register_auth_provider_name() calls in modperl_cmd.c] -- in >> that case, the call_sv(ab->cb2, G_SCALAR) will be reached [it never is >> at the moment] in perl_get_realm_hash() and it will crash if interp is >> NULL. >> > > Thanks for this explanation. I wrote that code so long ago that I was > actually surprised I'm the author :D. > > > I'm now wondering if that's really likely, though? - in >> perl_parse_require_line() it would happen because this second handler >> is an init-time parser of the Require line and there seems to be no >> interpreter available at this time in the current design. In the authn >> case, the second handler obtains a password hash for the realm; does >> that also happen at init-time? If so then it will fail for the same >> reason (no interpreter available yet), but if it isn't run until later >> then it should work. >> >> When is the second handler called in the authn case? >> > > I've checked get_realm_hash() calls in httpd and it seems they are all > called during request_rec processing, so it should be OK. > yeah, totally different kind of callback > > Jan Kaluza > > > >> >> >>> Jan Kaluza >>> >>> 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 >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org >>> For additional commands, e-mail: dev-h...@perl.apache.org >>> >>> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org > For additional commands, e-mail: dev-h...@perl.apache.org > > -- Born in Roswell... married an alien... http://emptyhammock.com/