Fix memory leak in Data::Dumper::Dumper

[niner]

When Dumper was called without arguments and in list context it leaked the
preallocated retval scalar.

[jkeenan]

When Dumper was called without arguments and in list context it leaked the
preallocated retval scalar.

How could one observe this leakage?

[niner]

I added the example to the commit message: perl -MData::Dumper -e '() = Dumper while 1'

[niner]

All the smoke test failures seem to be because of Data::Dumper's version staying the same despite changes to the source.

[jkeenan]

All the smoke test failures seem to be because of Data::Dumper's version staying the same despite changes to the source.

Corrected and pushed for smoking to branch smoke-me/jkeenan/niner/ghpr-17673-dumper-leak.

[Leont]

I think the fix is correct but surprising.

I think it would be clearer to just always create retval as a mortal. That way the right thing happens whether it is pushed or not. Or am I missing something?

[dur-randir]

I think it would be clearer to just always create retval as a mortal. That way the right thing happens whether it is pushed or not. Or am I missing something?

I felt horrified reading that code. There're two places where it's created, so both should be updated. And there're at least two more problems with this patch's approach:

• looks like it should unconditionally be freed on the 'else' branch, not under 'imax < 0)'
• croaking when hit by a style.maxrecursed limit leaks - and creating mortal also helps here

[smpeters]

Looks like this is formatted incorrectly.

[tonycoz]

No, it's a tabs vs spaces, the if (gimme... is indented with a tab, while the else is just spaces.

[toddr]

Given the leak is not critical, I assume it should not go into 5.32 at this point?

[jkeenan]

Given the leak is not critical, I assume it should not go into 5.32 at this point?

Since 5.33.0 has been released, this ticket is now eligible for further work. Can the contributors take another look?

Thank you very much.
Jim Keenan

[tonycoz]

This was fixed by 4146316