Bug #56722 for Test-Weaken: leaks on object with overloaded "+"

Mon Apr 19 14:38:38 2010 user42 [...] zip.com.au - Ticket created

Subject:	leaks on object with overloaded "+"
Date:	Tue, 20 Apr 2010 04:38:07 +1000
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

Test::Weaken 3.002 seems to run an overloaded "+" on objects traced, where I hoped it would not. bigint.pl below reports no leaks, but I think it should say $x leaked. And overload.pl below dies with I am not in the adding mood at /usr/share/perl5/Test/Weaken.pm line 163 I struck this with perl-glib where "flags" are a scalar ref with overloaded "+". They don't allow addition with a number like "$flags+0" (only another flags of its own kind, and some strings). I see Scalar::Util::refaddr is imported, and noted in the docs, I suppose it has the advantage over numizing "$obj+0" of avoiding overload.pm nonsense.

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

Mon Apr 19 15:46:30 2010 jkegl [...] cpan.org - Taken

Mon Apr 19 15:47:12 2010 jkegl [...] cpan.org - Status changed from 'new' to 'open'

Mon Apr 19 15:47:13 2010 jkegl [...] cpan.org - Severity Important added

Mon Apr 19 22:18:18 2010 jkegl [...] cpan.org - Correspondence added

@Kevin: Thanks for the excellent bug report, including a description of the correct fix and a good test. I've put them into Test::Weaken and uploaded it as 3.003_000. This fix is especially version- and environment-dependent, and I'll wait a few days for the verdict of cpantesters. In the process I also fixed a second bug -- T::W also could be subverted if 'bool' was overloaded. I'm not sure what happens if dereferencing is overloaded and don't really want to think about it. :-) 5.12 has a 'no overloading' pragma, and if it does what reading the docs makes me think it does, that is the *REAL* fix. But using it means abandoning support of 5.8 and 5.10, which for a debugging tool like T::W seems like too high a price to pay -- in theory a user can control the Perl version they develop on, but there's no controlling the versions you need to debug.

Fri Apr 23 21:52:50 2010 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #56722] leaks on object with overloaded "+"
Date:	Sat, 24 Apr 2010 11:52:24 +1000
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > This fix is especially version- and environment-dependent, and I'll wait > a few days for the verdict of cpantesters.

refaddr itself should be pretty safe. Reading the changes file suggests Scalar::Util 1.18 has a bug fix for the pure-perl refaddr() though, which may be worth requiring. Show quoted text

> In the process I also fixed a second bug -- T::W also could be subverted > if 'bool' was overloaded.

I didn't notice that. Do you have to check anything as a bool? Show quoted text

> 5.12 has a 'no overloading' pragma,

Scalar::Util::PP avoids overloads by a temporary re-blessing. Works, but a bit scary :-). Show quoted text

> 5.8 and 5.10

I expect some eval{} at BEGIN time could do it the one or other way, if in fact necessary.

Thu Apr 29 22:22:56 2010 jkegl [...] cpan.org - Correspondence added

All tests were perfect on 3.003_000, but I decided that not explicitly requiring version 1.18 of Scalar::Util was too much of a loose end. So I've put that fix into 3.003_001, a new developer's release, released just now. @Kevin: Thanks for pointing out this dependency. I'll let it go through the usual cpantesters cycle, and make it an official release if all goes well. On Fri Apr 23 21:52:50 2010, user42@zip.com.au wrote: Show quoted text

> I didn't notice that. Do you have to check anything as a bool?

I had a check on defined-ness that I implemented as a bool. It was defined-ness of a reference and no valid, defined, reference is ever a Perl false. The possibility of overloading didn't occur to me, frankly. I've changed the test so that it is explicitly for defined-ness. I probably should do something about overloading of dereferencing. But I'm not sure what the right thing to do would be. I think that's something I leave until after this official release. @Kevin: Do you have an opinion about deleting old versions of Test::Weaken? I want to be a good citizen and keep CPAN clean, but I certainly don't want to risk pulling the rug out from under users.

Mon May 03 19:59:17 2010 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #56722] leaks on object with overloaded "+"
Date:	Tue, 04 May 2010 09:58:07 +1000
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > I probably should do something about overloading of dereferencing.

I suppose test::weaken should look for actual memory stuff, not what some object generates through overload code. The temporary unblessing like Scalar::Util::PP might be the way to go, though it seems a touch scary. Show quoted text

> @Kevin: Do you have an opinion about deleting old versions of > Test::Weaken? I want to be a good citizen and keep CPAN clean, but I > certainly don't want to risk pulling the rug out from under users.

For my own stuff I don't bother leaving any old versions, as the latest is supposed to be fully compatible and always an improvement over the past :-). You could keep the newest of each major feature if you wanted, otherwise it's all always on backpan. I wish the cpan dist pages would show backpan old versions, even if it didn't index them etc.

Sun May 16 18:52:04 2010 jkegl [...] cpan.org - Correspondence added

RT-Send-CC:

user42 [...] zip.com.au

This fix looks good, so I've just uploaded official release Test-Weaken- 3.004000. Per my usual practice, I'll await the verdict of cpantesters before closing this ticket. As for overloaded dereferencing, I think for now I'll just (in another release) document the issue. Present behavior is to honor the overloading, but my guess (like Kevin's) is that it is better to override the overloading. But there have been no complaints, and if I change it I run the risk of breaking a behavior that is working in favor of a "fix" for which there is no demand. On Mon May 03 19:59:17 2010, user42@zip.com.au wrote: Show quoted text

> "Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes:

> > > > I probably should do something about overloading of dereferencing.

> > I suppose test::weaken should look for actual memory stuff, not what > some object generates through overload code. The temporary unblessing > like Scalar::Util::PP might be the way to go, though it seems a touch > scary. >

> > @Kevin: Do you have an opinion about deleting old versions of > > Test::Weaken? I want to be a good citizen and keep CPAN clean, but

I Show quoted text

> > certainly don't want to risk pulling the rug out from under users.

> > For my own stuff I don't bother leaving any old versions, as the

latest Show quoted text

> is supposed to be fully compatible and always an improvement over the > past :-). > > You could keep the newest of each major feature if you wanted,

otherwise Show quoted text

> it's all always on backpan. I wish the cpan dist pages would show > backpan old versions, even if it didn't index them etc.

Mon May 17 20:33:04 2010 user42 [...] zip.com.au - Correspondence added

Subject:	Re: [rt.cpan.org #56722] leaks on object with overloaded "+"
Date:	Tue, 18 May 2010 10:31:48 +1000
To:	bug-Test-Weaken [...] rt.cpan.org
From:	Kevin Ryde <user42 [...] zip.com.au>

"Jeffrey Kegler via RT" <bug-Test-Weaken@rt.cpan.org> writes: Show quoted text

> > better to override the overloading.

Maybe overloaded scalar deref is the same as a tied scalar and tied array or hash, if there was a consistent approach to not running the object's code in such cases. Not running would suit if the fetched result was something dynamically created and thus not existing storage whose destruction is to be checked. Do XS level magic handlers do similar things? It may be difficult and even undesirable to bypass them though.

Tue Jun 22 13:50:49 2010 jkegl [...] cpan.org - Correspondence added

Fixed in 3.004000.

Tue Jun 22 13:50:52 2010 jkegl [...] cpan.org - Status changed from 'open' to 'resolved'