On Wed, Jan 01, 2003 at 05:47:43PM +0200, Ville Herva wrote:
> [Cc:'ing me appreciated - I will poll the archive, though]
>
> I downloaded ReadDir module from CPAN. I works great, but leaks memory.
> I tried to make sense of where the leak takes place, but even with the help
> of perl-xs mailing list archives I couldn't spot it.
>
> Could someone tell me off hand what is being done wrong in the following
> snippet?
>
> (Confirmed as leaky with perl 5.6.1 and perl 5.0-5-3.)
>
>
> My current (leaky) try (trying to make a hash instead of an array):
> -------------------------------------------------------------------------
> /* <=-*- C -*-=> */
>
> #include "EXTERN.h"
> #include "perl.h"
> #include "XSUB.h"
> #include <sys/types.h>
> #include <dirent.h>
> #include <stdio.h>
>
> MODULE = ReadDir PACKAGE = ReadDir
>
> void
> readdir_inode(dirname)
> char* dirname
> INIT:
> struct dirent *ent;
> DIR* dir;
> AV *entry, *ret_val;
> HV* hash;
> PPCODE:
> dir = opendir(dirname);
> hash = newHV();
>
> if (dir) {
> while ((ent = readdir(dir))) {
> hv_store_ent(hash, newSVpvf(ent->d_name),
> newSViv(ent->d_ino), 0);
> }
>
> closedir(dir);
>
> XPUSHs(sv_2mortal(newRV_noinc((SV *) hash)));
> }
> ------------------------------------------------------------------------
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
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);
}
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);
}
The docs for hv_store_ent (and hv_fetch_ent) could do with some clarification,
which I'll try to do today before I forget, as it's an excellent excuse for
some more procrastination from what I should be doing.
> The original ReadDir 0.01 from CPAN (leaks too):
> ------------------------------------------------------------------------
> /* <=-*- C -*-=> */
>
> #include "EXTERN.h"
> #include "perl.h"
> #include "XSUB.h"
> #include <sys/types.h>
> #include <dirent.h>
> #include <stdio.h>
>
> MODULE = ReadDir PACKAGE = ReadDir
>
> 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_inc((SV*)av_make(2, record))));
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.
> }
> closedir(dir);
> }
Nicholas Clark
