On 11/06/2013 11:53 PM, Steve Hay 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).
Can you point me to commit which caused this part of code you are
patching below to not work? It was working in httpd24 branch before
merging with threading branch, so I would like to check it out.
Jan Kaluza
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;
}
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) {
- MP_INTERP_PUTBACK(interp, aTHX);
- return ret;
+ return AUTH_USER_NOT_FOUND;
}
- 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;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org
For additional commands, e-mail: dev-h...@perl.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org
For additional commands, e-mail: dev-h...@perl.apache.org
