Thread: BUG #6672: Memory leaks in dumputils.c
The following bug has been logged on the website: Bug reference: 6672 Logged by: Anna Zaks Email address: zaks.anna@gmail.com PostgreSQL version: 9.1.3 Operating system: MacOSX Description:=20=20=20=20=20=20=20=20 There are two memory leaks in dumputils (v9.2.0beta1): 1) File: src/bin/scripts/dumputils.c Location: line 604, column 11 Description: Memory is never released; potential leak of memory pointed to by 'aclitems' 2) File: src/bin/scripts/dumputils.c Location: line 793, column 10 Description: Memory is never released; potential leak of memory pointed to by 'eqpos' See detailed error paths in the attached html reports. Issues found by clang static analyzer.
zaks.anna@gmail.com writes:
> The following bug has been logged on the website:
> Bug reference: 6672
> Logged by: Anna Zaks
> Email address: zaks.anna@gmail.com
> PostgreSQL version: 9.1.3
> Operating system: MacOSX
> Description:
> There are two memory leaks in dumputils (v9.2.0beta1):
> 1)
> File: src/bin/scripts/dumputils.c
> Location: line 604, column 11
> Description: Memory is never released; potential leak of memory
> pointed to by 'aclitems'
> 2)
> File: src/bin/scripts/dumputils.c
> Location: line 793, column 10
> Description: Memory is never released; potential leak of memory
> pointed to by 'eqpos'
This is a remarkably unhelpful report. I do not see any memory
allocation occurring on either line 604 or line 793 of dumputils.c,
in either 9.2beta1 or 9.1.3. Could you perhaps provide source code
extracts rather than line numbers that reference indeterminate versions
of files?
> See detailed error paths in the attached html reports.
There were no html reports attached, and I'd prefer plain text
anyway please ...
regards, tom lane
On 1 June 2012 06:06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > There were no html reports attached, and I'd prefer plain text > anyway please ... I saw a number of false positives when I ran the Clang static analyser a few months ago. As I recall, I could see why the tool concluded that certain lines of code may have contained errors, even though it was evident to me that they actually did not. That said, it probably wouldn't hurt to give it another try sometime soon. --=20 Peter Geoghegan =A0 =A0 =A0 http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On Thu, May 31, 2012 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> zaks.anna@gmail.com writes:
>> There are two memory leaks in dumputils (v9.2.0beta1):
>
>> 1)
>> File: src/bin/scripts/dumputils.c
>> Location: line 604, column 11
>> Description: Memory is never released; potential leak of memory
>> pointed to by 'aclitems'
>
>> 2)
>> File: src/bin/scripts/dumputils.c
>> Location: line 793, column 10
>> Description: Memory is never released; potential leak of memory
>> pointed to by 'eqpos'
>
> This is a remarkably unhelpful report. I do not see any memory
> allocation occurring on either line 604 or line 793 of dumputils.c,
> in either 9.2beta1 or 9.1.3. Could you perhaps provide source code
> extracts rather than line numbers that reference indeterminate versions
> of files?
I suspect the first complaint is about this bit in git head's
./src/bin/pg_dump/dumputils.c:
if (!parseAclItem(aclitems[i], type, name, subname, remoteVersion,
grantee, grantor, privs, privswgo))
return false;
since 'aclitems' isn't being freed before the return. And the second
complaint seems to concern parseAclItem() not freeing 'buf' when it
returns false. Both of these errors seem academic, since the callers
of buildACLCommands() will bail out with exit_horribly() or
exit_nicely() if it returns false. But IMO it's worth fixing anyway,
to keep the compilers happy or in case of future code calling
buildACLCommands() or parseAclItem().
Attached is a patch to hopefully fix those two errors. I couldn't
quite verify this fixes the OP's error messages, since "checker-266"
isn't done running make after several hours on this OS X machine.
Josh
Attachment
On Fri, Jun 1, 2012 at 11:38 AM, Anna Zaks <zaks.anna@gmail.com> wrote: > Josh, > > What kid of machine are you using? Please, let me know how long it > took after it's done (It takes about one and a half hours on mine). It just finished, actually: took about 7 hours to run, not counting the time the machine was asleep last night. Specs: Macbook Pro, 2.53 GHz i5 with 8GB RAM, OS X 10.6.8, usually modest load on the machine. I downloaded checker-266.tar.bz2 from http://clang-analyzer.llvm.org/ , and then ran: /path/to/checker-266/scan-build ./configure --prefix=/Users/josh/runtime/ --enable-cassert --enable-debug /path/to/checker-266/scan-build make Josh
On Fri, Jun 1, 2012 at 4:23 PM, Anna Zaks <zaks.anna@gmail.com> wrote: > I opened an analyzer Bugzilla report for this issue in case you 'd > like to follow up there: > http://llvm.org/bugs/show_bug.cgi?id=13010 Thanks, I'll try to schedule another run tonight and post additional details on that ticket. On Fri, Jun 1, 2012 at 4:02 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote: > I saw a number of false positives when I ran the Clang static analyser > a few months ago. As I recall, I could see why the tool concluded that > certain lines of code may have contained errors, even though it was > evident to me that they actually did not. That said, it probably > wouldn't hurt to give it another try sometime soon. Yeah, there seems to be a lot of noise in the results, but I found them interesting nonetheless. I put up the HTML report at: http://kupershmidt.org/pg/scan-build-2012-05-31-1/ since it's way too big to mail even a tarball. It's too bad the clang doesn't understand our ereport(ERROR, ...) calls don't return to the caller, as those seem to account for a fair bit of the spurious warnings. I haven't seen anything which I'd call an outright bug, though there are e.g. non-kosher uses of malloc() which could certainly be improved. Hrm, I wonder if proc_exit() and ExitPostmaster() could be declared with __attribute__((noreturn)) , that seems like it would quiet a few errors. Josh
Josh Kupershmidt <schmiddy@gmail.com> writes:
>> zaks.anna@gmail.com writes:
>>> There are two memory leaks in dumputils (v9.2.0beta1):
> ... Both of these errors seem academic, since the callers
> of buildACLCommands() will bail out with exit_horribly() or
> exit_nicely() if it returns false. But IMO it's worth fixing anyway,
> to keep the compilers happy or in case of future code calling
> buildACLCommands() or parseAclItem().
Agreed.
> Attached is a patch to hopefully fix those two errors.
Applied, thanks.
regards, tom lane
Peter, Some of the false positives can be suppressed by teaching the analyzer about the codebase. For example, many of them are due to the custom assertion/error handlers, where you stop execution on an error path by calling, for example, 'elog'. You can drastically reduce the number of false positives by annotating these few functions as 'noreturn'. See "Custom Assertion Handlers" in http://clang-analyzer.llvm.org/annotations.html. If you run into other false positives please feel free to file bugs against the analyser or ask questions on clang-dev mailing list (http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev). Cheers, Anna. On Fri, Jun 1, 2012 at 4:02 AM, Peter Geoghegan <peter@2ndquadrant.com> wro= te: > On 1 June 2012 06:06, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> There were no html reports attached, and I'd prefer plain text >> anyway please ... > > I saw a number of false positives when I ran the Clang static analyser > a few months ago. As I recall, I could see why the tool concluded that > certain lines of code may have contained errors, even though it was > evident to me that they actually did not. That said, it probably > wouldn't hurt to give it another try sometime soon. > > -- > Peter Geoghegan =A0 =A0 =A0 http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training and Services
On Fri, Jun 1, 2012 at 11:13 AM, Josh Kupershmidt <schmiddy@gmail.com> wrot= e: > On Thu, May 31, 2012 at 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> zaks.anna@gmail.com writes: > >>> There are two memory leaks in dumputils (v9.2.0beta1): >> >>> 1) >>> File: =A0 src/bin/scripts/dumputils.c >>> Location: =A0 =A0 =A0 line 604, column 11 >>> Description: =A0 =A0Memory is never released; potential leak of memory >>> pointed to by 'aclitems' >> >>> 2) >>> File: =A0 src/bin/scripts/dumputils.c >>> Location: =A0 =A0 =A0 line 793, column 10 >>> Description: =A0 =A0Memory is never released; potential leak of memory >>> pointed to by 'eqpos' >> >> This is a remarkably unhelpful report. =A0I do not see any memory >> allocation occurring on either line 604 or line 793 of dumputils.c, >> in either 9.2beta1 or 9.1.3. =A0Could you perhaps provide source code >> extracts rather than line numbers that reference indeterminate versions >> of files? > > I suspect the first complaint is about this bit in git head's > ./src/bin/pg_dump/dumputils.c: > > =A0 =A0 =A0 =A0if (!parseAclItem(aclitems[i], type, name, subname, remote= Version, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0grantee, grantor, priv= s, privswgo)) > =A0 =A0 =A0 =A0 =A0 =A0return false; > > since 'aclitems' isn't being freed before the return. And the second > complaint seems to concern parseAclItem() not freeing 'buf' when it > returns false. Both of these errors seem academic, since the callers > of buildACLCommands() will bail out with exit_horribly() or > exit_nicely() if it returns false. But IMO it's worth fixing anyway, > to keep the compilers happy or in case of future code calling > buildACLCommands() or parseAclItem(). > > Attached is a patch to hopefully fix those two errors. I couldn't > quite verify this fixes the OP's error messages, since "checker-266" > isn't done running make after several hours on this OS X machine. > Josh, What kid of machine are you using? Please, let me know how long it took after it's done (It takes about one and a half hours on mine). Thanks, Anna. > Josh
On Fri, Jun 1, 2012 at 11:53 AM, Josh Kupershmidt <schmiddy@gmail.com> wrot= e: > On Fri, Jun 1, 2012 at 11:38 AM, Anna Zaks <zaks.anna@gmail.com> wrote: >> Josh, >> >> What kid of machine are you using? Please, let me know how long it >> took after it's done (It takes about one and a half hours on mine). > > It just finished, actually: took about 7 hours to run, not counting > the time the machine was asleep last night. Specs: Macbook Pro, 2.53 > GHz i5 with 8GB RAM, OS X 10.6.8, usually modest load on the machine. > I downloaded checker-266.tar.bz2 from http://clang-analyzer.llvm.org/ > , and then ran: > > =A0 /path/to/checker-266/scan-build ./configure > --prefix=3D/Users/josh/runtime/ --enable-cassert --enable-debug > =A0 /path/to/checker-266/scan-build make > Any way to know if it was swapping? It should not be this slow.. > Josh
Josh, I opened an analyzer Bugzilla report for this issue in case you 'd like to follow up there: http://llvm.org/bugs/show_bug.cgi?id=3D13010 Thanks, Anna. On Fri, Jun 1, 2012 at 3:19 PM, Anna Zaks <zaks.anna@gmail.com> wrote: > On Fri, Jun 1, 2012 at 11:53 AM, Josh Kupershmidt <schmiddy@gmail.com> wr= ote: >> On Fri, Jun 1, 2012 at 11:38 AM, Anna Zaks <zaks.anna@gmail.com> wrote: >>> Josh, >>> >>> What kid of machine are you using? Please, let me know how long it >>> took after it's done (It takes about one and a half hours on mine). >> >> It just finished, actually: took about 7 hours to run, not counting >> the time the machine was asleep last night. Specs: Macbook Pro, 2.53 >> GHz i5 with 8GB RAM, OS X 10.6.8, usually modest load on the machine. >> I downloaded checker-266.tar.bz2 from http://clang-analyzer.llvm.org/ >> , and then ran: >> >> =A0 /path/to/checker-266/scan-build ./configure >> --prefix=3D/Users/josh/runtime/ --enable-cassert --enable-debug >> =A0 /path/to/checker-266/scan-build make >> > > Any way to know if it was swapping? It should not be this slow.. > >> Josh
On Fri, Jun 1, 2012 at 9:26 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote: > On Fri, Jun 1, 2012 at 4:23 PM, Anna Zaks <zaks.anna@gmail.com> wrote: > >> I opened an analyzer Bugzilla report for this issue in case you 'd >> like to follow up there: >> http://llvm.org/bugs/show_bug.cgi?id=3D13010 > > Thanks, I'll try to schedule another run tonight and post additional > details on that ticket. > > On Fri, Jun 1, 2012 at 4:02 AM, Peter Geoghegan <peter@2ndquadrant.com> w= rote: > >> I saw a number of false positives when I ran the Clang static analyser >> a few months ago. As I recall, I could see why the tool concluded that >> certain lines of code may have contained errors, even though it was >> evident to me that they actually did not. That said, it probably >> wouldn't hurt to give it another try sometime soon. > > Yeah, there seems to be a lot of noise in the results, but I found > them interesting nonetheless. I put up the HTML report at: > =A0http://kupershmidt.org/pg/scan-build-2012-05-31-1/ > > since it's way too big to mail even a tarball. It's too bad the clang > doesn't understand our ereport(ERROR, ...) calls don't return to the > caller, as those seem to account for a fair bit of the spurious > warnings. I haven't seen anything which I'd call an outright bug, > though there are e.g. non-kosher uses of malloc() which could > certainly be improved. > > Hrm, I wonder if proc_exit() and ExitPostmaster() could be declared > with __attribute__((noreturn)) , that seems like it would quiet a few > errors. > Keep in mind that the analyser does not perform analyses across translation units (files), so it will not 'see' implementation of the custom assertion handler unless it's defined in the same file. Cheers, Anna. > Josh
On fre, 2012-06-01 at 21:26 -0700, Josh Kupershmidt wrote: > It's too bad the clang > doesn't understand our ereport(ERROR, ...) calls don't return to the > caller, as those seem to account for a fair bit of the spurious > warnings. I haven't seen anything which I'd call an outright bug, > though there are e.g. non-kosher uses of malloc() which could > certainly be improved. > > Hrm, I wonder if proc_exit() and ExitPostmaster() could be declared > with __attribute__((noreturn)) , that seems like it would quiet a few > errors. I have a few in-progress patches for this that I didn't finish for 9.2. (Some noreturn bits already went into 9.2.) proc_exit() and ExitPostmaster() are low-hanging fruit, but then there is the whole PostgresMain(), PostmasterMain(), GucInfoMain(), WalSenderMain() line of functions (for which I have a patch), then the elog/ereport issue (for which I have a patch, but it could be controversial), and then the Assert() issue (which seems more difficult to solve, because you can turn off assertions at run time, so you can't guarantee anything to the compiler). I will rebase and submit what I have for the next commitfest.