Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Inline CPAN distribution.

Report information
The Basics
Id: 55543
Status: resolved
Priority: 0/
Queue: Inline

People
Owner: Nobody in particular
Requestors: asuffield [...] suffields.me.uk
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Inline::C can crash the perl interpreter if function uses PUSHMARK
Date: Sun, 14 Mar 2010 04:26:53 +0000
To: bug-Inline [...] rt.cpan.org
From: Andrew Suffield <asuffield [...] suffields.me.uk>
Inline::C uses this hack in an attempt to detect the difference between void returns and XSUB list returns: temp = PL_markstack_ptr++; $function($arg_name_list); if (PL_markstack_ptr != temp) { /* truly void, because dXSARGS not invoked */ PL_markstack_ptr = temp; XSRETURN_EMPTY; /* return empty stack */ } /* must have used dXSARGS; list context implied */ return; /* assume stack size is correct */ Sadly, this does not work. If the mark stack is reallocated (due to use of PUSHMARK in any function called), then PL_markstack_ptr can have a completely different value. This code then scribbles over it with the old value, pointing to freed memory; the interpreter will crash shortly after this. I haven't checked carefully, but I think this should have been examining the value of (PL_markstack_ptr - PL_markstack) instead - that's the current height of the stack, rather than its current address.
RT-Send-CC: inline [...] perl.org
On Sat Mar 13 23:27:29 2010, asuffield@suffields.me.uk wrote: Show quoted text
> Inline::C uses this hack in an attempt to detect the difference > between void returns and XSUB list returns: > > temp = PL_markstack_ptr++; > $function($arg_name_list); > if (PL_markstack_ptr != temp) { > /* truly void, because dXSARGS not invoked */ > PL_markstack_ptr = temp; > XSRETURN_EMPTY; /* return empty stack */ > } > /* must have used dXSARGS; list context implied */ > return; /* assume stack size is correct */ > > Sadly, this does not work. If the mark stack is reallocated (due to > use of PUSHMARK in any function called), then PL_markstack_ptr can > have a completely different value. This code then scribbles over it > with the old value, pointing to freed memory; the interpreter will > crash shortly after this. > > I haven't checked carefully, but I think this should have been > examining the value of (PL_markstack_ptr - PL_markstack) instead - > that's the current height of the stack, rather than its current > address.
(cc'ing the Inline mailing list in case anyone there is interested.) Wow ... that code has been around for ages. I think I get the picture, though I'm currently having trouble reproducing the bug from the description. (Dimness on my part, one suspects :-) Do you have a simple demo script ? It doesn't have to do anything meaningful - just something that demonstrates the problem. Thanks for the report ! Cheers, Rob
Subject: Re: [rt.cpan.org #55543] Inline::C can crash the perl interpreter if function uses PUSHMARK
Date: Sun, 14 Mar 2010 11:29:46 +0000
To: Sisyphus via RT <bug-Inline [...] rt.cpan.org>
From: Andrew Suffield <asuffield [...] suffields.me.uk>
On Sun, Mar 14, 2010 at 06:07:28AM -0400, Sisyphus via RT wrote: Show quoted text
> Wow ... that code has been around for ages. > I think I get the picture, though I'm currently having trouble > reproducing the bug from the description. (Dimness on my part, one > suspects :-) > > Do you have a simple demo script ? It doesn't have to do anything > meaningful - just something that demonstrates the problem.
It's viciously hard to reproduce this one on demand. The precise triggering conditions are subtle and can be random. I found it entirely by chance, in some code that had been working fine for about a week before hitting the right combination. In order for the bug to break things, you need the mark stack to be reallocated while inside the wrapped function. So, there has to be a call to PUSHMARK inside it or the bug can't be triggered; notably, any call_sv usage is at risk here (since perl code will fiddle with the mark stack on every function call). However, most PUSHMARK calls don't reallocate the stack, so won't break anything. Getting the stack to reallocate is much harder. It happens via this bit of scope.c (I'm using 5.10 here): void Perl_markstack_grow(pTHX) { dVAR; const I32 oldmax = PL_markstack_max - PL_markstack; const I32 newmax = GROW(oldmax); Renew(PL_markstack, newmax, I32); PL_markstack_ptr = PL_markstack + oldmax; PL_markstack_max = PL_markstack + newmax; } Renew is a realloc wrapper, so the triggering condition is whatever makes realloc decide to allocate-copy-free rather than extend. That's dependent on the host system and the memory layout at the time. There's no way to write a conventional test script for it. Fortunately, it looks like somebody needed to debug one of these once before. Unfortunately you'll have to rebuild perl. The code's of 5.005 vintage and I haven't tested to see if it's bitrotten, but assuming it isn't, the incantation is: sh Configure -DDEBUGGING -Dusemymalloc -Accflags="-DSTRESS_REALLOC" Then if you create an Inline-wrapped function that calls PUSHMARK and POPMARK a few times, it should do the trick. With STRESS_REALLOC, mymalloc will make a new copy on every realloc call. Finally, to encourage perl to report the confusion promptly when things go wrong, set PERL_BADFREE=1 PERL_MALLOC_OPT='d=1;c=1' in the environment. That makes it report bad frees, and fill freed memory with 0xdeadbeef. I think. If that still doesn't work, you may have to find a wizard.
On Sun Mar 14 07:33:13 2010, asuffield@suffields.me.uk wrote: Show quoted text
> Fortunately, it looks like somebody needed to debug one of these once > before. Unfortunately you'll have to rebuild perl. The code's of 5.005 > vintage and I haven't tested to see if it's bitrotten, but assuming it > isn't, the incantation is: > sh Configure -DDEBUGGING -Dusemymalloc -Accflags="-DSTRESS_REALLOC" > > Then if you create an Inline-wrapped function that calls PUSHMARK and > POPMARK a few times, it should do the trick. With STRESS_REALLOC, > mymalloc will make a new copy on every realloc call. Finally, to > encourage perl to report the confusion promptly when things go wrong, > set PERL_BADFREE=1 PERL_MALLOC_OPT='d=1;c=1' in the environment. That > makes it report bad frees, and fill freed memory with 0xdeadbeef. I > think.
I've given that a try, but still can't get anything nasty to happen. No matter ... let's concentrate on a fix. How about (as per your suggestion): PREINIT: I32* temp; PPCODE: temp = PL_markstack_ptr - INT2PTR(I32, PL_markstack); PL_markstack_ptr++; $function($arg_name_list); if (PL_markstack_ptr - INT2PTR(I32, PL_markstack) != temp) { /* truly void, because dXSARGS not invoked */ PL_markstack_ptr--; XSRETURN_EMPTY; /* return empty stack */ } /* must have used dXSARGS; list context implied */ return; /* assume stack size is correct */ Questions: 1) Does that fix the particular problem for you ? 2) I gather that there's currently never any problem for the "truly void" functions - that the problem can only arise when dXSARGS are invoked. Is that right ? 3) In the "truly void" case, instead of assigning the original value back to PL_markstack_ptr, the above code just does a "PL_markstack_ptr-- ;". Are there situations when that's *not* the right thing to do ? If we really need to assign the old value back again, I guess we could store it in a second temp value - ie do: temp2 = PL_markstack_ptr++; and then, later: PL_markstack_ptr = temp2; 4) The "INT2PTR()" stuff is just to shut up an "assignment makes pointer from integer without a cast" and a "comparison between pointer and integer" warning. Is that the right approach to getting rid of those warnings ? (It seems to have done the trick.) 5) I'm also a little worried about the "I32" - both in the INT2PTR() calls and in the declaration of "temp" in the PREINIT. The latter has been there for a long time, and doesn't seem to have posed any problems as yet - at least afaik. But how does "I32" impact upon builds of perl that use 64-bit addresses ? If we can get the patch sorted out to the extent that it looks like it *ought* to be right, then I'd probably be happy to include it in a new "developer" release on CPAN ... and sit back for a while and see if anything adverse occurs. We'd want to add some tests to the test suite that might uncover problems introduced by the patch. (Some tests that perform callbacks to perl would seem to be a good idea - any other suggestions on tests we could use to verify that the fix does not cause breakages are welcome.) Cheers, Rob
Subject: Re: [rt.cpan.org #55543] Inline::C can crash the perl interpreter if function uses PUSHMARK
Date: Mon, 15 Mar 2010 12:25:18 +0000
To: Sisyphus via RT <bug-Inline [...] rt.cpan.org>
From: Andrew Suffield <asuffield [...] suffields.me.uk>
On Mon, Mar 15, 2010 at 07:07:41AM -0400, Sisyphus via RT wrote: Show quoted text
> 2) I gather that there's currently never any problem for the "truly > void" functions - that the problem can only arise when dXSARGS are > invoked. Is that right ?
No, a void function could have used call_sv and ended up reallocating the mark stack somewhere. Anything that hands control back to the perl interpreter is 'unsafe' here. Show quoted text
> 3) In the "truly void" case, instead of assigning the original value > back to PL_markstack_ptr, the above code just does a "PL_markstack_ptr-- > ;". Are there situations when that's *not* the right thing to do ? If > we really need to assign the old value back again, I guess we could > store it in a second temp value - ie do: > temp2 = PL_markstack_ptr++; > and then, later: > PL_markstack_ptr = temp2;
Other way around; assigning back the old value is the thing which is wrong (since in the reallocation case, the old value points to freed memory). You have to assume that all stack pointers from before the $function() call are invalidated, and fetch new ones. Using PL_markstack_ptr-- should be safe - I think. However, this is all making a lot of fragile assumptions about how perl handles the mark stack. It occurs to me that it would be sensible to write: <record mark stack height> PUSHMARK(SP); $function($arg_name_list); if ( <mark stack height is unchanged> ) { POPMARK; ... That's obviously correct, and more robust against changes to perl's internals. Show quoted text
> 4) The "INT2PTR()" stuff is just to shut up an "assignment makes > pointer from integer without a cast" and a "comparison between pointer > and integer" warning. Is that the right approach to getting rid of > those warnings ? (It seems to have done the trick.)
Wrong way around. The difference between two pointers is just an integer, so you wanted this: PTRV temp; PPCODE: temp = PL_markstack_ptr - PL_markstack; (And the corresponding expression later) This also fixes #5. PTRV is whatever Configure decided was an unsigned integer the same size as a pointer, hence must be big enough. Ironically, in the couple of places where perl does this, it just uses an I32 - but the mark stack is unlikely to ever grow that large, it's roughly the same as the call stack height. Show quoted text
> 1) Does that fix the particular problem for you ?
(I'll get back to you on this when I've had time to run some tests)
On Mon Mar 15 08:25:50 2010, asuffield@suffields.me.uk wrote: Show quoted text
> > 1) Does that fix the particular problem for you ?
> > (I'll get back to you on this when I've had time to run some tests)
Ok - thanks for all of that. I'm now starting to feel a little bit more comfortable. If it does the trick, I guess we end up with something like: $XS .= <<END; PREINIT: PTRV temp; PPCODE: temp = PL_markstack_ptr - PL_markstack; PUSHMARK(SP); $function($arg_name_list); if (PL_markstack_ptr - PL_markstack != temp) { POPMARK; XSRETURN_EMPTY; /* return empty stack */ } return; /* assume stack size is correct */ END Anyway - wait and see how your testing turns out. Cheers, Rob
On Tue Mar 16 00:22:46 2010, SISYPHUS wrote: Show quoted text
> Anyway - wait and see how your testing turns out.
If it helps, Inline-0.46_01 is now on CPAN. It contains a test script (C/t/10callback.t) that does a lot of PUSHMARK()ing - and then checks to see that xs_bindings() still gets it right regarding whether the function is "truly void" or not. Perhaps that test script already produces unexpected results for you ... or perhaps you can modify it so that it *does* reproduce the problem you've come up against. (Or, perhaps, it's totally useless :-) The actual xs_bindings() code used to distinguish "truly void" from "return a list" is essentially unchanged from 0.46 - so the bug you've described should still be present (if we can just get it to stick its ugly head up). Cheers, Rob
Lack of follow-up