Thread: Memory leak fixes for pg_dump, pg_dumpall, initdb and pg_upgrade
Hi all, Please find attached a set of fixes for a couple of things in src/bin: - pg_dump/pg_dumpall: -- getFormattedTypeName, convertTSFunction and myFormatType return strdup'd results that are never free'd. -- convertTSFunction returns const char. I fail to see the point of that... In my opinion we are fine with just returning a char pointer, which is strdup'd so as it can be freed by the caller. - initdb's and pg_regress' use getaddrinfo, but do not free the returned result with freeaddrinfo(). - Coverity noticed on the way some leaked memory in pg_upgrade's equivalent_locale(). Those issues have been mostly spotted by Coverity, I may have spotted some of them while looking at similar code paths... In any case that's Coverity's win ;) Regards, -- Michael
Attachment
On Mon, Jun 8, 2015 at 3:48 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Hi all, > > Please find attached a set of fixes for a couple of things in src/bin: > - pg_dump/pg_dumpall: > -- getFormattedTypeName, convertTSFunction and myFormatType return > strdup'd results that are never free'd. > -- convertTSFunction returns const char. I fail to see the point of > that... In my opinion we are fine with just returning a char pointer, > which is strdup'd so as it can be freed by the caller. > - initdb's and pg_regress' use getaddrinfo, but do not free the > returned result with freeaddrinfo(). > - Coverity noticed on the way some leaked memory in pg_upgrade's > equivalent_locale(). > > Those issues have been mostly spotted by Coverity, I may have spotted > some of them while looking at similar code paths... In any case that's > Coverity's win ;) Attached are new patches, I simplified the use of free in the fixes of pg_dumpall. -- Michael
Attachment
On Mon, Jun 8, 2015 at 10:26 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Jun 8, 2015 at 3:48 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Hi all, >> >> Please find attached a set of fixes for a couple of things in src/bin: >> - pg_dump/pg_dumpall: >> -- getFormattedTypeName, convertTSFunction and myFormatType return >> strdup'd results that are never free'd. >> -- convertTSFunction returns const char. I fail to see the point of >> that... In my opinion we are fine with just returning a char pointer, >> which is strdup'd so as it can be freed by the caller. >> - initdb's and pg_regress' use getaddrinfo, but do not free the >> returned result with freeaddrinfo(). >> - Coverity noticed on the way some leaked memory in pg_upgrade's >> equivalent_locale(). >> >> Those issues have been mostly spotted by Coverity, I may have spotted >> some of them while looking at similar code paths... In any case that's >> Coverity's win ;) > > Attached are new patches, I simplified the use of free in the fixes of > pg_dumpall. Please ignore those versions, I am just too sleepy... -- Michael
On 06/08/2015 09:48 AM, Michael Paquier wrote: > Hi all, > > Please find attached a set of fixes for a couple of things in src/bin: > - pg_dump/pg_dumpall: > -- getFormattedTypeName, convertTSFunction and myFormatType return > strdup'd results that are never free'd. > -- convertTSFunction returns const char. I fail to see the point of > that... In my opinion we are fine with just returning a char pointer, > which is strdup'd so as it can be freed by the caller. > - initdb's and pg_regress' use getaddrinfo, but do not free the > returned result with freeaddrinfo(). > - Coverity noticed on the way some leaked memory in pg_upgrade's > equivalent_locale(). > > Those issues have been mostly spotted by Coverity, I may have spotted > some of them while looking at similar code paths... In any case that's > Coverity's win ;) "Fixing" most of these is not really an improvement, IMO. They're in pg_dump and pg_ugprade, which you only run once and then it exits, so as long as the leaks are not in some tight loops that execute millions of time, it doesn't matter. I committed some of these that seemed like improvements on readability grounds, but please just mark the rest as "ignore" in coverity. - Heikki
On Fri, Jul 3, 2015 at 3:14 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I committed some of these that seemed like improvements on readability > grounds, but please just mark the rest as "ignore" in coverity. Done. Thanks. -- Michael