perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: need sanity check on my mod_perl + 2.4.6 on Windows attempt
Date Wed, 06 Nov 2013 19:06:45 GMT
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.

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



>
>
> >
> > I guess TestAPI::access2_24.pm needs more code to declare a require
> parser.
> > Any idea how to do that, just to see something found in the hash?
>
> Sorry, I don't know that right now either, but I will look into it too.
>

After looking, I think there's no way (see above).

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Mime
View raw message