Why is this XS code that returns a PerlIO* leaky?

Question

I am trying to write some XS code that exposes pieces a library to Perl code as a stream interface that can be written to. The get_stream function below is supposed to be a constructor that prepares and returns a PerlIO object. I figured that I only need the Write and Close methods, so I left all other function slots blank.

typedef struct {
    struct _PerlIO base;
    mylib_context* ctx;
} PerlIOmylib;

/* [...] */

PERLIO_FUNCS_DECL(PerlIO_mylib_funcs) = {
.fsize = sizeof(PerlIO_funcs),
.name  = "mylib",
.size  = sizeof(PerlIOmylib,
.Write = mylib_write,
.Close = mylib_close,
};

/* XS below */

PerlIO*
get_stream (SV* context_obj)
CODE:
mylib_context* ctx = (mylib_context*) SvIV (SvRV (context_obj));
PerlIO* f = PerlIO_allocate (aTHX);
f = PerlIO_push (aTHX, f, PERLIO_FUNCS_CAST(&PerlIO_mylib_funcs), "a", NULL);
PerlIOSelf(f, PerlIOmylib)->ctx = ctx;
PerlIOBase(f)->flags |= PERLIO_F_OPEN;
RETVAL = f;
OUTPUT:
RETVAL

When I use the provided interface like this...

{
    my $fh = MyLib::get_stream($lib_ctx);
    print $fh "x" x 300;
}

... the mylib_write function gets called, so I haven't totally screwed up so far. (I verified this by inserting debug printf statements.) However, I would like the PerlIO object to be closed when $fh goes out of scope, just the way things work with regular filehandles created by open. But at the moment, the mylib_close function is only called during interpreter shutdown.

Directly calling close works fine, setting $fh to undef does not.

UPDATE: Following ikegami's advice, I used Devel::Peek::Dump and sv_dump and found out that the handle returned get_stream function is a "RV" which points to a SV = PVGV(...). The glob (PVGV) has its reference counter set to 3 which doesn't seem right.

I added

CLEANUP:
SvREFCNT_dec (SvRV (ST(0)));
SvREFCNT_dec (SvRV (ST(0)));

which cures the symptom: The close function is called when $fh goes out of scope at the end of the block. But I still don't quite understand the underlying problem.

This is the C code generated for the OUTPUT section:

ST(0) = sv_newmortal();
{
    GV *gv = newGVgen("MyLib");
    if (do_open(gv, "+<&", 3, FALSE, 0, 0, RETVAL) )
        sv_setsv(ST(0), sv_bless(newRV((SV*)gv), gv_stashpv("MyLib",1)));
    else
        ST(0) = &PL_sv_undef;
}
XSRETURN(1);

How does the GV's reference count end up at 3?

Haynie

ug, whole new question in same in same post? – Warrenwarrener

Answer 1

If close is called at global destruction, that means your handle still exists at global destruction. You're leaking!

In C/XS code, you can use sv_dump(sv) to dump a scalar to stderr. In Perl code, you can use Devel::Peek's Dump to get the same functionality. This will show you reference counts.

---

In answer to your new question,

You have three allocation, but only one deallocation (the delayed one from sv_2mortal).

• gv: Pointer always discarded. Memory leak!

  You could either decrement the refcnt of gv on error, or you decrement the refcnt on unconditionally after using newRV_inc to "transfer ownership" to the RV when open succeeds.

• SV from newRV: Pointer always discarded. Memory leak!

  Why not just return it instead of copying it? Just mark it as mortal to cause Perl to decrement its refcnt after the caller gets it.

Fixed:

{
    GV *gv = newGVgen("MyLib");
    if (!do_open(gv, "+<&", 3, FALSE, 0, 0, RETVAL) ) {
        SvREFCNT_dec(gv);
        XSRETURN_UNDEF;
    }

    ST(0) = sv_2mortal(sv_bless(newRV_noinc((SV*)gv), gv_stashpv("MyLib",1))));
    XSRETURN(1);
}

Warrenwarrener

When I add Pushed and Popped functions, I see that Popped is still only called during global interpreter shutdown. – Haynie

I previously missed that close was called then too. (I thought you said it wasn't being called at all.) Your file handle is surviving until global destruction. You're leaking. – Warrenwarrener

Putting sv_dump (ST(0)) into a CLEANUP section revealed that the reference count of the "thing" that the PerlIO * was turned into was 3. Reducing that by 2 causes close to be called at the rigth time, but I am still confused. – Haynie

Added answer to new question. – Warrenwarrener

The C code in question was generated by ExtUtils::ParseXS, presumably based on the return type (PerlIO *). So... is the generator buggy? – Haynie

Answer 2

I just reproduced the issue with a trivial example:

$ h2xs -n foo
Defaulting to backwards compatibility with perl 5.14.2
If you intend this module to be compatible with earlier perl versions, please
specify a minimum perl version with the -b option.

Writing foo/ppport.h
Writing foo/lib/foo.pm
Writing foo/foo.xs
Writing foo/fallback/const-c.inc
Writing foo/fallback/const-xs.inc
Writing foo/Makefile.PL
Writing foo/README
Writing foo/t/foo.t
Writing foo/Changes
Writing foo/MANIFEST

To foo/foo.xs, I added:

PerlIO*
get_stream(char* name);
CODE:
RETVAL = PerlIO_open (name, "w");
OUTPUT:
RETVAL

and the following trivial test program:

#!/usr/bin/perl
use foo;
use Devel::Peek;
{
    my $fh = foo::get_stream ("testfile");
    Devel::Peek::Dump $fh;
    print $fh "hello\n";
}
print "bye\n";

Sure enough, the reference count of the generated glob is set to 3, and strace reveals that closing the file descriptor is the last thing that the Perl interpreter does.

So, PerlIO* handling seems to be leaky by default. :-(

The following typemap snippet seems to fix this (thanks, ikegami!):

TYPEMAP
PerlIO *    T_PIO
OUTPUT
T_PIO
    {
        GV *gv = newGVgen("$Package");
        if (do_open(gv, "+<&", 3, FALSE, 0, 0, $var) ) {
            $arg = sv_2mortal(sv_bless(newRV_noinc((SV*)gv), gv_stashpv("$Package",1)));
        } else {
            SvREFCNT_dec(gv);
            $arg = &PL_sv_undef;
        }
    }

Haynie