Mailing List Archive

On 5 November 2013 16:53, Jeff Trawick <trawick@gmail.com> wrote:
> * Have existing 64-bit debug build of httpd/apr/etc. with VS 2012
> * Checked out httpd24threading branch of mod_perl
> * Have ActiveState 64-bit build of Perl 5.16.3 in PATH
>
> From a VS 2012 64-bit command prompt:
>
>> set MP_USE_MY_EXTUTILS_EMBED=1
> (ActiveState ppm doesn't show the exact version requested as available.)
>
>> perl Makefile.PL MP_AP_PREFIX=%HOME%\PREFIXES\241
> (and let it download apxs.bat et al)
>> nmake
>> nmake install
>> nmake test
>
> ==> crash at httpd startup
>
> If I believe the Visual Studio debugger, it fails with a very basic linkage
> issue -- mod_authz_core.so!add_authz_provider has non-NULL cmd_parms; it
> calls perl_parse_require_line(), which the debugger believes has a NULL
> cmd_parms. And indeed it crashes at a place in modperl_interp_pool_select()
> where a NULL pointer would be used, after making a bad decision based on the
> NULL cmd_parms.
>
> Any guess on what to change first?
>

Using the 2.4.6 debug build which I just made (and currently working
with a 5.18.1 debug perl build) I now have a working system! The
KeepAliveTimeout problem has gone away, and so has my earlier problem
of the api/add_config.t test crashing httpd.exe.

Your build notes look fine. The crash you have sounds like the same as
the call stack posted by Jan Kaluza (on Linux), but I'm not seeing
that myself right now.

The function your crash is in was new in the httpd24 branch, and it's
one of things that I had to sort out when merging the threading branch
into it. The threading branch changes the way that perl interpreters
are managed in mod_perl, and I've tried to apply those changes to the
functions since added in the httpd24 branch, but I've quite possibly
not got it all right.

What happens if you try the httpd24 branch? That worked (bar a few
test failures) for Jan on Linux, but for me on Windows it crashed
httpd.exe in api/add_config.t, with an error relating to interpreters
and memory pools -- which is why I went down the road of merging the
threading branch with the httpd24 branch, in the hope that it would
fix my problem.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On Tue, Nov 5, 2013 at 1:29 PM, Steve Hay <steve.m.hay@googlemail.com>wrote:

> On 5 November 2013 16:53, Jeff Trawick <trawick@gmail.com> wrote:
> > * Have existing 64-bit debug build of httpd/apr/etc. with VS 2012
> > * Checked out httpd24threading branch of mod_perl
> > * Have ActiveState 64-bit build of Perl 5.16.3 in PATH
> >
> > From a VS 2012 64-bit command prompt:
> >
> >> set MP_USE_MY_EXTUTILS_EMBED=1
> > (ActiveState ppm doesn't show the exact version requested as available.)
> >
> >> perl Makefile.PL MP_AP_PREFIX=%HOME%\PREFIXES\241
> > (and let it download apxs.bat et al)
> >> nmake
> >> nmake install
> >> nmake test
> >
> > ==> crash at httpd startup
> >
> > If I believe the Visual Studio debugger, it fails with a very basic
> linkage
> > issue -- mod_authz_core.so!add_authz_provider has non-NULL cmd_parms; it
> > calls perl_parse_require_line(), which the debugger believes has a NULL
> > cmd_parms. And indeed it crashes at a place in
> modperl_interp_pool_select()
> > where a NULL pointer would be used, after making a bad decision based on
> the
> > NULL cmd_parms.
> >
> > Any guess on what to change first?
> >
>
> Using the 2.4.6 debug build which I just made (and currently working
> with a 5.18.1 debug perl build) I now have a working system! The
> KeepAliveTimeout problem has gone away, and so has my earlier problem
> of the api/add_config.t test crashing httpd.exe.
>
> Your build notes look fine. The crash you have sounds like the same as
> the call stack posted by Jan Kaluza (on Linux), but I'm not seeing
> that myself right now.
>
> The function your crash is in was new in the httpd24 branch, and it's
> one of things that I had to sort out when merging the threading branch
> into it. The threading branch changes the way that perl interpreters
> are managed in mod_perl, and I've tried to apply those changes to the
> functions since added in the httpd24 branch, but I've quite possibly
> not got it all right.
>
> What happens if you try the httpd24 branch? That worked (bar a few
> test failures) for Jan on Linux, but for me on Windows it crashed
> httpd.exe in api/add_config.t, with an error relating to interpreters
> and memory pools -- which is why I went down the road of merging the
> threading branch with the httpd24 branch, in the hope that it would
> fix my problem.
>

I get a lot further with the httpd24 branch. Some tests run before a crash
during t\api\add_config.t. (repeatable)

modperl_cleanup_pnotes() crashes on "*pnotes = NULL" when the pool for a
subrequest is destroyed.

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

On Tue, Nov 5, 2013 at 2:01 PM, Jeff Trawick <trawick@gmail.com> wrote:

> On Tue, Nov 5, 2013 at 1:29 PM, Steve Hay <steve.m.hay@googlemail.com>wrote:
>
>> On 5 November 2013 16:53, Jeff Trawick <trawick@gmail.com> wrote:
>> > * Have existing 64-bit debug build of httpd/apr/etc. with VS 2012
>> > * Checked out httpd24threading branch of mod_perl
>> > * Have ActiveState 64-bit build of Perl 5.16.3 in PATH
>> >
>> > From a VS 2012 64-bit command prompt:
>> >
>> >> set MP_USE_MY_EXTUTILS_EMBED=1
>> > (ActiveState ppm doesn't show the exact version requested as available.)
>> >
>> >> perl Makefile.PL MP_AP_PREFIX=%HOME%\PREFIXES\241
>> > (and let it download apxs.bat et al)
>> >> nmake
>> >> nmake install
>> >> nmake test
>> >
>> > ==> crash at httpd startup
>> >
>> > If I believe the Visual Studio debugger, it fails with a very basic
>> linkage
>> > issue -- mod_authz_core.so!add_authz_provider has non-NULL cmd_parms; it
>> > calls perl_parse_require_line(), which the debugger believes has a NULL
>> > cmd_parms. And indeed it crashes at a place in
>> modperl_interp_pool_select()
>> > where a NULL pointer would be used, after making a bad decision based
>> on the
>> > NULL cmd_parms.
>> >
>> > Any guess on what to change first?
>> >
>>
>> Using the 2.4.6 debug build which I just made (and currently working
>> with a 5.18.1 debug perl build) I now have a working system! The
>> KeepAliveTimeout problem has gone away, and so has my earlier problem
>> of the api/add_config.t test crashing httpd.exe.
>>
>> Your build notes look fine. The crash you have sounds like the same as
>> the call stack posted by Jan Kaluza (on Linux), but I'm not seeing
>> that myself right now.
>>
>> The function your crash is in was new in the httpd24 branch, and it's
>> one of things that I had to sort out when merging the threading branch
>> into it. The threading branch changes the way that perl interpreters
>> are managed in mod_perl, and I've tried to apply those changes to the
>> functions since added in the httpd24 branch, but I've quite possibly
>> not got it all right.
>>
>> What happens if you try the httpd24 branch? That worked (bar a few
>> test failures) for Jan on Linux, but for me on Windows it crashed
>> httpd.exe in api/add_config.t, with an error relating to interpreters
>> and memory pools -- which is why I went down the road of merging the
>> threading branch with the httpd24 branch, in the hope that it would
>> fix my problem.
>>
>
> I get a lot further with the httpd24 branch. Some tests run before a
> crash during t\api\add_config.t. (repeatable)
>
> modperl_cleanup_pnotes() crashes on "*pnotes = NULL" when the pool for a
> subrequest is destroyed.
>
>
I see
http://mail-archives.apache.org/mod_mbox/perl-dev/201310.mbox/%3C526FA19B.6050504@redhat.com%3E
though
it isn't clear that it is the same problem.

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

On Tue, Nov 5, 2013 at 11:53 AM, Jeff Trawick <trawick@gmail.com> wrote:

> * Have existing 64-bit debug build of httpd/apr/etc. with VS 2012
> * Checked out httpd24threading branch of mod_perl
> * Have ActiveState 64-bit build of Perl 5.16.3 in PATH
>
> From a VS 2012 64-bit command prompt:
>
> > set MP_USE_MY_EXTUTILS_EMBED=1
> (ActiveState ppm doesn't show the exact version requested as available.)
>
> > perl Makefile.PL MP_AP_PREFIX=%HOME%\PREFIXES\241
> (and let it download apxs.bat et al)
> > nmake
> > nmake install
> > nmake test
>
> ==> crash at httpd startup
>
> If I believe the Visual Studio debugger, it fails with a very basic
> linkage issue -- mod_authz_core.so!add_authz_provider has non-NULL
> cmd_parms; it calls perl_parse_require_line(), which the debugger believes
> has a NULL cmd_parms. And indeed it crashes at a place in
> modperl_interp_pool_select() where a NULL pointer would be used, after
> making a bad decision based on the NULL cmd_parms.
>
> Any guess on what to change first?
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
>

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).

Any idea where to try to hack in some -debug support for "devenv /debugexe"?

Where do I see messages like

MP_TRACE_i(MP_FUNC, "using parent interpreter at startup");

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

On 6 November 2013 00:48, Jeff Trawick <trawick@gmail.com> wrote:
> On Tue, Nov 5, 2013 at 11:53 AM, Jeff Trawick <trawick@gmail.com> wrote:
>>
>> * Have existing 64-bit debug build of httpd/apr/etc. with VS 2012
>> * Checked out httpd24threading branch of mod_perl
>> * Have ActiveState 64-bit build of Perl 5.16.3 in PATH
>>
>> From a VS 2012 64-bit command prompt:
>>
>> > set MP_USE_MY_EXTUTILS_EMBED=1
>> (ActiveState ppm doesn't show the exact version requested as available.)
>>
>> > perl Makefile.PL MP_AP_PREFIX=%HOME%\PREFIXES\241
>> (and let it download apxs.bat et al)
>> > nmake
>> > nmake install
>> > nmake test
>>
>> ==> crash at httpd startup
>>
>> If I believe the Visual Studio debugger, it fails with a very basic
>> linkage issue -- mod_authz_core.so!add_authz_provider has non-NULL
>> cmd_parms; it calls perl_parse_require_line(), which the debugger believes
>> has a NULL cmd_parms. And indeed it crashes at a place in
>> modperl_interp_pool_select() where a NULL pointer would be used, after
>> making a bad decision based on the NULL cmd_parms.
>>
>> Any guess on what to change first?
>>
>> --
>> Born in Roswell... married an alien...
>> http://emptyhammock.com/
>
>
> 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).
>
> Any idea where to try to hack in some -debug support for "devenv /debugexe"?

Perl XS extensions (like mod_perl) pick up compile and link flags from
perl itself. If you're using ActivePerl then they'll be set to
"release with debug info" flags. I normally get a debug build by
building my own perl:

cd win32
nmake CFG=Debug INST_TOP=/path/to/install/into
nmake CFG=Debug INST_TOP=/path/to/install/into installbare

(See win32/Makefile for other settings).

You might be able to get mod_perl to build in debug mode with
ActivePerl by editing ActivePerl's lib/Config_heavy.pl to change
ccflags and lddlflags appropriately (perl uses compile / link flags O1
-MD -Zi -DNDEBUG / -debug -opt:ref,icf for release builds vs. -Od -MD
-Zi -DDEBUGGING / -debug for debug builds), but I've never tried doing
that myself -- I sometimes find I need perl built in debug mode to get
the full picture anyway, so that's what I do.


>
> Where do I see messages like
>
> MP_TRACE_i(MP_FUNC, "using parent interpreter at startup");
>

When you build mod_perl, specify "MP_TRACE=1" on the Makefie.PL
command-line. Then you can use the PerlTrace directive in httpd.conf
and/or the MOD_PERL_TRACE environment variable to set various
different modes as documented here:

http://perl.apache.org/docs/2.0/user/config/config.html#C_PerlTrace_

The output goes to STDERR (i.e. logs/error_log).

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On 6 November 2013 08:44, Steve Hay <steve.m.hay@googlemail.com> wrote:

> Perl XS extensions (like mod_perl) pick up compile and link flags from
> perl itself. If you're using ActivePerl then they'll be set to
> "release with debug info" flags. I normally get a debug build by
> building my own perl:
>
> cd win32
> nmake CFG=Debug INST_TOP=/path/to/install/into
> nmake CFG=Debug INST_TOP=/path/to/install/into installbare
>
> (See win32/Makefile for other settings).
>

Ah, I should add that you'll want to set CCTYPE=MSVC110 for VC++ 2012,
and you'll need at least Win32::Process installed into your new perl
if you go down that route too. That's the only other module *required*
by mod_perl on Windows (for running the test suite).

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

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...

(Not sure how you break into things in your debugger, btw. I find the
easiest way is to start up devenv.exe with "devenv /useenv httpd.exe"
and then set the command-line arguments in the httpd project to those
that "t\TEST -start-httpd" would use, namely something like "-d
C:/Dev/Temp/modperl-httpd24threading/t -f
C:/Dev/Temp/modperl-httpd24threading/t/conf/httpd.conf -D APACHE2 -D
PERL_USEITHREADS -D ONE_PROCESS". Then you can just hit F5 etc to set
it running.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

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.


> (Not sure how you break into things in your debugger, btw. I find the
> easiest way is to start up devenv.exe with "devenv /useenv httpd.exe"
> and then set the command-line arguments in the httpd project to those
> that "t\TEST -start-httpd" would use, namely something like "-d
> C:/Dev/Temp/modperl-httpd24threading/t -f
> C:/Dev/Temp/modperl-httpd24threading/t/conf/httpd.conf -D APACHE2 -D
> PERL_USEITHREADS -D ONE_PROCESS". Then you can just hit F5 etc to set
> it running.)
>

Yeah, that was one of the first things I thought of staring at the ceiling
this a.m. Thanks for the shortcut! No need to chase down the -debug code.

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

On Wed, Nov 6, 2013 at 3:47 AM, Steve Hay <steve.m.hay@googlemail.com>wrote:

> On 6 November 2013 08:44, Steve Hay <steve.m.hay@googlemail.com> wrote:
>
> > Perl XS extensions (like mod_perl) pick up compile and link flags from
> > perl itself. If you're using ActivePerl then they'll be set to
> > "release with debug info" flags. I normally get a debug build by
> > building my own perl:
> >
> > cd win32
> > nmake CFG=Debug INST_TOP=/path/to/install/into
> > nmake CFG=Debug INST_TOP=/path/to/install/into installbare
> >
> > (See win32/Makefile for other settings).
> >
>
> Ah, I should add that you'll want to set CCTYPE=MSVC110 for VC++ 2012,
> and you'll need at least Win32::Process installed into your new perl
> if you go down that route too. That's the only other module *required*
> by mod_perl on Windows (for running the test suite).
>


With the ActiveState Perl, MP_DEBUG=on seems to do the right thing on the
mod_perl build.

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

On Wed, Nov 6, 2013 at 7:27 AM, 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.
>

I don't think an interpreter has been created at the point that the require
line is parsed. (Try setting a breakpoint for modperl_interp_init()). I
see that some command processors call perl_run() to force early
initialization, but if perl_parse_require_line() calls perl_run() there are
problems later when PerlSwitches is encountered ("mod_perl is already
running, too late for PerlSwitches").

If I bypass this whole issue by avoiding this MP_dINTERP_POOLa in the test
configuration (which apparently has no Perl require???), I get lots of
successful tests.

Index: src/modules/perl/modperl_util.c
===================================================================
--- src/modules/perl/modperl_util.c (revision 1539040)
+++ src/modules/perl/modperl_util.c (working copy)
@@ -989,21 +989,26 @@
int count;
void *key;
auth_callback *ab;
- MP_dINTERP_POOLa(cmd->server->process->pool, cmd->server);
+#if 0
+ int ignored_call_before_macro = modperl_run(); /* must init earlier
than
+ * normal */
+#endif

- if (global_authz_providers == NULL) {
- MP_INTERP_PUTBACK(interp, aTHX);
+ if (global_authz_providers == NULL ||
apr_hash_count(global_authz_providers) == 0) {
return ret;
}

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) {
- MP_INTERP_PUTBACK(interp, aTHX);
return ret;
}

{
+ MP_dINTERP_POOLa(cmd->pool, cmd->server);
+
+
+ {
dSP;
ENTER;
SAVETMPS;
@@ -1030,6 +1035,8 @@
}

MP_INTERP_PUTBACK(interp, aTHX);
+ }
+
return ret;
}

Test Summary Report
-------------------
t\apache\subprocess.t (Wstat: 0 Tests: 1 Failed: 0)
Parse errors: Bad plan. You planned 5 tests but ran 1.
t\compat\conn_rec.t (Wstat: 0 Tests: 2 Failed: 0)
Parse errors: Bad plan. You planned 4 tests but ran 2.
t\hooks\authen_digest.t (Wstat: 0 Tests: 7 Failed: 4)
Failed tests: 4-7
t\modperl\interpreter.t (Wstat: 0 Tests: 0 Failed: 0)
Parse errors: Bad plan. You planned 17 tests but ran 0.
t\modperl\local_env.t (Wstat: 0 Tests: 6 Failed: 1)
Failed test: 6
t\modperl\merge.t (Wstat: 0 Tests: 10 Failed: 3)
Failed tests: 3, 6, 9
t\modperl\merge2.t (Wstat: 0 Tests: 10 Failed: 3)
Failed tests: 3, 6, 9
t\modperl\merge3.t (Wstat: 0 Tests: 10 Failed: 3)
Failed tests: 3, 6, 9
t\modules\cgi.t (Wstat: 0 Tests: 5 Failed: 5)
Failed tests: 1-5
t\modules\cgi2.t (Wstat: 0 Tests: 5 Failed: 5)
Failed tests: 1-5
t\modules\cgipost.t (Wstat: 0 Tests: 6 Failed: 5)
Failed tests: 2-6
t\modules\cgipost2.t (Wstat: 0 Tests: 6 Failed: 5)
Failed tests: 2-6
t\modules\cgiupload.t (Wstat: 0 Tests: 2 Failed: 2)
Failed tests: 1-2
t\modules\cgiupload2.t (Wstat: 0 Tests: 2 Failed: 2)
Failed tests: 1-2
t\protocol\echo_block.t (Wstat: 0 Tests: 3 Failed: 2)
Failed tests: 2-3
t\protocol\echo_nonblock.t (Wstat: 0 Tests: 3 Failed: 1)
Failed test: 2
t\protocol\echo_timeout.t (Wstat: 0 Tests: 5 Failed: 4)
Failed tests: 2-5
t\protocol\pseudo_http.t (Wstat: 0 Tests: 13 Failed: 9)
Failed tests: 3-8, 11-13
Files=252, Tests=2585, 606 wallclock secs ( 1.48 usr + 0.97 sys = 2.45
CPU)
Result: FAIL
Failed 18/252 test programs. 54/2585 subtests failed.
[warning] server WIN-MOEO9JF0AKP:8529 shutdown
[ error] error running tests (please examine t\logs\error_log)

Hopefully somebody that knows a lot about mod_perl [init] and the threading
changes can think about the ordering issue with require???




>
>> (Not sure how you break into things in your debugger, btw. I find the
>> easiest way is to start up devenv.exe with "devenv /useenv httpd.exe"
>> and then set the command-line arguments in the httpd project to those
>> that "t\TEST -start-httpd" would use, namely something like "-d
>> C:/Dev/Temp/modperl-httpd24threading/t -f
>> C:/Dev/Temp/modperl-httpd24threading/t/conf/httpd.conf -D APACHE2 -D
>> PERL_USEITHREADS -D ONE_PROCESS". Then you can just hit F5 etc to set
>> it running.)
>>
>
> Yeah, that was one of the first things I thought of staring at the ceiling
> this a.m. Thanks for the shortcut! No need to chase down the -debug code.
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/
>



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

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.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

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;

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

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/

On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
> 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'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-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>wrote:

> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
> > 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'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.

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).



>
>
> >
> > 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.
>

After looking, I think there's no way (see above).

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

On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
> wrote:
>>
>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>> > 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'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;
}

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

On 11/06/2013 11:53 PM, Steve Hay wrote:
> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>> wrote:
>>>
>>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>>>> 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'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-unsubscribe@perl.apache.org
> For additional commands, e-mail: dev-help@perl.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On 7 November 2013 06:48, Jan Kaluža <jkaluza@redhat.com> wrote:
> On 11/06/2013 11:53 PM, Steve Hay wrote:
>>
>> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>>>
>>> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>>> wrote:
>>>>
>>>>
>>>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>>>>>
>>>>> 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'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.

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?


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

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On 11/07/2013 09:53 AM, Steve Hay wrote:
> On 7 November 2013 06:48, Jan Kaluža <jkaluza@redhat.com> wrote:
>> On 11/06/2013 11:53 PM, Steve Hay wrote:
>>>
>>> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>>>>
>>>> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>>>>>>
>>>>>> 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'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-unsubscribe@perl.apache.org
>>> For additional commands, e-mail: dev-help@perl.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
>> For additional commands, e-mail: dev-help@perl.apache.org
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org

On Thu, Nov 7, 2013 at 5:03 AM, Jan Kaluža <jkaluza@redhat.com> wrote:

> On 11/07/2013 09:53 AM, Steve Hay wrote:
>
>> On 7 November 2013 06:48, Jan Kaluža <jkaluza@redhat.com> wrote:
>>
>>> On 11/06/2013 11:53 PM, Steve Hay wrote:
>>>
>>>>
>>>> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>>>>
>>>>>
>>>>> On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>>>>>>
>>>>>>>
>>>>>>> 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'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.
>

yeah, totally different kind of callback


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


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

A few comments down below...

On Wed, Nov 6, 2013 at 5:53 PM, Steve Hay <steve.m.hay@googlemail.com>wrote:

> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
> > On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
> > wrote:
> >>
> >> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
> >> > 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'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/

On 7 November 2013 17:19, Jeff Trawick <trawick@gmail.com> wrote:
> A few comments down below...

Thanks. I've applied a modified version of my original patch in
r1539746. I hope I've taken on board all your comments.



>
> On Wed, Nov 6, 2013 at 5:53 PM, Steve Hay <steve.m.hay@googlemail.com>
> wrote:
>>
>> On 6 November 2013 19:06, Jeff Trawick <trawick@gmail.com> wrote:
>> > On Wed, Nov 6, 2013 at 1:20 PM, Steve Hay <steve.m.hay@googlemail.com>
>> > wrote:
>> >>
>> >> On 6 November 2013 14:27, Jeff Trawick <trawick@gmail.com> wrote:
>> >> > 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'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/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org