Bug 800196

Summary: Bugs found in veusz-1.14-3.fc17 using gcc-with-cpychecker static analyzer
Product: [Fedora] Fedora Reporter: Dave Malcolm <dmalcolm>
Component: veuszAssignee: Jeremy Sanders <jeremy>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: jeremy
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
URL: http://fedorapeople.org/~dmalcolm/gcc-python-plugin/2012-03-05/veusz-1.14-3.fc17/
Whiteboard:
Fixed In Version: veusz-1.15-1.fc17 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-04-04 15:18:18 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 789472    

Description Dave Malcolm 2012-03-05 17:52:35 EST
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
veusz-1.14-3.fc17.src.rpm, and it reports various errors.

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-03-05/veusz-1.14-3.fc17/

I've manually reviewed the issues reported by the tool.

Within the category "Segfaults within error-handling paths" the 3 issues reported appear to be genuine bugs where a call can fail under low memory conditions, returning NULL, but then gets used.

Within the category "Returning (PyObject*)NULL without setting an exception" the 1 issue reported appears to be a genuine bug: PyMem_Malloc does not set the thread's exception state when it fails.  The error-handling near line 1510 needs a call to PyErr_NoMemory().

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):
veusz-1.14-3.fc17
gcc-python-plugin post-0.9 git 11462291a66c8db693c8884cb84b795bb5988ffb running the checker in an *f16* chroot
Comment 1 Jeremy Sanders 2012-03-19 17:57:50 EDT
Thanks for the helpful report - I think I've fixed this upstream with this patch:
https://github.com/jeremysanders/veusz/commit/3e000ca5fd892c15ef4338cda0ec1760bb08ed19

I didn't fix this problem because it looks like a false positive

"Returning (PyObject*)NULL without setting an exception

These messages are often false-positives: the analysis tool has no knowledge about internal API calls that can lead to an exception being set
helpers/src/_nc_cntr.c 	cntr_trace 	returning (PyObject*)NULL without setting an exception"

I can't see any way for the error part of cntr_trace to be called without an exception being set. If any PyMem_Alloc fails this should set the exception.

The fix should be in 1.15 which should be released shortly.
Comment 2 Dave Malcolm 2012-03-21 15:14:00 EDT
(In reply to comment #1)
> Thanks for the helpful report - I think I've fixed this upstream with this
> patch:
> https://github.com/jeremysanders/veusz/commit/3e000ca5fd892c15ef4338cda0ec1760bb08ed19

Excellent; thanks!


> I didn't fix this problem because it looks like a false positive
> 
> "Returning (PyObject*)NULL without setting an exception
> 
> These messages are often false-positives: the analysis tool has no knowledge
> about internal API calls that can lead to an exception being set
> helpers/src/_nc_cntr.c  cntr_trace  returning (PyObject*)NULL without setting
> an exception"
> 
> I can't see any way for the error part of cntr_trace to be called without an
> exception being set. If any PyMem_Alloc fails this should set the exception.

Although these are typically false positives, I think that in this case, it actually *is* a bug.

PyMem_Malloc() *doesn't* set an exception when it fails.
http://docs.python.org/c-api/memory.html#PyMem_Malloc doesn't spell this out, but review of the source code shows it to be the case: it's a thin macro wrapper around malloc(); indeed the example code on that page shows an explicit call to PyErr_NoMemory() for the case when PyMem_Malloc fails.

So I think that error-handling path needs a call to PyErr_NoMemory():
  http://docs.python.org/c-api/exceptions.html#PyErr_NoMemory

Hope this is helpful
Dave
Comment 3 Jeremy Sanders 2012-03-24 05:49:02 EDT
That's very helpful, thanks. I've fixed the PyMem_Malloc bug upstream in another commit.

https://github.com/jeremysanders/veusz/commit/f61d1cb2c42dba5f23c4ba2110d5be4ac01ec974

I've checked the other PyMem_Malloc cases. None were marked by your checker and I think they always set an exception on failure.
Comment 4 Jeremy Sanders 2012-04-04 15:18:18 EDT
Upstream Veusz 1.15 fixes this. I've updated rawhide and f17-candidate to Veusz 1.15.
http://koji.fedoraproject.org/koji/buildinfo?buildID=311679
http://koji.fedoraproject.org/koji/buildinfo?buildID=311678
Thanks.
Comment 5 Fedora Update System 2012-04-04 15:23:08 EDT
veusz-1.15-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/veusz-1.15-1.fc17
Comment 6 Fedora Update System 2012-04-11 23:17:43 EDT
veusz-1.15-1.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.