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 14:27:24 GMT
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 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?

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

Mime
View raw message