A few comments down below...
On Wed, Nov 6, 2013 at 5:53 PM, Steve Hay <steve.m....@googlemail.com>wrote:
> On 6 November 2013 19:06, Jeff Trawick <traw...@gmail.com> wrote:
> > On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m....@googlemail.com>
> > wrote:
> >>
> >> 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!
> >
> >
> > 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/
