Bug #29791 for DBD-Pg: Memory leak with savepoints (lack of mortalization)

At AirWave, we noticed savepoint name strings getting leaked (we use savepoints extensively). It turns out they're popped off of imp_dbh->savepoints, but their REFCNT remains > 0, leaking. Also, pg_db_release() and pg_db_rollback_to() were duplicating code (the leak existed in both copies). Attached are a script for reproducing/exposing the leak and a patch which fixes it. After an __END__, the script contains run results, environment details, and a pair of results from "make test" with and without the patch. Please let me know if you have any questions. Thanks, Darrell Bishop (darrell.bishop@airwave.com - AirWave Wireless) [root@pico tmp]# uname -a Linux pico.dev.airwave.com 2.6.18-8.el5 #1 SMP Sun Jul 22 17:06:49 PDT 2007 i686 i686 i386 GNU/Linux [root@pico tmp]# perl -V Summary of my perl5 (revision 5 version 8 subversion 8) configuration: Platform: osname=linux, osvers=2.6.18-8.el5, archname=i386-linux-thread-multi uname='linux pico.dev.airwave.com 2.6.18-8.el5 #1 smp sun jul 22 17:06:49 pdt 2007 i686 i686 i386 gnulinux ' config_args='-des -Doptimize=-g -Dversion=5.8.8 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dinstallprefix=/usr -Dprefix=/usr -Dccflags=-DDEBUGGING -DDEBUGGING_MSTATS -Dusemymalloc=y -Darchname=i386-linux -Dvendorprefix=/usr -Dsiteprefix=/usr -Duseshrplib -Dusethreads -Duseithreads -Duselargefiles -Dd_dosuid -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dinc_version_list=5.8.7 5.8.6 5.8.5 -Dscriptdir=/usr/bin' hint=recommended, useposix=true, d_sigaction=define usethreads=define use5005threads=undef useithreads=define usemultiplicity=define useperlio=define d_sfio=undef uselargefiles=define usesocks=undef use64bitint=undef use64bitall=undef uselongdouble=undef usemymalloc=y, bincompat5005=undef Compiler: cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DDEBUGGING -DDEBUGGING_MSTATS -fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm', optimize='-g', cppflags='-D_REENTRANT -D_GNU_SOURCE -DDEBUGGING -DDEBUGGING_MSTATS -fno-strict-aliasing -pipe -Wdeclaration-after-statement -I/usr/local/include -I/usr/include/gdbm' ccversion='', gccversion='3.4.6 20060404 (Red Hat 3.4.6-8)', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='gcc', ldflags =' -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc libc=/lib/libc-2.3.4.so, so=so, useshrplib=true, libperl=libperl.so gnulibc_version='2.3.4' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/lib/perl5/5.8.8/i386-linux-thread-multi/CORE' cccdlflags='-fPIC', lddlflags='-shared -g -L/usr/local/lib' Characteristics of this binary (from libperl): Compile-time options: DEBUGGING MULTIPLICITY MYMALLOC PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP USE_ITHREADS USE_LARGE_FILES USE_PERLIO USE_REENTRANT_API Built under linux Compiled at Sep 10 2007 16:37:24 %ENV: PERL5LIB="/root/svn/mercury/common/lib/perl:/root/svn/mercury/lib/perl:/root/svn/devtoys/rally/lib:/root/svn/devtoys/bug_emails" @INC: /root/svn/mercury/common/lib/perl /root/svn/mercury/lib/perl /root/svn/devtoys/rally/lib /root/svn/devtoys/bug_emails /usr/lib/perl5/site_perl/5.8.8/i386-linux-thread-multi /usr/lib/perl5/site_perl/5.8.7/i386-linux-thread-multi /usr/lib/perl5/site_perl/5.8.6/i386-linux-thread-multi /usr/lib/perl5/site_perl/5.8.5/i386-linux-thread-multi /usr/lib/perl5/site_perl/5.8.8 /usr/lib/perl5/site_perl/5.8.7 /usr/lib/perl5/site_perl/5.8.6 /usr/lib/perl5/site_perl/5.8.5 /usr/lib/perl5/site_perl /usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi /usr/lib/perl5/vendor_perl/5.8.7/i386-linux-thread-multi /usr/lib/perl5/vendor_perl/5.8.6/i386-linux-thread-multi /usr/lib/perl5/vendor_perl/5.8.5/i386-linux-thread-multi /usr/lib/perl5/vendor_perl/5.8.8 /usr/lib/perl5/vendor_perl/5.8.7 /usr/lib/perl5/vendor_perl/5.8.6 /usr/lib/perl5/vendor_perl/5.8.5 /usr/lib/perl5/vendor_perl /usr/lib/perl5/5.8.8/i386-linux-thread-multi /usr/lib/perl5/5.8.8 .

diff -u -r DBD-Pg-1.49-orig/dbdimp.c DBD-Pg-1.49/dbdimp.c --- DBD-Pg-1.49-orig/dbdimp.c 2006-05-03 19:11:14.000000000 -0700 +++ DBD-Pg-1.49/dbdimp.c 2007-10-04 23:26:49.000000000 -0700 @@ -2975,6 +2975,24 @@ } +/* Pop off savepoints to the specified savepoint name */ +static void pg_db_free_savepoints_to (dbh, imp_dbh, savepoint) + SV *dbh; + imp_dbh_t *imp_dbh; + char * savepoint; +{ + I32 i; + for (i = av_len(imp_dbh->savepoints); i >= 0; i--) { + SV *elem = av_pop(imp_dbh->savepoints); + if (strEQ(SvPV_nolen(elem), savepoint)) { + sv_2mortal(elem); + break; + } + sv_2mortal(elem); + } +} + + /* ================================================================== */ int pg_db_rollback_to (dbh, imp_dbh, savepoint) SV *dbh; @@ -3007,12 +3025,7 @@ return 0; } - for (i = av_len(imp_dbh->savepoints); i >= 0; i--) { - SV *elem = *av_fetch(imp_dbh->savepoints, i, 0); - if (strEQ(SvPV_nolen(elem), savepoint)) - break; - (void)av_pop(imp_dbh->savepoints); - } + pg_db_free_savepoints_to(dbh, imp_dbh, savepoint); return 1; } @@ -3049,11 +3062,7 @@ return 0; } - for (i = av_len(imp_dbh->savepoints); i >= 0; i--) { - SV *elem = av_pop(imp_dbh->savepoints); - if (strEQ(SvPV_nolen(elem), savepoint)) - break; - } + pg_db_free_savepoints_to(dbh, imp_dbh, savepoint); return 1; }