Bug #92540 for version: odd interactions with version::vxs and Readonly on 5.12

Tue Jan 28 13:57:19 2014 cja987 [...] gmail.com - Ticket created

Subject:

odd interactions with version::vxs and Readonly on 5.12

perl -Mversion -e 'Readonly::Scalar our $ver => 1.01; version->new($ver)' Invalid version format (non-numeric data) at -e This is with the latest versions of version, Readonly, and Readonly::XS, but on an older perl (5.12). I don't have this exact setup on later perl versions, so I'm not sure if it's perl-version-dependent or not. This also does not occur with version::vpp, nor with a non-readonly version, and if I either stringify (with "$ver") or numify (with 0+$ver), the problem disappears, so I suspect it's something to do with the SV flags that Readonly::XS fiddles with not interacting right with version::vxs. I've had to compile version with XS disabled to get around it. A way of disabling version::vxs at import time with an environment variable would be much appreciated.

Wed Jan 29 05:23:27 2014 BULKDD [...] cpan.org - Correspondence added

On Tue Jan 28 13:57:19 2014, sproingie wrote: Show quoted text

> > perl -Mversion -e 'Readonly::Scalar our $ver => 1.01; version-

> >new($ver)'

> Invalid version format (non-numeric data) at -e > > This is with the latest versions of version, Readonly, and > Readonly::XS, but on an older perl (5.12). I don't have this exact > setup on later perl versions, so I'm not sure if it's perl-version- > dependent or not. This also does not occur with version::vpp, nor > with a non-readonly version, and if I either stringify (with "$ver") > or numify (with 0+$ver), the problem disappears, so I suspect it's > something to do with the SV flags that Readonly::XS fiddles with not > interacting right with version::vxs. > > I've had to compile version with XS disabled to get around it. A way > of disabling version::vxs at import time with an environment variable > would be much appreciated.

Your code does not run. --------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -e "Rea donly::Scalar our $ver => 1.01; version->new($ver)" syntax error at -e line 1, near "Readonly::Scalar our " Execution of -e aborted due to compilation errors. C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -v This is perl 5, version 12, subversion 2 (v5.12.2) built for MSWin32-x86-multi-t hread --------------------------------------------------- after fixing it to ------------------------------------------------------ perl -Mversion -MReadonly -e "Readonly::Scalar our $ver => 1.01; system('pause'); version->new($ver)" ------------------------------------------------------ I also noticed there is a SV leak in new_version if there is a croak in upg_version, ill try to make a patch. The reason it says non-numeric is, the SV* from Readonly::Scalar gives this kind of SV ------------------------------------------------------ SV = PVMG(0x822164) at 0x82b504 REFCNT = 1 FLAGS = (pNOK) IV = 0 NV = 1.01 PV = 0 ------------------------------------------------------ None of the flag tests in Perl_upg_version check private flags, only public flags so it falls through to "Invalid version format (non-numeric data)". Notice the pNOK but not NOK. The rest i need to think more about.

Wed Jan 29 05:23:27 2014 The RT System itself - Status changed from 'new' to 'open'

Wed Jan 29 05:26:30 2014 BULKDD [...] cpan.org - Correspondence added

Actual RO scalar SV is -------------------------------------------- SV = PVMG(0x822144) at 0x82baa4 REFCNT = 2 FLAGS = (GMG,SMG,RMG,pNOK) IV = 0 NV = 1.01 PV = 0 MAGIC = 0x83e914 MG_VIRTUAL = &PL_vtbl_packelem MG_TYPE = PERL_MAGIC_tiedscalar(q) MG_FLAGS = 0x02 REFCOUNTED MG_OBJ = 0x397ca4 SV = IV(0x397ca0) at 0x397ca4 REFCNT = 1 FLAGS = (ROK) RV = 0x8b30c4 ------------------------------------------------

Wed Jan 29 14:27:25 2014 BULKDD [...] cpan.org - Correspondence added

On 5.19.8 ------------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -MReado nly -e "Readonly::Scalar our $ver => 1.01; print version->new($ver);" 1.01 C:\Documents and Settings\Owner\Desktop\cpan libs\version> -------------------------------------------------------- on 5.12.2 ---------------------------------------------------- C:\Documents and Settings\Owner\Desktop\cpan libs\version>perl -Mversion -MReado nly -e "Readonly::Scalar our $ver => 1.01; print version->new($ver);" Invalid version format (non-numeric data) at -e line 1. C:\Documents and Settings\Owner\Desktop\cpan libs\version> ----------------------------------------------------

Wed Jan 29 15:33:10 2014 BULKDD [...] cpan.org - Correspondence added

On 5.16.3 --------------------------------------------------- C:\Documents and Settings\Administrator\Desktop\version-master>perl -Mversion -M Readonly -e "Readonly::Scalar our $ver => 1.01; print version->new($ver);" Invalid version format (non-numeric data) at -e line 1. C:\Documents and Settings\Administrator\Desktop\version-master> ---------------------------------------------------- on 5.19, notice the mg sv is NOK, it plain pNOK on older perls ---------------------------------------------------- SV = PVMG(0x8f379c) at 0x36801c REFCNT = 1 FLAGS = (GMG,SMG,RMG,NOK,pNOK) IV = 0 NV = 1.01 PV = 0 MAGIC = 0x9123cc MG_VIRTUAL = &PL_vtbl_packelem MG_TYPE = PERL_MAGIC_tiedscalar(q) MG_FLAGS = 0x02 REFCOUNTED MG_OBJ = 0x36805c SV = IV(0x368058) at 0x36805c REFCNT = 1 FLAGS = (ROK) RV = 0x9559f4 SV = PVMG(0x8f381c) at 0x8fcdac REFCNT = 1 FLAGS = (NOK,pNOK) IV = 0 NV = 1.01 PV = 0 ------------------------------------- on 5.18.1 --------------------------------------- C:\Documents and Settings\Administrator\Desktop\version-master>perl -Mversion -M Readonly -e "Readonly::Scalar our $ver => 1.01; print version->new($ver);" 1.01 C:\Documents and Settings\Administrator\Desktop\version-master> ----------------------- I'm going to make a guess, based on comments from #p5p, that this was fixed in 5.17.2 in http://perl5.git.perl.org/perl.git/commit/4bac9ae47b5ad7845a24e26b0e95609805de688a .

Wed Jan 29 17:14:48 2014 BULKDD [...] cpan.org - Cc BULKDD added

Sat Feb 01 11:47:29 2014 jpeacock [...] cpan.org - Correspondence added

On Wed Jan 29 15:33:10 2014, BULKDD wrote: Show quoted text

> I'm going to make a guess, based on comments from #p5p, that this was > fixed in 5.17.2 in > http://perl5.git.perl.org/perl.git/commit/4bac9ae47b5ad7845a24e26b0e95609805de688a > .

I cannot see that error, no matter what Perl version I run: bash-4.2$ perl -Mversion -MReadonly -e "Readonly::Scalar our $ver => 1.01; version->new($ver)" Attempt to reassign a readonly scalar at -e line 1 bash-4.2$ perl -v This is perl 5, version 12, subversion 4 (v5.12.4) built for x86_64-linux I'm mystified. In any case, I don't see that there is anything that the version.pm code could do to help.

Sat Feb 01 11:56:54 2014 cja987 [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 1 Feb 2014 08:56:44 -0800
To:	"bug-version [...] rt.cpan.org" <bug-version [...] rt.cpan.org>
From:	Chuck Adams <cja987 [...] gmail.com>

What could help is if it were possible to disable the XS side with an environment variable. As it is now, the only way I've found is either brutally hacking up @LIB before using version (which won't work if something already used it) or to rebuild version without XS. On Saturday, February 1, 2014, John Peacock via RT <bug-version@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=92540 > > > On Wed Jan 29 15:33:10 2014, BULKDD wrote:

> > I'm going to make a guess, based on comments from #p5p, that this was > > fixed in 5.17.2 in > >

> http://perl5.git.perl.org/perl.git/commit/4bac9ae47b5ad7845a24e26b0e95609805de688a

> > .

> > > I cannot see that error, no matter what Perl version I run: > > bash-4.2$ perl -Mversion -MReadonly -e "Readonly::Scalar our $ver => 1.01; > version->new($ver)" > Attempt to reassign a readonly scalar at -e line 1 > > bash-4.2$ perl -v > > This is perl 5, version 12, subversion 4 (v5.12.4) built for x86_64-linux > > I'm mystified. In any case, I don't see that there is anything that the > version.pm code could do to help. >

Sat Feb 01 12:06:23 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 12:06:13 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 11:56 AM, Chuck Adams via RT wrote: Show quoted text

> Queue: version > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=92540 > > > What could help is if it were possible to disable the XS side with an > environment variable. As it is now, the only way I've found is either > brutally hacking up @LIB before using version (which won't work if > something already used it) or to rebuild version without XS.

I would much rather understand why you are seeing an error that I am not seeing. Adding a hack to disable the XS code won't solve any problems, since the built in version code in the core will not share that new behavior. Unless you could guarantee that you would always be the first to 'use version' there is no way to make such a hack even work. Please attach your 'perl -V' output to the ticket and I'll try and recreate here. John

Sat Feb 01 12:23:13 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 12:06:23 2014, john.peacock@havurah-software.org wrote: Show quoted text

> > I would much rather understand why you are seeing an error that I am not > seeing. Adding a hack to disable the XS code won't solve any problems, > since the built in version code in the core will not share that new > behavior. Unless you could guarantee that you would always be the first > to 'use version' there is no way to make such a hack even work. > > Please attach your 'perl -V' output to the ticket and I'll try and > recreate here. > > John

I can reproduce it, and I know why he gets non-numeric. The reason is in Perl_upg_version only public flags are checked. Tieds/magic NEVER get public OK flags set on older perls apparently. so on < 5.17.2 Perl_upg_version needs to check SvNOKp and SvPOKp and etc. There is also a leak in Perl_new_version since if UPG_VERSION croaks, SV "rv" is leaked. Either a early sv_2mortal and at the very end, after UPG_VERSION (and it is guarenteed we can't croak), a SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with SAVEFREESV. I will try to get a fix in someone this week. My idea is to add more else if blocks to upg_version towards the end of the branch tree, with gotos to their public flag counterparts. the private flag blocks are #if'ed out on >= 5.17.2.

Sat Feb 01 12:38:00 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 12:37:50 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 12:23 PM, Daniel Dragan via RT wrote: Show quoted text

> > I can reproduce it, and I know why he gets non-numeric. The reason is > in Perl_upg_version only public flags are checked. Tieds/magic NEVER > get public OK flags set on older perls apparently. so on < 5.17.2 > Perl_upg_version needs to check SvNOKp and SvPOKp and etc. There is > also a leak in Perl_new_version since if UPG_VERSION croaks, SV "rv" > is leaked. Either a early sv_2mortal and at the very end, after > UPG_VERSION (and it is guarenteed we can't croak), a > SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with > SAVEFREESV. I will try to get a fix in someone this week. My idea is > to add more else if blocks to upg_version towards the end of the > branch tree, with gotos to their public flag counterparts. the > private flag blocks are #if'ed out on >= 5.17.2. >

No, I understand why it goes down that code path; I would be much happier being able to recreate the error locally. I agree with your plan to fix the tied/magic bits and I'll take a crack at it. With Perl 5.20.0 nearing the station, I'd like to get this all wrapped up and blead synced to a good CPAN release before the 20th, so I'll take a crack at your scheme today if possible. John

Sat Feb 01 12:40:55 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 12:40:42 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 12:38 PM, John Peacock via RT wrote: Show quoted text

> No, I understand why it goes down that code path; I would be much > happier being able to recreate the error locally.

Never mind, I've got it sussed out. You must both be running on Windows, where double quotes are default. I'm running Linux, of course, so using double-quotes cause the $ver inside to get interpreted by the shell to nothingness, hence the error I was getting. Once I switch to single quotes, the error pops out. John

Sat Feb 01 12:47:06 2014 cja987 [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 1 Feb 2014 09:46:56 -0800
To:	"bug-version [...] rt.cpan.org" <bug-version [...] rt.cpan.org>
From:	Chuck Adams <cja987 [...] gmail.com>

I'm running on Linux (EL5), but I typed the original error report from memory, and I guessed I messed it up. :-/ So nothing windows-specific there. On Sat, Feb 1, 2014 at 9:40 AM, John Peacock via RT <bug-version@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=92540 > > > On 02/01/2014 12:38 PM, John Peacock via RT wrote:

>> No, I understand why it goes down that code path; I would be much >> happier being able to recreate the error locally.

> > Never mind, I've got it sussed out. You must both be running on > Windows, where double quotes are default. I'm running Linux, of course, > so using double-quotes cause the $ver inside to get interpreted by the > shell to nothingness, hence the error I was getting. Once I switch to > single quotes, the error pops out. > > John >

Sat Feb 01 13:22:48 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 12:38:00 2014, john.peacock@havurah-software.org wrote: Show quoted text

> > No, I understand why it goes down that code path; I would be much > happier being able to recreate the error locally. I agree with your > plan to fix the tied/magic bits and I'll take a crack at it. With Perl > 5.20.0 nearing the station, I'd like to get this all wrapped up and > blead synced to a good CPAN release before the 20th, so I'll take a > crack at your scheme today if possible. > > John

Remember to check private flags after all attemtps at checking public flags failed since private flags also supposedly (from what I read) represent lossy type conversions.

Sat Feb 01 13:25:20 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 13:25:09 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 12:40 PM, John Peacock via RT wrote: Show quoted text

> Queue: version > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=92540 > > > On 02/01/2014 12:38 PM, John Peacock via RT wrote:

>> No, I understand why it goes down that code path; I would be much >> happier being able to recreate the error locally.

Trivially speaking, this works: diff -r 5e76102adf1a vutil/vutil.c --- a/vutil/vutil.c Sat Feb 01 08:55:41 2014 -0500 +++ b/vutil/vutil.c Sat Feb 01 13:22:11 2014 -0500 @@ -558,7 +558,11 @@ #endif PERL_ARGS_ASSERT_UPG_VERSION; - if ( SvNOK(ver) && !( SvPOK(ver) && SvCUR(ver) == 3 ) ) + if (SvNOK(ver) +#if PERL_VERSION_LT(5,17,2) + || (SvTYPE(ver) == SVt_PVMG && SvNOKp(ver)) +#endif + && !( SvPOK(ver) && SvCUR(ver) == 3 ) ) { STRLEN len; but I wonder if there are other sorts of magical things that could look like that (not just tied scalars). John

Sat Feb 01 13:39:09 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 13:38:57 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 01:22 PM, Daniel Dragan via RT wrote: Show quoted text

> > Remember to check private flags after all attemtps at checking public flags failed since private flags also supposedly (from what I read) represent lossy type conversions. >

I think what I just pushed is sufficiently paranoid. Notionally: Take SvNOK or if magical and not SvPOK, accept SvNOKp else take SvPOK or if magical, accept SvPOKp Does that seem to cover everything? John

Sat Feb 01 13:50:08 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 13:49:54 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 01:39 PM, John Peacock via RT wrote: Show quoted text

> I think what I just pushed is sufficiently paranoid. Notionally:

One more branch: Show quoted text

> Take SvNOK or if magical and not SvPOK, accept SvNOKp

else take SvIOK or if magical and not SvPOK, accept SvIOKp Show quoted text

> else take SvPOK or if magical, accept SvPOKp >

John

Sat Feb 01 14:17:42 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 14:17:32 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 12:23 PM, Daniel Dragan via RT wrote: Show quoted text

> There is > also a leak in Perl_new_version since if UPG_VERSION croaks, SV "rv" > is leaked. Either a early sv_2mortal and at the very end, after > UPG_VERSION (and it is guarenteed we can't croak), a > SvREFCNT_inc_optimized is required. Or replace the sv_2mortal with > SAVEFREESV.

New ticket for the leak only: https://rt.cpan.org/Ticket/Display.html?id=92642 John

Sat Feb 01 15:56:06 2014 BULKDD [...] cpan.org - Correspondence added

On Sat Feb 01 13:25:20 2014, john.peacock@havurah-software.org wrote: Show quoted text

> On 02/01/2014 12:40 PM, John Peacock via RT wrote:

> > Queue: version > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=92540 > > > > > On 02/01/2014 12:38 PM, John Peacock via RT wrote:

> >> No, I understand why it goes down that code path; I would be much > >> happier being able to recreate the error locally.

> > Trivially speaking, this works: > > diff -r 5e76102adf1a vutil/vutil.c > --- a/vutil/vutil.c Sat Feb 01 08:55:41 2014 -0500 > +++ b/vutil/vutil.c Sat Feb 01 13:22:11 2014 -0500 > @@ -558,7 +558,11 @@ > #endif > PERL_ARGS_ASSERT_UPG_VERSION; > > - if ( SvNOK(ver) && !( SvPOK(ver) && SvCUR(ver) == 3 ) ) > + if (SvNOK(ver) > +#if PERL_VERSION_LT(5,17,2) > + || (SvTYPE(ver) == SVt_PVMG && SvNOKp(ver))

SVt_PVMG does not determine if a SV is magical or not. A magical can also be SVt_PVLV. Checking the type only matters if you want to make something magical that wasn't magical before or you are setting new flags that were previously off. A SvUPGRADE will be hanging near by too. That check is wrong. Just check flags alone. I suggest checking the private flags after all public flags are checked, otherwise there will be more complaints over (some details ahead may be wrong), a version number that is IV 2^54 on a 64 bit IV machine, that had something like ---------------------------------------- $v = 2**54; say $v+0.5; version->new($v) ---------------------------------------- I tried to simulate that ona 64 bit perl but its not working and #p5p hasn't given me an answer. ---------------------------------------- C:\Documents and Settings\Administrator>perl -MDevel::Peek -E "{use integer; $v= (2**55)+0;} {no integer; say $v*1.3; Dump($v);}" 4.68374361246532e+016 SV = PVNV(0x4e1ab8) at 0x1d47c48 REFCNT = 1 FLAGS = (IOK,NOK,pIOK,pNOK) IV = 36028797018963968 NV = 3.6028797018964e+016 PV = 0 ---------------------------------------- NOK should be off on that according to perlguts.

Sat Feb 01 17:52:13 2014 john.peacock [...] havurah-software.org - Correspondence added

Subject:	Re: [rt.cpan.org #92540] odd interactions with version::vxs and Readonly on 5.12
Date:	Sat, 01 Feb 2014 17:52:00 -0500
To:	bug-version [...] rt.cpan.org, undisclosed-recipients:;
From:	John Peacock <john.peacock [...] havurah-software.org>

On 02/01/2014 03:56 PM, Daniel Dragan via RT wrote: Show quoted text

> SVt_PVMG does not determine if a SV is magical or not. A magical can also be SVt_PVLV.

As you can probably imagine, I was trying to determine if I was dealing with a tied hash (as produced by Readonly), but I was unable to come up with a test that would work. Show quoted text

> Just check flags alone. I suggest checking the private > flags after all public flags are checked, otherwise there will be > more complaints over (some details ahead may be wrong), a version > number that is IV 2^54 on a 64 bit IV machine, that had something > like

That isn't a case we need to directly handle, since it will trigger the VERSION_MAX code. I reordered to the tests to check IV, NV, then PV, which I think will always DTRT (SvIOK will still be set for those overly large numbers where the NV might no longer be accurate). I moved all of the private flags to the end as you suggested. I was resistant to the goto methodology, but it does make the extra code slightly tidier. I've pushed my changes to both repos. John

Sat Feb 01 19:19:29 2014 jpeacock [...] cpan.org - Correspondence added

Fixed in 0.9908

Sat Feb 01 19:19:31 2014 jpeacock [...] cpan.org - Status changed from 'open' to 'resolved'