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...
> That newSVpvf should be a newSVpv. However, it's actually also the cause
> of your leak
>
> hv_store_ent, like hv_store, takes ownership of one reference on the value
> SV (unless it fails with an error). However, it doesn't take ownership of
> the key, so each newSVpv (or newSV anything) is creating a new SV with a
> reference count of 1. The only pointer to it is lost immediately
> hv_store_ent returns, but it retains a reference count of 1, hence the
> leak that you see. This code doesn't leak:
>
> while ((ent = readdir(dir))) {
> SV *key = newSVpv(ent->d_name, 0);
> hv_store_ent(hash, key,
> newSViv(ent->d_ino), 0);
> SvREFCNT_dec (key);
> }
Hmm, ok. (I'm sorry - this all is little murky to me as I've never used XS
before...) I picked the hash code from an example elsewhere.
> but I'd suggest avoiding the key SV completely, as the internals of
> hv_store don't use SVs for keys, so you save having to create then destroy
> a temporary SV:
>
> while ((ent = readdir(dir))) {
> hv_store(hash, ent->d_name, strlen (ent->d_name),
> newSViv(ent->d_ino), 0);
> }
Yes, that seems a fair bit faster, too! Nice.
> > (The array version)
>
> That should be newRV_noinc av_make returns an AV with a reference count of
> 1. No pointer is kept to that AV in the XS code, so it must not own any
> references by the time it returns. newRV_inc takes a reference to the AV,
> but (as the name implies) increments the reference count of the AV, to
> give the RV a "new" reference to own. At the point of return from the XS
> code the AV's reference count is 2; 1 is owned by the RV, and the other
> isn't owned by anyone - a leak.
>
> 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:
void
readdir_inode(dirname)
char* dirname
INIT:
struct dirent *ent;
DIR* dir;
SV* record[2];
AV *entry, *ret_val;
PPCODE:
dir = opendir(dirname);
if (dir) {
while ((ent = readdir(dir))) {
record[0] = newSVpv(ent->d_name, 0);
record[1] = newSViv((IV)ent->d_ino);
PUSHs(sv_2mortal(newRV_noinc((SV*)av_make(2, record))));
SvREFCNT_dec(record[0]);
SvREFCNT_dec(record[1]);
}
closedir(dir);
}
And_ thank you *very* much for taking the time to answer in such
exhaustive and comprehensive manner!
-- v --
[EMAIL PROTECTED]
