You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Stas Bekman <st...@stason.org> on 2005/02/25 23:48:51 UTC
[mp2] issues with certain serverrec setting methods
Last week we have discussed on the modperl list a bug in document_root,
where when setting it, we were just pointing the s->document_root to the
PV slot in the scalar, which was obviously wrong. So it needed to be
copied via pool and restored to the original when that pool went out of
scope (as Joe has suggested).
As at appears there is a bunch of other methods which suffer from a
similar but not indenical problem. Take for example $s->server_admin.
char *
server_admin(obj, val=NULL)
Apache::ServerRec obj
char * val
[...]
RETVAL = (char *) obj->server_admin;
if (items > 1) {
MP_CROAK_IF_THREADS_STARTED("setting server_admin");
obj->server_admin = (char *) val;
}
Here the problem is the same:
obj->server_admin = (char *) val;
is wrong.
But in this case we can't allocate it from the pool, since the object that
we have is server object, and if we allocate from it, we will get a
continuous memory leak. The only solution I can see is the one suggested
earlier by Geoff, which comes from mp1:
SV *server_admin = perl_get_sv("Apache::ServerRec::ServerAdmin", TRUE);
sv_setsv(server_admin, ST(1));
obj->server_admin = SvPVX(serever_admin);
i.e. we create a superficial perl variable to make sure that the value
will stick around if the original scalar is modified or destroyed.
Anybody sees a better solution?
If we go with this one, should we use some obscure name space for this
kind of stashing? We don't really want to support more APIs, do we? and
using the "Apache::ServerRec::ServerAdmin" is something that people will
discover and certainly use. Which of course could be a good thing too, as
it'll be faster, but on the other hand it won't be set unless a user has
modified $s->server_admin. That's why I think it should be hidden into
something like: $_modperl_private::apache_server_admin
Ideas?
Finally if we go with that solution, should we use it for
$r->document_root as well, so it'll behave more like mp1 did? in other
thread we agreed that it's a goodness to restore it at the end of each
request. I think we should stick to that solution.
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2] issues with certain serverrec setting methods
Posted by Stas Bekman <st...@stason.org>.
Stas Bekman wrote:
[...]
> SV *server_admin = perl_get_sv("Apache::ServerRec::ServerAdmin", TRUE);
> sv_setsv(server_admin, ST(1));
> obj->server_admin = SvPVX(serever_admin);
>
> i.e. we create a superficial perl variable to make sure that the value
> will stick around if the original scalar is modified or destroyed.
So the fields that need a fixup are:
Apache::ServerRec: (all autogenerated)
server_admin
server_hostname
error_fname
path
names
wild_names
the following are non-pointer assignments (mostly int), so they need no
backup:
port
loglevel
timeout
keep_alive_timeout
keep_alive_max
keep_alive
limit_req_line
limit_req_fieldsize
limit_req_fields
And here is the patch:
Index: xs/maps/apache_structures.map
===================================================================
--- xs/maps/apache_structures.map (revision 155373)
+++ xs/maps/apache_structures.map (working copy)
@@ -76,10 +76,10 @@
< next
- defn_name
- defn_line_number
-$ server_admin
-$ server_hostname
+% server_admin
+% server_hostname
$ port
-$ error_fname
+% error_fname
$ error_log
$ loglevel
< is_virtual
@@ -90,10 +90,10 @@
$ keep_alive_timeout
$ keep_alive_max
$ keep_alive
-$ path
+% path
- pathlen
-$ names
-$ wild_names
+% names
+% wild_names
$ limit_req_line
$ limit_req_fieldsize
$ limit_req_fields
Index: lib/ModPerl/MapUtil.pm
===================================================================
--- lib/ModPerl/MapUtil.pm (revision 155373)
+++ lib/ModPerl/MapUtil.pm (working copy)
@@ -25,10 +25,13 @@
our @ISA = qw(Exporter);
-# the mapping happens in lib/ModPerl/StructureMap.pm
+# the mapping happens in lib/ModPerl/StructureMap.pm: sub parse
# '<' => 'auto-generated but gives only a read-only access'
# '&' => 'RDWR accessor to a char* field, supporting undef arg'
-# '$' => 'RONLY accessor, with WRITE accessor before child_init'
+# '$' => 'RONLY accessor, with WRITE accessor before child_init'
+# '%' => like $, but makes sure that for the write accessor the
+# original perl scalar can change or go away w/o affecting
+# the object
my %disabled_map = (
'!' => 'disabled or not yet implemented',
'~' => 'implemented but not auto-generated',
Index: lib/ModPerl/WrapXS.pm
===================================================================
--- lib/ModPerl/WrapXS.pm (revision 155376)
+++ lib/ModPerl/WrapXS.pm (working copy)
@@ -256,6 +256,37 @@
EOF
}
+ elsif ($access_mode eq 'r+w_startup_dup') {
+
+ my $convert = $cast !~ /\bchar\b/
+ ? "mp_xs_sv2_$cast"
+ : "SvPV_nolen";
+
+ $code = <<EOF;
+$type
+$name(obj, val=Nullsv)
+ $class obj
+ SV *val
+
+ PREINIT:
+ $preinit
+$attrs
+
+ CODE:
+ RETVAL = ($cast) obj->$name;
+
+ if (items > 1) {
+ SV *dup = get_sv("_modperl_private::server_rec_$name", TRUE);
+ MP_CROAK_IF_THREADS_STARTED("setting $name");
+ sv_setsv(dup, val);
+ obj->$name = ($cast)$convert(dup);
+ }
+
+ OUTPUT:
+ RETVAL
+
+EOF
+ }
elsif ($access_mode eq 'rw_char_undef') {
my $pool = $e->{pool}
or die "rw_char_undef accessors need pool";
Index: lib/ModPerl/StructureMap.pm
===================================================================
--- lib/ModPerl/StructureMap.pm (revision 155373)
+++ lib/ModPerl/StructureMap.pm (working copy)
@@ -128,6 +128,9 @@
elsif ($1 eq '$') {
$map->{$class}->{$_} = 'r+w_startup';
}
+ elsif ($1 eq '%') {
+ $map->{$class}->{$_} = 'r+w_startup_dup';
+ }
}
else {
$map->{$class}->{$_} = undef;
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2] issues with certain serverrec setting methods
Posted by Stas Bekman <st...@stason.org>.
As I get no further feedback I've committed the patch I've posted earlier.
If you want it different feel free to change, but at least it's not broken
now.
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2] issues with certain serverrec setting methods
Posted by Stas Bekman <st...@stason.org>.
Geoffrey Young wrote:
[...]
> I see your point that we can't allocate from the server pool each time, and
> I guess why the current situation is bad is because the PV will go out of
> scope at the end of the request leaving a bad pointer around, right?
Not only that. It may have an immediate affect if you use a TMP scalar,
which is the case when you don't pass a lexical variable as an argument
but a return value from another call (as it was in the original bug
report). Moreover, if it is a lexical, if it's changed later the entry in
the server rec will be changed too.
> still, I don't see that we need to go the whole
> _modperl_private::apache_server_admin route for these fields. error_fname
> is one that comes to mind as being especially useless to modify - it might
> be (and is) useful to read, but it's useless at request time since the
> error_log fd is opened and forgotten about long before then. maybe it makes
> sense to offer write access during open_logs or something, but at that point
> the PV wouldn't go out of scope, right?
It doesn't make any difference when you allow it. The current
implementation is problematic as explained above. You *have* to copy that
string, unless you can somehow ensure that it's not a TMP and it's not
modified later on. And you can't use the pool, since you will get leaks.
>>If we go with this one, should we use some obscure name space for this
>>kind of stashing? We don't really want to support more APIs, do we? and
>>using the "Apache::ServerRec::ServerAdmin" is something that people will
>>discover and certainly use. Which of course could be a good thing too,
>>as it'll be faster, but on the other hand it won't be set unless a user
>>has modified $s->server_admin. That's why I think it should be hidden
>>into something like: $_modperl_private::apache_server_admin
>
> I'm not opposed to that. I was just offering some thoughts off the top of
> my head :)
Thanks Geoff!
I just thought to use a namespace which is the least likely that someone
will want to use.
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: [mp2] issues with certain serverrec setting methods
Posted by Geoffrey Young <ge...@modperlcookbook.org>.
> But in this case we can't allocate it from the pool, since the object
> that we have is server object, and if we allocate from it, we will get a
> continuous memory leak. The only solution I can see is the one suggested
> earlier by Geoff, which comes from mp1:
>
> SV *server_admin = perl_get_sv("Apache::ServerRec::ServerAdmin", TRUE);
> sv_setsv(server_admin, ST(1));
> obj->server_admin = SvPVX(serever_admin);
>
> i.e. we create a superficial perl variable to make sure that the value
> will stick around if the original scalar is modified or destroyed.
I don't think we need to worry about restoring elements that are native to
the server_rec - these are official record slots, ones that aren't really
messed with all that much, and ones that don't necessarily cause much harm
if the multi-request span isn't fully understood.
document_root is, of course, different because we're messing with the
internal configuration data of http_core _and_ because that value is used
during translation of lots of requests.
> Anybody sees a better solution?
I see your point that we can't allocate from the server pool each time, and
I guess why the current situation is bad is because the PV will go out of
scope at the end of the request leaving a bad pointer around, right?
still, I don't see that we need to go the whole
_modperl_private::apache_server_admin route for these fields. error_fname
is one that comes to mind as being especially useless to modify - it might
be (and is) useful to read, but it's useless at request time since the
error_log fd is opened and forgotten about long before then. maybe it makes
sense to offer write access during open_logs or something, but at that point
the PV wouldn't go out of scope, right?
>
> If we go with this one, should we use some obscure name space for this
> kind of stashing? We don't really want to support more APIs, do we? and
> using the "Apache::ServerRec::ServerAdmin" is something that people will
> discover and certainly use. Which of course could be a good thing too,
> as it'll be faster, but on the other hand it won't be set unless a user
> has modified $s->server_admin. That's why I think it should be hidden
> into something like: $_modperl_private::apache_server_admin
I'm not opposed to that. I was just offering some thoughts off the top of
my head :)
>
> Ideas?
>
> Finally if we go with that solution, should we use it for
> $r->document_root as well, so it'll behave more like mp1 did? in other
> thread we agreed that it's a goodness to restore it at the end of each
> request. I think we should stick to that solution.
agreed.
--Geoff
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org