perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Kaluža <jkal...@redhat.com>
Subject Re: need sanity check on my mod_perl + 2.4.6 on Windows attempt
Date Thu, 07 Nov 2013 10:03:01 GMT
On 11/07/2013 09:53 AM, Steve Hay wrote:
> On 7 November 2013 06:48, Jan Kaluža <jkaluza@redhat.com> wrote:
>> On 11/06/2013 11:53 PM, Steve Hay wrote:
>>>
>>> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>>>>
>>>> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Nov 6, 2013 at 8:50 AM, Steve Hay <steve.m.hay@googlemail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6 November 2013 13:50, Steve Hay <steve.m.hay@googlemail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 6 November 2013 12:27, Jeff Trawick <trawick@gmail.com>
wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Nov 6, 2013 at 4:02 AM, Steve Hay
>>>>>>>>> <steve.m.hay@googlemail.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 6 November 2013 00:48, Jeff Trawick <trawick@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.

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-unsubscribe@perl.apache.org
>>> For additional commands, e-mail: dev-help@perl.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
>> For additional commands, e-mail: dev-help@perl.apache.org
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org


Mime
View raw message