BUG #6672: Memory leaks in dumputils.c

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.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

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 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

On Fri, Jun 1, 2012 at 11:13 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

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,

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.

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 11:53 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

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

Any way to know if it was swapping? It should not be this slow..

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=13010

Thanks,
Anna.

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

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=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.

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 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

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.