Skip Menu |

This queue is for tickets about the Sub-Attribute CPAN distribution.

Report information
The Basics
Id: 107588
Status: resolved
Priority: 0/
Queue: Sub-Attribute
People
Owner: Nobody in particular
Requestors: mschout [...] gkg.net
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Tue Oct 06 17:29:01 2015 mschout [...] gkg.net - Ticket created
Subject: Bug since 5.19.x due to stack moving changes.
Date: Tue, 6 Oct 2015 16:28:15 -0500
To: bug-sub-attribute [...] rt.cpan.org
From: Michael Schout <mschout [...] gkg.net>
Hi. I have been hit by a bug in Sub::Attribute that started on the 5.19 branch due to the stack moving when it is reallocated. There is a lengthy discussion of this issue here: https://rt.perl.org/Public/Bug/Display.html?id=126145 The short summary though is that Sub::Attribute has this line in Attribute.xs: PL_stack_sp -= call_sv(method, G_VOID | G_EVAL); The problem is that its possible that the stack gets grown or reallocated during the call_sv() function, which causes PL_stat_sp to get overwritten based on the OLD stack value, not the new/reallocated one. The attached patch fixes this problem. Thanks! Regards, Michael Schout

Message body is not shown because sender requested not to inline it.

  Tue May 16 09:04:44 2017 dcantrell [...] cpan.org - Correspondence added
On Tue Oct 06 17:29:01 2015, mschout@gkg.net wrote: Show quoted text
> Hi. > > I have been hit by a bug in Sub::Attribute that started on the 5.19 > branch due to the stack moving when it is reallocated. There is a > lengthy discussion of this issue here: > > https://rt.perl.org/Public/Bug/Display.html?id=126145 > > The short summary though is that Sub::Attribute has this line in > Attribute.xs: > > PL_stack_sp -= call_sv(method, G_VOID | G_EVAL); > > The problem is that its possible that the stack gets grown or > reallocated during the call_sv() function, which causes PL_stat_sp to > get overwritten based on the OLD stack value, not the new/reallocated one. > > The attached patch fixes this problem. > > Thanks! > > Regards, > Michael Schout
I believe I have a similar problem, but I don't think that patch is correct. All it seems to do is store the return value from call_sv() into a temporary variable, and then subtract that variable's contents from PL_stack_sp. Also, should it be I32? What about 64-bit builds?
  Tue May 16 09:04:44 2017 The RT System itself - Status changed from 'new' to 'open'
  Tue May 16 10:20:49 2017 mschout [...] gkg.net - Correspondence added
Subject: Re: [rt.cpan.org #107588] Bug since 5.19.x due to stack moving changes.
Date: Tue, 16 May 2017 09:14:13 -0500
To: bug-Sub-Attribute [...] rt.cpan.org
From: Michael Schout <mschout [...] gkg.net>
On 5/16/17 8:04 AM, David Cantrell via RT wrote: Show quoted text
> I believe I have a similar problem, but I don't think that patch is > correct. All it seems to do is store the return value from call_sv() > into a temporary variable, and then subtract that variable's contents > from PL_stack_sp.
Yes, I believe the patch is correct. Here is why: PL_stack_sp can change during the call to call_sv() due to the fact that call_sv() might reallocate the stack. In C, the order of evaluation of "a = b" is undefined, so PL_stack_sp (which is a macro) might be read BEFORE the call to call_sv() happens. If this is the case, and if call_sv() ends up reallocating the stack, then writing it as: PL_stack_sp = call_sv(...) will overwrite PL_stack_sp with the old free()'d value. That is, the value of PL_stack_sp before the stack was moved. This is the crux of the problem. Using a temporary solves this problem because it guarantees that PL_stack_sp will not be read until after the call to call_sv(), so this guarantees that PL_stack_sp will get the reallocated stack pointer, not the free()'d one. We had many SEGV's per day before patching Sub::Attribute with this fix. I have not seen a single one since. Show quoted text
> Also, should it be I32? What about 64-bit builds?
The prototype for Perl_call_sv() is I32. proto.h in perl sources has it as: PERL_CALLCONV I32 Perl_call_sv(pTHX_ SV* sv, I32 flags) This indicates to me that I32 is correct here. Regards, Michael Schout
  Thu Jun 15 12:03:10 2017 dcantrell [...] cpan.org - Correspondence added
I've got co-maint so will release a patched version.
  Mon Jul 03 17:41:08 2017 dcantrell [...] cpan.org - Correspondence added
Fixed in 0.06
  Mon Jul 03 17:41:14 2017 dcantrell [...] cpan.org - Status changed from 'open' to 'resolved'