Bug #94079 for Gearman-XS: Gearman::XS::Client leaks previous callbacks if they are re-set

The Gearman::XS::Client object doesn't handle references to client callbacks correctly. All is well if code only assigns each callback once. However, if code takes a client and assigns any callback a second time, the previous closure is leaked. The last callback assigned is correctly freed on object destruction, which means that there is no leak if callbacks are assigned at most once. I suspect that this has gone unnoticed because most users don't re-assign callbacks. I guess they either create a client, set it up with callbacks, and use it repeatedly, or they create a client, set up custom callbacks, use it once, then release the client. In both scenarios all memory is correctly freed. The code is here is somewhat quirky, in that it creates a client once that is re-used, but for each use sets up a new callback. Hence the callbacks leak. What makes it really noticeable is that the assigned callback closes over a large (temporary) data structure. Well, it's meant to be temporary... :-( Attached patch fixes this bug. I'm happy to assign copyright on the new test to Data Differential, hence the copyright notice I put in it. Thanks for the module. It's useful. Nicholas Clark

commit a5c9e59440b3dfa0d5faeedf20fa1b8512a3f7ec Author: Nicholas Clark <nick@ccl4.org> Date: Fri Mar 21 11:36:43 2014 +0100 When setting a callback the second time, don't leak any previous callback. diff --git a/Client.xs b/Client.xs index 1bb893f..8475900 100644 --- a/Client.xs +++ b/Client.xs @@ -566,6 +566,8 @@ set_created_fn(self, fn) gearman_xs_client *self SV * fn CODE: + /* Release any previous callback. Safe because sv_free() handles NULL). */ + sv_free(self->created_fn); self->created_fn= newSVsv(fn); gearman_client_set_created_fn(self->client, _perl_task_created_fn); @@ -574,6 +576,7 @@ set_data_fn(self, fn) gearman_xs_client *self SV * fn CODE: + sv_free(self->data_fn); self->data_fn= newSVsv(fn); gearman_client_set_data_fn(self->client, _perl_task_data_fn); @@ -582,6 +585,7 @@ set_complete_fn(self, fn) gearman_xs_client *self SV * fn CODE: + sv_free(self->complete_fn); self->complete_fn= newSVsv(fn); gearman_client_set_complete_fn(self->client, _perl_task_complete_fn); @@ -590,6 +594,7 @@ set_fail_fn(self, fn) gearman_xs_client *self SV * fn CODE: + sv_free(self->fail_fn); self->fail_fn= newSVsv(fn); gearman_client_set_fail_fn(self->client, _perl_task_fail_fn); @@ -598,6 +603,7 @@ set_status_fn(self, fn) gearman_xs_client *self SV * fn CODE: + sv_free(self->status_fn); self->status_fn= newSVsv(fn); gearman_client_set_status_fn(self->client, _perl_task_status_fn); @@ -606,6 +612,7 @@ set_warning_fn(self, fn) gearman_xs_client *self SV * fn CODE: + sv_free(self->warning_fn); self->warning_fn= newSVsv(fn); gearman_client_set_warning_fn(self->client, _perl_task_warning_fn); diff --git a/MANIFEST b/MANIFEST index da39fcf..170b85c 100644 --- a/MANIFEST +++ b/MANIFEST @@ -28,6 +28,7 @@ t/04-live.t t/05-live-worker.t t/06_live-worker-timeout.t t/80-memleak.t +t/81-callbackleak.t t/99-perlcritic.t t/lib/TestLib.pm t/perlcriticrc diff --git a/t/81-callbackleak.t b/t/81-callbackleak.t new file mode 100644 index 0000000..84b7d32 --- /dev/null +++ b/t/81-callbackleak.t @@ -0,0 +1,93 @@ +# Gearman Perl front end +# Copyright (C) 2014 Data Differential, http://datadifferential.com/ +# All rights reserved. +# +# This library is free software; you can redistribute it and/or modify +# it under the same terms as Perl itself, either Perl version 5.8.9 or, +# at your option, any later version of Perl 5 you may have available. + +use strict; +use warnings; + +use Test::More; +use Gearman::XS::Client; + +our $count = 0; +{ + package Counter; + + sub new { + ++$count; + bless [], shift; + } + + sub DESTROY { + --$count; + } +} + +sub gen_callback { + my $counter = Counter->new(); + return sub { + $counter; + } +} + +# Simple test: +my $client = Gearman::XS::Client->new(); +isa_ok($client, 'Gearman::XS::Client'); + +is($count, 0, 'At the start we have no objects'); + +$client->set_created_fn(gen_callback()); + +is($count, 1, 'One callback'); + +undef $client; + +is($count, 0, 'No callbacks'); + +# Repeat test: +$client = Gearman::XS::Client->new(); + +$client->set_created_fn(gen_callback()); + +is($count, 1, 'One callback'); + +$client->set_created_fn(gen_callback()); + +is($count, 1, 'Still one callback'); + +$client->set_created_fn(gen_callback()); + +is($count, 1, 'Still one callback'); + +undef $client; + +is($count, 0, 'No callbacks'); + +# All the callbacks: +my @callbacks = qw(created data complete fail status warning); +my %setters = map {$_ , "set_${_}_fn"} @callbacks; +my $all = scalar @callbacks; +$client = Gearman::XS::Client->new(); + +foreach my $setter (values %setters) { + $client->$setter(gen_callback()); +} + +is($count, $all, "All $all callbacks"); + +while (my ($callback, $setter) = each %setters) { + $client->$setter(gen_callback()); + is($count, $all, "Still $all callbacks after setting $callback again"); + + $client->$setter(gen_callback()); + is($count, $all, "Still $all callbacks after setting $callback again"); +} + +undef $client; + +is($count, 0, 'No callbacks'); + +done_testing();