On 11/07/2013 09:53 AM, Steve Hay wrote:
On 7 November 2013 06:48, Jan Kaluža <jkal...@redhat.com> wrote:
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.
r1538005.
As explained here:
http://permalink.gmane.org/gmane.comp.apache.mod-perl.devel/10353
r1503193 made perl_parse_require_line() tolerant of
modperl_interp_pool_select() returning NULL, so that
perl_parse_require_line() just returns NULL too in that case, but in
r1538005 I changed the code that selects the interpreter to use
MP_dINTERP_POOLa(), which crashes if modperl_interp_pool_select()
returns NULL.
r1539412/1539414/1539487 restore graceful handling of
modperl_interp_pool_select() returning NULL in
perl_parse_require_line(). I'm just thinking that
perl_get_realm_hash() could do with the same changes since as Jeff
explained above it seems to suffer the same problem as
perl_parse_require_line().
I don't see any tests that cover PerlAddAuthnProvider, though, hence
the authn case has not actually caused any crashes in testing. Also,
r1539412 will stop MP_dINTERPa() from crashing if
modperl_interp_select() returns NULL anyway, so as with the authz
case, we're now only talking about a possible future crash if
PerlAddAuthnProvider is ever enhanced to accept the second handler
[i.e. provide non-NULL cb2 in the
modperl_register_auth_provider_name() calls in modperl_cmd.c] -- in
that case, the call_sv(ab->cb2, G_SCALAR) will be reached [it never is
at the moment] in perl_get_realm_hash() and it will crash if interp is
NULL.
Thanks for this explanation. I wrote that code so long ago that I was
actually surprised I'm the author :D.
I'm now wondering if that's really likely, though? - in
perl_parse_require_line() it would happen because this second handler
is an init-time parser of the Require line and there seems to be no
interpreter available at this time in the current design. In the authn
case, the second handler obtains a password hash for the realm; does
that also happen at init-time? If so then it will fail for the same
reason (no interpreter available yet), but if it isn't run until later
then it should work.
When is the second handler called in the authn case?
I've checked get_realm_hash() calls in httpd and it seems they are all
called during request_rec processing, so it should be OK.
Jan Kaluza
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@perl.apache.org
For additional commands, e-mail: dev-h...@perl.apache.org
