Bug 680269

Summary: RETURN_STRING in python-ethtool-0.6 doesn't correctly handle the reference count of the Py_None singleton
Product: Red Hat Enterprise Linux 6 Reporter: Dave Malcolm <dmalcolm>
Component: python-ethtoolAssignee: Dave Malcolm <dmalcolm>
Status: CLOSED ERRATA QA Contact: Petr Šplíchal <psplicha>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 6.1CC: davids, ohudlick, psplicha
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: python-ethtool-0.6-1.el6 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-05-19 14:25:16 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On:    
Bug Blocks: 605533    
Attachments:
Description Flags
RETURN_STRING() did not use Py_INCREF() when returning Py_None
none
Improved error situations in case of NULL returns none

Description Dave Malcolm 2011-02-24 20:53:49 UTC
Description of problem:
Looking at e.g.:
http://fedorapeople.org/gitweb?p=dsommers/public_git/python-ethtool.git;a=blob;f=python- ethtool/etherinfo_obj.c;h=04eb634a4e0fababf3369be649fcc039d2f791bb;hb=HEAD

I see e.g. _ethtool_etherinfo_getter using the RETURN_STRING macro.

This is declared in:
http://fedorapeople.org/gitweb?p=dsommers/public_git/python-ethtool.git;a=blob;f=python-ethtool/etherinfo_struct.h;h=b1d453ffae8b282386e60cb60ed9d31360b1701c;hb=HEAD

  #define RETURN_STRING(str) (str ? PyString_FromString(str) : Py_None)

This isn't incrementing the reference count on the Py_None singleton when it should be (the caller assumes that it "owns" a ref on the result of _getter, and will decref it), it could cause the python process to bail out:
  "Fatal Python error: deallocating None"
if run repeatedly.

However, this doesn't seem to present in the 0.3 currently in RHEL6.

(There also seem to be a few places in python-ethtool/etherinfo_obj.c:_ethtool_etherinfo_get_ipv6_addresses where the code assumes that certain Py_ API calls don't return NULL)

Filing since bug 605533 proposes a rebase to 0.6

Comment 2 Dave Malcolm 2011-02-24 21:55:36 UTC
>>> for i in range(10000): ethtool.get_interfaces_info('lo')[0].ipv4_broadcast
... 
Fatal Python error: deallocating None
Aborted (core dumped)

This was with a local rebuild of python-ethtool-0.6-0.el6.x86_64

Comment 3 Dave Malcolm 2011-02-25 00:38:36 UTC
Note that Python/object.h has:
   /* Macro for returning Py_None from a function */
   #define Py_RETURN_NONE return Py_INCREF(Py_None), Py_None

so either that could be used, or adapted to something like:
  #define RETURN_STRING(str) \
      (str ? \
       PyString_FromString(str) : \
       (Py_INCREF(Py_None), Py_None) \
      )

though I haven't tested that code.

Comment 4 David Sommerseth 2011-02-25 12:04:39 UTC
Created attachment 480977 [details]
RETURN_STRING() did not use Py_INCREF() when returning Py_None

From aa2c20e697af1907b92129410aa10952a3ffdd68 Mon Sep 17 00:00:00 2001
From: David Sommerseth <davids>
Date: Fri, 25 Feb 2011 09:57:04 +0100
Subject: [PATCH 1/2] RETURN_STRING() did not use Py_INCREF() when returning Py_None

From RH BZ#680269:

     #define RETURN_STRING(str) (str ? PyString_FromString(str) : Py_None)

  This isn't incrementing the reference count on the Py_None singleton when it
  should be (the caller assumes that it "owns" a ref on the result of _getter,
  and will decref it), it could cause the python process to bail out:
    "Fatal Python error: deallocating None"
  if run repeatedly.

Reported-by: Dave Malcolm <dmalcolm>
Signed-off-by: David Sommerseth <davids>
---
 python-ethtool/etherinfo_struct.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comment 5 David Sommerseth 2011-02-25 12:05:24 UTC
Created attachment 480978 [details]
Improved error situations in case of NULL returns

From 4e928d62a8e3c1dfefe6a55b81c0f0b4510b14eb Mon Sep 17 00:00:00 2001
From: David Sommerseth <davids>
Subject: [PATCH 2/2] Improved error situations in case of NULL returns

_ethtool_etherinfo_get_ipv6_addresses() didn't check too well several
Python calls if it would return NULL.

Reported-by: Dave Malcolm <dmalcolm>
Signed-off-by: David Sommerseth <davids>
---
 python-ethtool/etherinfo_obj.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

Comment 9 errata-xmlrpc 2011-05-19 14:25:16 UTC
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0770.html