You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Torsten Foertsch <to...@gmx.net> on 2008/02/18 22:00:40 UTC
$r->location bug
Hi,
modperl_config_dir_create begins with these lines:
void *modperl_config_dir_create(apr_pool_t *p, char *dir)
{
modperl_config_dir_t *dcfg = modperl_config_dir_new(p);
dcfg->location = dir;
While dcfg is created anew the dir pointer is simply stored. This means the
lifetime of dcfg may exceed the lifetime of the dir pointer. In fact I have
found the bug because I got rubbish from $r->location in the response phase
after the response handler was added via $r->add_config in the map2storage
phase.
Here is a patch and a test case.
Torsten
Index: src/modules/perl/modperl_config.c
===================================================================
--- src/modules/perl/modperl_config.c (revision 35)
+++ src/modules/perl/modperl_config.c (working copy)
@@ -20,7 +20,7 @@
{
modperl_config_dir_t *dcfg = modperl_config_dir_new(p);
- dcfg->location = dir;
+ dcfg->location = dir ? apr_pstrdup(p, dir) : NULL;
MP_TRACE_d(MP_FUNC, "dir %s", dir);
Index: t/response/TestAPI/add_config.pm
===================================================================
--- t/response/TestAPI/add_config.pm (revision 31)
+++ t/response/TestAPI/add_config.pm (working copy)
@@ -59,6 +59,14 @@
};
$r->pnotes(followsymlinks => "$@");
+ eval {
+ my $path="/a/path/to/somewhere";
+ $r->add_config(['PerlResponseHandler '.__PACKAGE__], -1, $path);
+ # now overwrite the path in place to see if the location pointer
+ # is really copied: see modperl_config_dir_create
+ $path=~tr[a-z][n-za-m];
+ };
+
return Apache2::Const::DECLINED;
}
@@ -83,7 +91,7 @@
my ($self, $r) = @_;
my $cf = $self->get_config($r->server);
- plan $r, tests => 8;
+ plan $r, tests => 9;
ok t_cmp $r->pnotes('add_config1'), qr/.+\n/;
ok t_cmp $r->pnotes('add_config2'), (APACHE22 ? qr/.+\n/ : '');
@@ -103,6 +111,8 @@
my $opts = APACHE22 ? Apache2::Const::OPT_SYM_LINKS : $expect;
ok t_cmp $r->allow_override_opts, $opts;
+ ok t_cmp $r->location, '/a/path/to/somewhere';
+
return Apache2::Const::OK;
}
@@ -118,7 +128,6 @@
<Directory @DocumentRoot@>
AllowOverride All
</Directory>
- PerlResponseHandler TestAPI::add_config
PerlMapToStorageHandler TestAPI::add_config::map2storage
PerlFixupHandler TestAPI::add_config::fixup
</VirtualHost>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: $r->location bug
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Torsten Foertsch wrote:
> On Fri 22 Feb 2008, Philippe M. Chiasson wrote:
>> - parms.path = path;
>> + parms.path = apr_pstrdup(p, path);
>
> Yes, Philippe, you are right.
>
> In current modperl there are 3 ways to get to modperl_config_insert():
>
> - $r->add_config via modperl_config_insert_request
> - $s->add_config via modperl_config_insert_server
> - $parms->add_config via modperl_config_insert_parms
>
> Since $parms->path is read-only and modperl_config_insert_server passes a NULL
> as path there is only one way to have a custom path passed to
> modperl_config_insert, modperl_config_insert_request. So patching
> $r->add_config only would be sufficient, as well.
Good digging, and correct.
> Nevertheless, fixing it in modperl_config_insert is probably the best way
> regarding future changes.
Yes, it's generally safer to fix it at the source of the problem, as opposed
to hoping folks don't make the same mistake in yet unwritten code.
Committed revision 630597.
--
Philippe M. Chiasson GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/
Re: $r->location bug
Posted by Torsten Foertsch <to...@gmx.net>.
On Fri 22 Feb 2008, Philippe M. Chiasson wrote:
> - parms.path = path;
> + parms.path = apr_pstrdup(p, path);
Yes, Philippe, you are right.
In current modperl there are 3 ways to get to modperl_config_insert():
- $r->add_config via modperl_config_insert_request
- $s->add_config via modperl_config_insert_server
- $parms->add_config via modperl_config_insert_parms
Since $parms->path is read-only and modperl_config_insert_server passes a NULL
as path there is only one way to have a custom path passed to
modperl_config_insert, modperl_config_insert_request. So patching
$r->add_config only would be sufficient, as well.
Nevertheless, fixing it in modperl_config_insert is probably the best way
regarding future changes.
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: $r->location bug
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
Torsten Foertsch wrote:
> On Mon 18 Feb 2008, Torsten Foertsch wrote:
>> Here is a patch and a test case.
>
> Forget the patch. It fixes the wrong place. The only other directive (that I
> know of) that uses the location path ProxyPassReverse also assumes that the
> pointer is valid for the whole request.
>
> So the correct place to approach is our $r->add_config.
I don't understand how this is fixing the problem cleanly. What seems
to be the culprit is modperl_config_insert() keeping a hold of the path
longer than it might live. Wouldn't this patch solve the problem as
well, and more directly ?
Index: src/modules/perl/modperl_config.c
===================================================================
--- src/modules/perl/modperl_config.c (revision 629347)
+++ src/modules/perl/modperl_config.c (working copy)
@@ -518,7 +518,7 @@
parms.limited = -1;
parms.server = s;
parms.override = override;
- parms.path = path;
+ parms.path = apr_pstrdup(p, path);
parms.pool = p;
#ifdef MP_HTTPD_HAS_OVERRIDE_OPTS
if (override_options == MP_HTTPD_OVERRIDE_OPTS_UNSET) {
--
Philippe M. Chiasson GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/
Re: $r->location bug
Posted by Torsten Foertsch <to...@gmx.net>.
On Thu 21 Feb 2008, Fred Moyer wrote:
> patching file src/modules/perl/modperl_config.c
> Hunk #1 succeeded at 607 with fuzz 1 (offset -17 lines).
I am working with the threading branch. So there may be an offset. Also, some
time ago I had to split up a jumbo patch. Since the bits built up on top of
one another I found it really hard to do it without write permission to the
SVN. The first part could be produced by svn diff. But then I had to wait
until someone had checked that in to do the next svn diff. That was too long.
So I set up my own SVN. Now I am working with it. As you might have noticed
my current revision is 36. I am not happy with that because it creates more
work for me. But for now I hold to it because it is a tool that helps. I had
published my patchset on Oct 23/24. It was applied to the threading branch
mostly on Nov 13, that is 2/3 of a month later.
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: $r->location bug
Posted by Fred Moyer <fr...@taperfriendlymusic.org>.
> On Mon 18 Feb 2008, Torsten Foertsch wrote:
>> Here is a patch and a test case.
>
> Forget the patch. It fixes the wrong place. The only other directive (that
> I
> know of) that uses the location path ProxyPassReverse also assumes that
> the
> pointer is valid for the whole request.
>
> So the correct place to approach is our $r->add_config.
Just tried this patch:
patching file src/modules/perl/modperl_config.c
Hunk #1 succeeded at 607 with fuzz 1 (offset -17 lines).
My modperl_config.c is at revision 29971 - do you have an old version of
this file?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: $r->location bug
Posted by Torsten Foertsch <to...@gmx.net>.
On Mon 18 Feb 2008, Torsten Foertsch wrote:
> Here is a patch and a test case.
Forget the patch. It fixes the wrong place. The only other directive (that I
know of) that uses the location path ProxyPassReverse also assumes that the
pointer is valid for the whole request.
So the correct place to approach is our $r->add_config.
Torsten