Thread: [CHECKER] 4 memory leaks in Postgresql 7.4.2
Sorry if this is a repeat. I checked the mailing list archive and this message didn't show up, so I am reposting it. ------------------------------------------------------------------------ ---------------------------------------- Hi, I'm from the Stanford Metacompilation research group where we use static analysis to find bugs. I'm trying a new technique, so I would appreciate feedback on these error reports. This technique tries to infer API roles automatically from the code. In particular, I am trying to check for ownership properties of functions (i.e., which functions return ownership of a pointer, which ones claim it, etc.). This allows us to potentially find memory leaks that we didn't check for before because we didn't know what functions allocated memory or acquired resources. The tool is still in the early stages, so the bugs reported may be false positives. I have applied it to Postgresql 7.4.2. Included below are 4 error reports that I believe to be memory leaks. Any feedback or confirmation of bugs would be appreciated. Best, Ted Kremenek ------------------------------------------------------------------------ ------------------------------------------ [BUG] memory leak of vacrelstats at end of function laxy_vaccum_rel() File where bug occurred: postgresql-7.4.2/src/backend/commands/vacuumlazy.c Diagnosis: (1) palloc0 returns memory at line 141 and address is stored in vacrelstats (2) vaccrelstats goes out of scope at the end of the function; leaking the memory 131 BlockNumber possibly_freeable; 132 133 if (vacstmt->verbose) 134 elevel = INFO; 135 else 136 elevel = DEBUG2; 137 138 vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared, 139 &OldestXmin, &FreezeLimit); 140 ---> vacrelstats is allocated 141 vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats)); 142 143 /* Set threshold for interesting free space = average request size */ 144 /* XXX should we scale it up or down? Adjust vacuum.c too, if so */ 145 vacrelstats->threshold = GetAvgFSMRequestSize(&onerel->rd_node); 146 147 /* Open all indexes of the relation */ 148 vac_open_indexes(onerel, &nindexes, &Irel); 149 hasindex = (nindexes > 0); 150 151 /* Do the vacuuming */ 152 lazy_scan_heap(onerel, vacrelstats, Irel, nindexes); 153 154 /* Done with indexes */ 155 vac_close_indexes(nindexes, Irel); 156 157 /* 158 * Optionally truncate the relation. 159 * 160 * Don't even think about it unless we have a shot at releasing a goodly 161 * number of pages. Otherwise, the time taken isn't worth it. 162 */ 163 possibly_freeable = vacrelstats->rel_pages - vacrelstats->nonempty_pages; 164 if (possibly_freeable >= REL_TRUNCATE_MINIMUM || 165 possibly_freeable >= vacrelstats->rel_pages / REL_TRUNCATE_FRACTION) 166 lazy_truncate_heap(onerel, vacrelstats); 167 168 /* Update shared free space map with final free space info */ 169 lazy_update_fsm(onerel, vacrelstats); 170 171 /* Update statistics in pg_class */ 172 vac_update_relstats(RelationGetRelid(onerel), vacrelstats->rel_pages, 173 vacrelstats->rel_tuples, hasindex); --> goes out of scope 174 } ------------------------------------------------------------------------ ------------------------------------------ [BUG] memory leak on error path (dtype != DTK_DELTA) File where bug occurred: postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c Diagnosis: (1) memory is allocated on line 778 by calling pgtypes_alloc; stored in result (2) leaked on error path on line 785 File where bug occurred: /home/kremenek/oses_src/postgresql-7.4.2/src/interfaces/ecpg/ pgtypeslib/interval.c 768 return NULL; 769 } 770 771 if ((ParseDateTime(str, lowstr, field, ftype, MAXDATEFIELDS, &nf, ptr) != 0) 772 || (DecodeInterval(field, ftype, nf, &dtype, tm, &fsec) != 0)) 773 { 774 errno = PGTYPES_INTVL_BAD_INTERVAL; 775 return NULL; 776 } 777 ---> memory is allocated 778 result = (interval *) pgtypes_alloc(sizeof(interval)); 779 if (!result) 780 return NULL; 781 782 if (dtype != DTK_DELTA) 783 { 784 errno = PGTYPES_INTVL_BAD_INTERVAL; 785 return NULL; ---> memory is leaked on error path 786 } 787 788 if (tm2interval(tm, fsec, result) != 0) ------------------------------------------------------------------------ ------------------------------------------ [BUG] difficult to analyze, but it appears that subquery is memory leaked File where bug occurred: postgresql-7.4.2/src/backend/optimizer/plan/subselect.c Diagnosis: (1) copyObject() appears to return newly allocated memory on line 261 (2) subquery is passed to subquery_planner on line 296, but within that function subquery itself never appears to be freed (3) subquery has a field dereference on line 301, but afterwards never appears to be referenced (4) pointer goes out of scope at end of function This one I am less confident about, since it is difficult for me to reason about the inter-procedural semantics. File where bug occurred: /home/kremenek/oses_src/postgresql-7.4.2/src/backend/optimizer/plan/ subselect.c 252 int paramid; 253 List *lst; 254 Node *result; 255 256 /* 257 * Copy the source Query node. This is a quick and dirty kluge to 258 * resolve the fact that the parser can generate trees with multiple 259 * links to the same sub-Query node, but the planner wants to scribble 260 * on the Query. Try to clean this up when we do querytree redesign... 261 */ ---> 262 subquery = (Query *) copyObject(subquery); 263 264 /* 265 * For an EXISTS subplan, tell lower-level planner to expect that only 266 * the first tuple will be retrieved. For ALL and ANY subplans, we 267 * will be able to stop evaluating if the test condition fails, so 268 * very often not all the tuples will be retrieved; for lack of a 269 * better idea, specify 50% retrieval. For EXPR and MULTIEXPR 270 * subplans, use default behavior (we're only expecting one row out, 271 * anyway). 272 * 273 * NOTE: if you change these numbers, also change cost_qual_eval_walker() 274 * in path/costsize.c. 275 * 276 * XXX If an ALL/ANY subplan is uncorrelated, we may decide to hash or 277 * materialize its result below. In that case it would've been better 278 * to specify full retrieval. At present, however, we can only detect 279 * correlation or lack of it after we've made the subplan :-(. Perhaps 280 * detection of correlation should be done as a separate step. 281 * Meanwhile, we don't want to be too optimistic about the percentage 282 * of tuples retrieved, for fear of selecting a plan that's bad for 283 * the materialization case. 284 */ 285 if (slink->subLinkType == EXISTS_SUBLINK) 286 tuple_fraction = 1.0; /* just like a LIMIT 1 */ 287 else if (slink->subLinkType == ALL_SUBLINK || 288 slink->subLinkType == ANY_SUBLINK) 289 tuple_fraction = 0.5; /* 50% */ 290 else 291 tuple_fraction = 0.0; /* default behavior */ 292 293 /* 294 * Generate the plan for the subquery. 295 */ 296 node->plan = plan = subquery_planner(subquery, tuple_fraction); ---> 297 298 node->plan_id = PlannerPlanId++; /* Assign unique ID to this 299 * SubPlan */ 300 301 node->rtable = subquery->rtable; 302 303 /* 304 * Initialize other fields of the SubPlan node. 305 */ 306 node->subLinkType = slink->subLinkType; ------------------------------------------------------------------------ ------------------------------------------ [BUG] memory leak, pformats is allocated but never freed or stored anywhere File where bug occurred: postgresql-7.4.2/src/backend/tcop/postgres.c Diagnosis: (1) pformats is allocated on line 1261 (2) pformats is never stored anywhere, nor is it freed before going out of scope 1251 MemoryContextSwitchTo(MessageContext); 1252 1253 /* Get the fixed part of the message */ 1254 portal_name = pq_getmsgstring(input_message); 1255 stmt_name = pq_getmsgstring(input_message); 1256 1257 /* Get the parameter format codes */ 1258 numPFormats = pq_getmsgint(input_message, 2); 1259 if (numPFormats > 0) 1260 { ---> 1261 pformats = (int16 *) palloc(numPFormats * sizeof(int16)); ---> 1262 for (i = 0; i < numPFormats; i++) 1263 pformats[i] = pq_getmsgint(input_message, 2); 1264 } 1265 1266 /* Get the parameter value count */ 1267 numParams = pq_getmsgint(input_message, 2); 1268 1269 if (numPFormats > 1 && numPFormats != numParams) 1270 ereport(ERROR, 1271 (errcode(ERRCODE_PROTOCOL_VIOLATION),
On 2-May-04, at 2:05 PM, Ted Kremenek wrote: > I'm from the Stanford Metacompilation research group where we use > static analysis to find bugs. Neat. BTW, I saw a talk last summer from Madanlal Musuvathi on some model checking work which I believe is being done by a related group at Stanford; it was very interesting. The problem with applying this kind of static analysis to PostgreSQL is that palloc() is not like malloc(): if the return value goes out of scope before it is freed, it is NOT necessarily the case that a memory leak has occurred. Each palloc() allocation occurs within a "memory context" (a.k.a an arena, if you're familiar with the usage in tcc). Individual allocations can be released via pfree(), or the entire memory context and all memory allocated within it can be released via MemoryContextReset() or a similar function. Some areas of the code bother doing a pfree() for each palloc(); some do not. > [BUG] memory leak of vacrelstats at end of function laxy_vaccum_rel() > File where bug occurred: > postgresql-7.4.2/src/backend/commands/vacuumlazy.c I believe the CommitTransactionCommand() at vacuum.c:894 (which calls CommitTransaction(), which calls AtCommit_Memory(), which performs a MemoryContextDelete()) deallocates this memory reasonably soon after it has been allocated, so this isn't a bug. > [BUG] memory leak on error path (dtype != DTK_DELTA) > File where bug occurred: > postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? > [BUG] difficult to analyze, but it appears that subquery is memory > leaked > File where bug occurred: > postgresql-7.4.2/src/backend/optimizer/plan/subselect.c Not sure about this one -- I didn't bother tracking down exactly where the memory context manipulation happens, but I think it's likely that we release this memory fairly soon after it's allocated. > [BUG] memory leak, pformats is allocated but never freed or stored > anywhere Doesn't look like a bug to me: pformats is allocated in the MessageContext (e.g. tcop/postgres.c:1308), which is reset for every FE command that is processed (e.g. postgres.c:2849). -Neil
Thanks Neil. The information about palloc() is extremely useful, and it will be interesting for us to see how our analysis can better deal with this. Thanks for the quick reply! Best, Ted On May 2, 2004, at 12:23 PM, Neil Conway wrote: > On 2-May-04, at 2:05 PM, Ted Kremenek wrote: >> I'm from the Stanford Metacompilation research group where we use >> static analysis to find bugs. > > Neat. BTW, I saw a talk last summer from Madanlal Musuvathi on some > model checking work which I believe is being done by a related group > at Stanford; it was very interesting. > > The problem with applying this kind of static analysis to PostgreSQL > is that palloc() is not like malloc(): if the return value goes out of > scope before it is freed, it is NOT necessarily the case that a memory > leak has occurred. Each palloc() allocation occurs within a "memory > context" (a.k.a an arena, if you're familiar with the usage in tcc). > Individual allocations can be released via pfree(), or the entire > memory context and all memory allocated within it can be released via > MemoryContextReset() or a similar function. Some areas of the code > bother doing a pfree() for each palloc(); some do not. > >> [BUG] memory leak of vacrelstats at end of function laxy_vaccum_rel() >> File where bug occurred: >> postgresql-7.4.2/src/backend/commands/vacuumlazy.c > > I believe the CommitTransactionCommand() at vacuum.c:894 (which calls > CommitTransaction(), which calls AtCommit_Memory(), which performs a > MemoryContextDelete()) deallocates this memory reasonably soon after > it has been allocated, so this isn't a bug. > >> [BUG] memory leak on error path (dtype != DTK_DELTA) >> File where bug occurred: >> postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c > > Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? > >> [BUG] difficult to analyze, but it appears that subquery is memory >> leaked >> File where bug occurred: >> postgresql-7.4.2/src/backend/optimizer/plan/subselect.c > > Not sure about this one -- I didn't bother tracking down exactly where > the memory context manipulation happens, but I think it's likely that > we release this memory fairly soon after it's allocated. > >> [BUG] memory leak, pformats is allocated but never freed or stored >> anywhere > > Doesn't look like a bug to me: pformats is allocated in the > MessageContext (e.g. tcop/postgres.c:1308), which is reset for every > FE command that is processed (e.g. postgres.c:2849). > > -Neil >
Neil Conway <neilc@samurai.com> writes: > The problem with applying this kind of static analysis to PostgreSQL is > that palloc() is not like malloc(): if the return value goes out of > scope before it is freed, it is NOT necessarily the case that a memory > leak has occurred. I'm a bit surprised that a tool unaware of this fact would generate only four complaints ... I'd have expected hundreds. I concur with Neil's opinion that none of the backend cases represent bugs. However: >> [BUG] memory leak on error path (dtype != DTK_DELTA) >> File where bug occurred: >> postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c > Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? It's entirely likely that ecpg's derivative of the backend's datetime modules contains lots and lots of memory leaks, since AFAIK the palloc infrastructure is not there in the ecpg environment :-(. regards, tom lane
Since the tool is in its nascent stages, I reported only a handful of reports that I myself looked at felt that they were potentially bugs. I appreciate everyone's feedback. Best, Ted On May 2, 2004, at 4:50 PM, Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: >> The problem with applying this kind of static analysis to PostgreSQL >> is >> that palloc() is not like malloc(): if the return value goes out of >> scope before it is freed, it is NOT necessarily the case that a memory >> leak has occurred. > > I'm a bit surprised that a tool unaware of this fact would generate > only > four complaints ... I'd have expected hundreds. > > I concur with Neil's opinion that none of the backend cases represent > bugs. However: > >>> [BUG] memory leak on error path (dtype != DTK_DELTA) >>> File where bug occurred: >>> postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c > >> Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? > > It's entirely likely that ecpg's derivative of the backend's datetime > modules contains lots and lots of memory leaks, since AFAIK the palloc > infrastructure is not there in the ecpg environment :-(. > > regards, tom lane > > ---------------------------(end of > broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html
On Sun, May 02, 2004 at 07:50:46PM -0400, Tom Lane wrote: > >> [BUG] memory leak on error path (dtype != DTK_DELTA) > >> File where bug occurred: > >> postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c > > > Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? > > It's entirely likely that ecpg's derivative of the backend's datetime > modules contains lots and lots of memory leaks, since AFAIK the palloc > infrastructure is not there in the ecpg environment :-(. This is true of course. But I'm hopeful we found most of the possible leaks. This one certainly was not found. Fix just committed. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!
Thank you for the confirmation. Best, Ted -----Original Message----- From: Michael Meskes [mailto:meskes@postgresql.org] Sent: Monday, July 05, 2004 11:55 AM To: Tom Lane Cc: Neil Conway; Ted Kremenek; pgsql-bugs@postgresql.org; Michael Meskes Subject: Re: [BUGS] [CHECKER] 4 memory leaks in Postgresql 7.4.2 On Sun, May 02, 2004 at 07:50:46PM -0400, Tom Lane wrote: > >> [BUG] memory leak on error path (dtype != DTK_DELTA) > >> File where bug occurred: > >> postgresql-7.4.2/src/interfaces/ecpg/pgtypeslib/interval.c > > > Looks suspicious to me, but ECPG is Michael Meskes' domain -- Michael? > > It's entirely likely that ecpg's derivative of the backend's datetime > modules contains lots and lots of memory leaks, since AFAIK the palloc > infrastructure is not there in the ecpg environment :-(. This is true of course. But I'm hopeful we found most of the possible leaks. This one certainly was not found. Fix just committed. Michael -- Michael Meskes Email: Michael at Fam-Meskes dot De ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!