On Wed, Jan 01, 2003 at 10:41:26PM +0200, Ville Herva wrote:
> Nicholas Clark wrote:
> > You don't want to be using newSVpvf on an untrusted string:
> >
> > $ touch /tmp/%s%s%s%s%s%s%s%s%s%s
> >
> > SEGV
>
> Ok, didn't think of that...
Been there, done that. Well, it wasn't my code, it was InterWorld 2.5,
which was logging via syslog (not that I ever saw their source code). The
main parameter to syslog is a format, not a literal string...
Until they fixed that bug, malicious URLs represented a remote denial-of-
service attack, as they would crash the server process during its error
logging.
> > The RV itself is (correctly, as I understand it) mortalised to be put on
> > the stack. The reference counts on the RV are correct - it will get
> > destroyed at the right time, and when that happens it stops owning a
> > reference on the AV, so perl calls SvREFCNT_dec on the AV as part of the
> > cleanup of the RV. This will leave the AV with a reference count of 1 - it
> > won't get cleaned up at this point even though nothing owns it. This is
> > the cause of the leak you have identified.
>
> But it seems not the only cause. Making it _noinc got rid of about half of
> the leak. I added the SvREFCNT_dec(record[0]); and
> SvREFCNT_dec(record[1]); too - now it seems not to leak. This is the
> version I ended up with:
Ah yes. I didn't know that, and didn't look too closely at av_make.
perlguts.pod says:
=item av_make
Creates a new AV and populates it with a list of SVs. The SVs are copied
into the array, so they may be freed after the call to av_make. The new AV
will have a reference count of 1.
AV* av_make(I32 size, SV** svp)
As av_make is copying the SVs you provide, they remain with a refcount of 1,
and your code owns that reference, so it needs to clean up when it has
finished. Which is almost immediately.
> And_ thank you *very* much for taking the time to answer in such
> exhaustive and comprehensive manner!
Well, I could see what I thought was the answer straight away, and I'm
procrastinating hard not doing something else.
Nicholas Clark
