Re: Possible memory leak in DBI

On 22/02/11 18:36, Martin J. Evans wrote:
> On 22/02/2011 17:40, Tim Bunce wrote:
>> On Mon, Feb 21, 2011 at 05:59:34PM +0000, Martin J. Evans wrote:
>>> It would appear this problem and possibly other leaks are attracting quite a lot of attention now. Someone else just posted another cpan rating mentioning leaks at:
>>>
>>> http://cpanratings.perl.org/dist/DBD-Pg#8254
>>>
>>> I narrowed down one of the problems (below) but it is beyond me now - sorry.
>> Using perl 5.12.2 I couldn't reproduce it with:
>>
>> perl -MDBI -we '$a = DBI::_concat_hash_sorted({1..10000},"=",",",1,1) while 1'
>>
>> nor with:
>>
>> perl -Mblib -MDBI -we '$sth=DBI->connect("dbi:NullP:",0,0,{ShowErrorStatement=>1})->prepare("ERROR 1 err"); $sth->execute(1..10000) while 1' 2> /dev/null
>>
>> The ticket says "Other DBI drivers do not seem to leak" so I suspect the
>> leak is in DBD::Pg's dbd_st_FETCH_attrib function (in dbdimp.c) where it
>> handles the ParamValues attribute by constructing a hash. (That's called
>> from DBI.xs by the mg_get(*svp);)
>>
> Thanks. I was not suggesting the problem was in DBI, just that by ifdeffing some code out in DBI the problem goes away and that might help identify the problem.
>
>> I've no time to dig deeper, sorry. (I'll copy the above to the ticket.)
>>
>> Tim.
>>
> This may be useful:
>
> http://www.perlmonks.org/?node_id=52609
>
> as I noted, I use hv_store in DBD::ODBC and DBD::Pg uses hv_store_ent.
>
> It appears if you use hv_store_ent you may need to look more closely at the key - see the link above.
>
> Hopefully this helps Greg. I don't have access to a postgres db right now to test it out.
>
> Martin

I found some time to look at this and I think there are 3 problems indbd_st_FETCH_attrib

1. hv_store_ent is called with a key SV and a val SV but if hv_store_ent fails (returns NULL) the reference count on the val SV is not decremented - don't think we are seeing this here but it is a possible bug.

2. hv_store_ent is passed a newly create key SV and a val SV and hv_store_ent takes the reference count on the val but not the key so the reference count on the key needs to be decremented.

3. the return statement is "return retsv" and I think it should be "return sv_2mortal(retsv)"

The test code provided for the leak no longer leaks if you apply the attached patch.

Martin
-- 
Martin J. Evans
Easysoft Limited
http://www.easysoft.com

>>> Martin
>>> --
>>> Martin J. Evans
>>> Easysoft Limited
>>> http://www.easysoft.com
>>>
>>> On 13/01/11 18:53, Martin J. Evans wrote:
>>>> I briefly looked at the DBD::Pg rt ticket https://rt.cpan.org/Ticket/Display.html?id=60863 and I'm not sure this is a bug in DBD::Pg and think it might be in DBI. Since ShowErrorStatement set causes the leak and unset the leak goes away I isolated it to some code in DBI.xs:
>>>>
>>>> if ( DBIc_has(imp_xxh, DBIcf_ShowErrorStatement)
>>>> && !is_unrelated_to_Statement
>>>> && (DBIc_TYPE(imp_xxh) == DBIt_ST || ima_flags& IMA_SHOW_ERR_STMT)
>>>> && (statement_svp = hv_fetch((HV*)SvRV(h), "Statement", 9, 0))
>>>> && statement_svp&& SvOK(*statement_svp)
>>>> ) {
>>>> SV **svp = 0;
>>>> sv_catpv(msg, " [for Statement \"");
>>>> sv_catsv(msg, *statement_svp);
>>>>
>>>> /* fetch from tied outer handle to trigger FETCH magic */
>>>> /* could add DBIcf_ShowErrorParams (default to on?) */
>>>> #ifdef THIS_ONE_STOPS_LEAK
>>>> if (!(ima_flags& IMA_HIDE_ERR_PARAMVALUES)) {
>>>> svp = hv_fetch((HV*)DBIc_MY_H(imp_xxh),"ParamValues",11,FALSE);
>>>> if (svp&& SvMAGICAL(*svp))
>>>> mg_get(*svp); /* XXX may recurse, may croak. could use eval */
>>>> }
>>>> #endif
>>>> if (svp&& SvRV(*svp)&& SvTYPE(SvRV(*svp)) == SVt_PVHV&& HvKEYS(SvRV(*svp))>0 ) {
>>>> #ifndef THESE_TWO_WITHOUT_ABOVE_DOES_NOT_STOP_LEAK
>>>> SV *param_values_sv = sv_2mortal(_join_hash_sorted((HV*)SvRV(*svp), "=",1, ", ",2, 1, -1));
>>>> #endif
>>>> sv_catpv(msg, "\" with ParamValues: ");
>>>> #ifndef THESE_TWO_WITHOUT_ABOVE_DOES_NOT_STOP_LEAK
>>>> sv_catsv(msg, param_values_sv);
>>>> #endif
>>>> sv_catpvn(msg, "]", 1);
>>>> }
>>>> else {
>>>> sv_catpv(msg, "\"]");
>>>> }
>>>> }
>>>>
>>>> I was suspicious about _join_hash_sorted but it appears if you just take THESE_TWO_WITHOUT_ABOVE_DOES_NOT_STOP_LEAK out it still leaks.
>>>>
>>>> If you just take THIS_ONE_STOPS_LEAK (which obviously also takes out the other 2) the leak goes away.
>>>>
>>>> I've not duplicated the problem with any other DBDs as yet but I'm running the exact code in the RT.
>>>>
>>>> I've no idea as yet what the actual problem is.
>>>>
>>>> I was running Perl 5.10.1 on Linux (32bit) but no one who has tried this yet (on multiple Perls in Linux) has failed to see the issue.
>>>>
>>>> With THIS_ONE_STOPS_LEAK defined I get:
>>>>
>>>> martin@bragi:/tmp/DBD-Pg-2.17.2$ perl -I blib/lib -I blib/arch leak.pl
>>>> Cycles: 2000 Proc size: 13700K
>>>> Cycles: 4000 Proc size: 13700K
>>>> Cycles: 6000 Proc size: 13700K
>>>> Cycles: 8000 Proc size: 13700K
>>>> Cycles: 10000 Proc size: 13700K
>>>> Cycles: 12000 Proc size: 13700K
>>>> Cycles: 14000 Proc size: 13700K
>>>> Cycles: 16000 Proc size: 13700K
>>>> Cycles: 18000 Proc size: 13700K
>>>>
>>>> and a stock DBI from trunk) I get:
>>>>
>>>> martin@bragi:/tmp/DBD-Pg-2.17.2$ perl -I blib/lib -I blib/arch leak.pl
>>>> Cycles: 2000 Proc size: 16792K
>>>> Cycles: 4000 Proc size: 19892K
>>>> Cycles: 6000 Proc size: 22980K
>>>> Cycles: 8000 Proc size: 26200K
>>>> Cycles: 10000 Proc size: 29296K
>>>> Cycles: 12000 Proc size: 32376K
>>>> Cycles: 14000 Proc size: 35472K
>>>> Cycles: 16000 Proc size: 38692K
>>>> Cycles: 18000 Proc size: 41796K
>>>>
>>>> Martin
>>>>
>
>

Index: dbdimp.c
===================================================================
--- dbdimp.c	(revision 14717)
+++ dbdimp.c	(working copy)
@@ -973,18 +973,22 @@
 			ph_t *currph;
 			int i;
 			for (i=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,i++) {
+                SV *key, *val;
+                key = (3==imp_sth->placeholder_type ? newSVpv(currph->fooname,0) : newSViv(i+1));
+ 
 				if (NULL == currph->value) {
-					(void)hv_store_ent 
-						(pvhv,
-						 (3==imp_sth->placeholder_type ? newSVpv(currph->fooname,0) : newSViv(i+1)),
-						 newSV(0), 0);
+                    val = newSV(0);
+					if (!hv_store_ent(pvhv, key, val, 0)) {
+                        SvREFCNT_dec(val) ;
+                    }
 				}
 				else {
-					(void)hv_store_ent
-						(pvhv,
-						 (3==imp_sth->placeholder_type ? newSVpv(currph->fooname,0) : newSViv(i+1)),
-						 newSVpv(currph->value,0),0);
+                    val = newSVpv(currph->value,0);
+					if (!hv_store_ent(pvhv, key, val, 0)) {
+                        SvREFCNT_dec(val) ;
+                    }
 				}
+                SvREFCNT_dec(key) ;
 			}
 			retsv = newRV_noinc((SV*)pvhv);
 		}
@@ -1037,7 +1041,7 @@
 
 	if (retsv != Nullsv) {
 		if (TEND) TRC(DBILOGFP, "%sEnd dbd_st_FETCH_attrib\n", THEADER);
-		return retsv;
+		return sv_2mortal(retsv);
 	}
 
 	if (! imp_sth->result) {