Bug #122083 for PHP-Serialization-XS: memory hog bug

Tue Jun 13 00:59:40 2017 Rune.Hylleberg [...] otto-office.com - Ticket created

Subject:	memory hog bug
Date:	Tue, 13 Jun 2017 04:44:04 +0000
To:	"bug-PHP-Serialization-XS [...] rt.cpan.org" <bug-PHP-Serialization-XS [...] rt.cpan.org>
From:	"Hylleberg, Rune" <Rune.Hylleberg [...] otto-office.com>

Hi, I think I found a memory hog bug. If I run this code use strict; use warnings; use Data::Dumper; #use PHP::Serialization qw(serialize unserialize); use PHP::Serialization::XS qw(serialize unserialize); my $_iCount = 0; while ($_iCount++ < 100000) { my $str = serialize({'test1' => {'deep' => 'hash', 'with' => ['a', 'r', 'r', 'a', 'y']}, 'test2' => 'done'}); my $href = unserialize($str); if (! ($_iCount % 1000)) { open(my $fh, "<", "/proc/$$/status"); while (my $_sLine = <$fh>) { print $_iCount.' - '.$_sLine if $_sLine =~ /VmSize:\s(.)/; } close $fh; } } The memory usage goes up, whereas if I use PHP::Serialization it is steady. (Tested on Suse perl 5.10 and Redhat perl 5.16) Cheers Rune Hylleberg Programmer OTTO Office GmbH & Co KG · Fabriciusstr. 105a · 22177 Hamburg Telefon: +49 40-650 44-462 · Fax: +49 40-650 4458-462 Rune.Hylleberg@otto-office.com<mailto:Rune.Hylleberg@otto-office.com> · www.otto-office.com<http://www.otto-office.com> -- Sitz der Gesellschaft: Fabriciusstr. 105a · 22177 Hamburg · AG Hamburg · HRA 90433 Persönlich haftend: Verwaltungsgesellschaft OTTO Office mbH · AG Hamburg · HRB 62764 USt.-ID-Nr. DE185856673 · Geschäftsführer: Uwe Orgas (Vorsitzender) · Roy Vieregge Ein Unternehmen der Hans R. Schmid Holding AG

Tue Jun 13 08:23:16 2017 kulp [...] cpan.org - Correspondence added

Thank you for the report and the test case. I plan to look into this issue shortly. -- --kulp

Tue Jun 13 08:23:17 2017 The RT System itself - Status changed from 'new' to 'open'

Tue Jun 13 08:23:22 2017 kulp [...] cpan.org - Taken

Sun Jun 18 23:01:14 2017 kulp [...] cpan.org - Correspondence added 60 min

I confirm the bug exists for me on both Debian Linux (perl 5.20) and macos Sierra (perl 5.18). I found one part of the memory leak, but as I am not very familiar with XS it is taking me a while to find it all. -- --kulp

Thu Jun 22 02:10:50 2017 kulp [...] cpan.org - Correspondence added 90 min

The attached patch resolves the multiple memory leaks (now the original testcase shows 0KiB increase in memory usage over 100000 iterations). The module actually gets a bit faster also, when not used in an object-oriented way, as a side effect of not creating unnecessary temporary objects. I hope to make a new release of the module in the next week or so. -- --kulp

Subject:

no-leak.patch

diff --git a/XS.xs b/XS.xs index 2c1304e..98a7f23 100644 --- a/XS.xs +++ b/XS.xs @@ -18,7 +18,7 @@ typedef struct self { static char _error_msg[256] = "Unknown error"; static void _register_error(const char *msg) { - strncpy(_error_msg, msg, sizeof _error_msg); + my_strlcpy(_error_msg, msg, sizeof _error_msg); } static void _croak(const char *msg) @@ -103,7 +103,6 @@ new(char *class, ...) /// super-class's new /// @todo permit passing parameters to this call me->parent = eval_pv("PHP::Serialization->new", true); - SvREFCNT_inc(me->parent); SvREFCNT_inc(RETVAL); OUTPUT: RETVAL @@ -128,6 +127,7 @@ _c_decode(self me, SV *input, ....) RETVAL = _convert_recurse(node, me->flags, claxx); + ps_read_string_fini(ps_state); ps_free(node); ps_fini(&ps_state); OUTPUT: diff --git a/convert.c b/convert.c index 0840aac..547e50d 100644 --- a/convert.c +++ b/convert.c @@ -38,7 +38,7 @@ _convert_recurse(const ps_node *node, int flags, const char *prefix) a = (SV*)newHV(); for (int i = 0; i < what->len; i++) { STRLEN len; - char *key = SvPV(_convert_recurse(what->pairs[i].key, flags, prefix), len); + char *key = SvPV(sv_2mortal(_convert_recurse(what->pairs[i].key, flags, prefix)), len); SV *val = _convert_recurse(what->pairs[i].val, flags, prefix); hv_store((HV*)a, key, len, val, 0); @@ -50,7 +50,7 @@ _convert_recurse(const ps_node *node, int flags, const char *prefix) av_push((AV*)a, _convert_recurse(what->pairs[i].val, flags, prefix)); } - result = newRV(a); + result = newRV_noinc(a); if (typename) { bool should_free = false; char *built = typename; diff --git a/lib/PHP/Serialization/XS.pm b/lib/PHP/Serialization/XS.pm index 280ce0a..8c8c6ed 100644 --- a/lib/PHP/Serialization/XS.pm +++ b/lib/PHP/Serialization/XS.pm @@ -27,10 +27,12 @@ XSLoader::load('PHP::Serialization::XS', $VERSION); # in XS sub new; +my $default = __PACKAGE__->new(%DEFAULT_OPTS); + sub unserialize { my ($str, $class) = @_; - return __PACKAGE__->new(%DEFAULT_OPTS)->decode($str, $class); + return $default->decode($str, $class); } sub serialize @@ -41,7 +43,7 @@ sub serialize sub decode { my ($self, $str, $class) = @_; - $self = $self->new(%DEFAULT_OPTS) unless ref $self; + $self = $default unless ref $self; return $self->_c_decode($str || "", $class); }

Thu Jun 22 02:10:50 2017 kulp [...] cpan.org - Status changed from 'open' to 'patched'

Fri Jun 23 22:51:52 2017 kulp [...] cpan.org - Correspondence added 90 min

New release 0.09 resolves this memory leak. Changes can be seen on GitHub at the URL below. The new release has been uploaded to PAUSE. https://github.com/kulp/PHP-Serialization-XS/compare/0.08...0.09 Danke schön ! -- --kulp

Fri Jun 23 22:52:03 2017 kulp [...] cpan.org - Status changed from 'patched' to 'resolved'

Sat Jun 24 00:04:14 2017 kulp [...] cpan.org - Correspondence added

Release 0.09 had a latent MANIFEST issue, and I think PAUSE failed to process it anyway, so it may never appear on CPAN. Version 0.10 has instead been uploaded. On Fri Jun 23 22:51:52 2017, KULP wrote: Show quoted text

> New release 0.09 resolves this memory leak. Changes can be seen on > GitHub at the URL below. The new release has been uploaded to PAUSE. > > https://github.com/kulp/PHP-Serialization-XS/compare/0.08...0.09 > > Danke schön !

-- --kulp