Bug 795011

Summary:	Possible segfaults under low memory conditions found in postgresql-9.1.2-2.fc17 using gcc-with-cpychecker static analyzer
Product:	[Fedora] Fedora	Reporter:	Dave Malcolm <dmalcolm>
Component:	postgresql	Assignee:	Tom Lane <tgl>
Status:	CLOSED UPSTREAM	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	rawhide	CC:	devrim, hhorak, tgl
Target Milestone:	---
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
URL:	http://fedorapeople.org/~dmalcolm/gcc-python-plugin/2012-02-18/postgresql-9.1.2-2.fc17/
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2012-03-13 19:33:24 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:
Bug Blocks:	789472

Description Dave Malcolm 2012-02-18 16:20:34 UTC

Description of problem:
I've been writing an experimental static analysis tool to detect bugs commonly occurring within C Python extension modules:
  https://fedorahosted.org/gcc-python-plugin/
  http://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html
  http://fedoraproject.org/wiki/Features/StaticAnalysisOfPythonRefcounts

I ran the latest version of the tool (in git master; post 0.9) on
postgresql-9.1.2-2.fc17.src.rpm, and it reports various errors.

I've manually reviewed the issues reported by the tool, and most issues are false positives, but there are some possible segfaults when memory allocations fail, I think.

You can see a list of errors here, triaged into categories (from most significant to least significant):
http://fedorapeople.org/~dmalcolm/gcc-python-plugin/2012-02-18/postgresql-9.1.2-2.fc17/

Within the category "Unclassified errors" the 1 issue reported is a bug in my tool, which I've filed as https://fedorahosted.org/gcc-python-plugin/ticket/33

Within the category "Segfaults in normal paths" the 1 issue reported is a false-positive (assuming that lineno>=0): the checker has no way of knowing that lineno>=0, and detects that s is still NULL for the case where lineno < 0.

Within the category "Reference count too low" the 1 issue reported may or may not be a true error; I can't tell without seeing the wider context.

Within the category "Reference count too low within an initialization routine" the 1 issue reported is nitpicking, and unlikely to be an issue.

Within the category "Reference leak within initialization" the 4 issues reported are inconsequential.

Within the category "Segfaults within error-handling paths" the 7 segfaults reported within PLy_init_interp, PLy_generate_spi_exceptions, and PLy_add_exceptions can only happen with very low memory at initialization time.
However, the remaining 4 issues do look like they could cause a segfaul under very low memory conditions under ongoing operations:
plpython.c:PLyList_FromArray:dereferencing NULL (MEM[(struct PyListObject *)list].ob_item) at plpython.c:2362
plpython.c:PLyList_FromArray:dereferencing NULL (MEM[(struct PyListObject *)list].ob_item) at plpython.c:2365
plpython.c:PLyUnicode_AsString:calling PyString_AsString with NULL as argument 1 (o) at plpython.c:4875
plpython.c:PLyDict_FromTuple:calling PyDict_SetItemString with NULL as argument 3 (value) at plpython.c:2404

Within the category "Possible reference leaks" the 1 issue reported may or may not be a memory leak: does PLy_spi_execute_fetch_result return a new reference or just a borrowed reference?  (it's a memory leak if it's the former).

Within the category "Returning (PyObject*)NULL without setting an exception" the 4 issues reported are false positives (the checker doesn't know about the behavior of PLy_exception_set).

There may of course be other bugs in my checker tool.

Hope this is helpful; let me know if you need help reading the logs that the tool generates - I know that it could use some improvement.

Version-Release number of selected component (if applicable):
postgresql-9.1.2-2.fc17
gcc-python-plugin post-0.9 git 771455b3128b1323e80bdda53939d8d140a84c68 running the checker in an *f16* chroot

Comment 1 Tom Lane 2012-02-18 19:26:14 UTC

(In reply to comment #0)

Thanks Dave, this is interesting stuff.  I don't hack plpython much myself, but I'll point some other upstream people to this report.

> Within the category "Segfaults in normal paths" the 1 issue reported is a
> false-positive (assuming that lineno>=0): the checker has no way of knowing
> that lineno>=0, and detects that s is still NULL for the case where lineno < 0.

Actually the case where it would fail is lineno == 0.  Since the code is troubling to check for bad lineno at all, it seems like it ought to return NULL for that case as well; so I'd call this a real bug, even though it might not currently be exercisable depending on what the calling logic is.

Have you considered running this tool over code unrelated to Python?  This particular bit of analysis doesn't seem at all Python-specific.

Comment 2 Dave Malcolm 2012-02-18 20:58:16 UTC

(In reply to comment #1)
> (In reply to comment #0)
> 
> Thanks Dave, this is interesting stuff.  I don't hack plpython much myself, but
> I'll point some other upstream people to this report.
Thanks.

> > Within the category "Segfaults in normal paths" the 1 issue reported is a
> > false-positive (assuming that lineno>=0): the checker has no way of knowing
> > that lineno>=0, and detects that s is still NULL for the case where lineno < 0.
> 
> Actually the case where it would fail is lineno == 0.  Since the code is

You're right; I misread the report.

> Have you considered running this tool over code unrelated to Python?  This
> particular bit of analysis doesn't seem at all Python-specific.
Yes, I'd like to do this eventually, but I'm restricting the scope for now to just Python C extensions, to keep me sane.

Comment 3 Tom Lane 2012-03-13 19:33:24 UTC

Upstream fixes for these issues have been committed at
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0cb4a0bfb80c7d75c745faf3597bed5c3f9a3c49

None of them seem critical enough to justify making a Fedora patch; I'll just wait to incorporate the next upstream minor release, which will likely be in a couple of months.

BTW, the people who looked at this commented that quite a few of the false positives were because the tool doesn't know that PLy_elog(ERROR, ...) does not return.  Not sure that it's worth trying to teach the tool about that, because it's a mighty project-specific thing, and not very cleanly defined (whether it returns or not depends on the error-severity argument :-().  But just thought I'd mention it.