perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steve Hay <steve.m....@googlemail.com>
Subject Re: need sanity check on my mod_perl + 2.4.6 on Windows attempt
Date Thu, 07 Nov 2013 18:26:19 GMT
On 7 November 2013 17:19, Jeff Trawick <trawick@gmail.com> wrote:
> A few comments down below...

Thanks. I've applied a modified version of my original patch in
r1539746. I hope I've taken on board all your comments.



>
> On Wed, Nov 6, 2013 at 5:53 PM, Steve Hay <steve.m.hay@googlemail.com>
> 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).
>>
>> 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;
>
>
> This is a should-not-occur path.  We shouldn't have been called here without
> us being able to find our configuration.
>
> return AUTH_GENERAL_ERROR;
>
>
>>
>>      }
>>
>>      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) {
>
>
> shouldn't that be "ab == NULL || !ab->cb2" ?
>
> ab == NULL is also a should not occur, right? not a provider we know about,
> so how would we be called?
>
>
>>
>> -        MP_INTERP_PUTBACK(interp, aTHX);
>> -        return ret;
>> +        return AUTH_USER_NOT_FOUND;
>
>
> I believe this is correct; it seems to be handled reasonably by
> mod_auth_digest and mod_auth_basic+AuthBasicUseDigestAlgorithm
>
> But if it were me, I wouldn't even tell them I have a realm hash function
> until there is support for providing a Perl handler for that.
>
>
>>      }
>>
>> -    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;
>>  }
>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/

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


Mime
View raw message