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.
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?
>
> 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
