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