[Patches] [ python-Patches-1444030 ] Several minor fixes for NULL checks, etc.

SourceForge.net noreply at sourceforge.net
Tue Mar 7 17:00:07 CET 2006


Patches item #1444030, was opened at 2006-03-06 20:03
Message generated for change (Comment added) made by perky
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1444030&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: Python 2.5
Status: Open
Resolution: None
Priority: 4
Submitted By: Hye-Shik Chang (perky)
Assigned to: Hye-Shik Chang (perky)
Summary: Several minor fixes for NULL checks, etc.

Initial Comment:
Attached patch includes fixes for missing NULL checks,
reference leaks on minor cases found by a static
analysis tool.

It touches:
Python/ceval.c
Python/traceback.c
Python/ast.c
Python/modsupport.c
Objects/object.c
Objects/weakrefobject.c
Objects/unicodeobject.c
Objects/longobject.c
Objects/stringobject.c
Parser/firstsets.c
Modules/arraymodule.c
Modules/zipimport.c
Modules/cStringIO.c


----------------------------------------------------------------------

>Comment By: Hye-Shik Chang (perky)
Date: 2006-03-08 01:00

Message:
Logged In: YES 
user_id=55188

Committed as r42894(trunk), r42895(2.4) except a fix for
object.c.
I'll think about it more tomorrow. :-)
Thanks!

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-03-07 09:09

Message:
Logged In: YES 
user_id=33168

Object/object.c:  You're right, my suggestion would leak v.
 I originally was going to tell you to just call
PyErr_BadInternalCall(), but that is not what
PyObject_String() does.  It's so messy to add a flag. :-(  I
don't see any other way at the moment.  Can you think of any
cleaner solution?

Objects/unicodeobject.c: yuck. I didn't notice the
shadowing.  You're right the outer v does need a DECREF. 
Please rename the inner v to avoid shadowing.

Objects/longobject.c: Oh, I see it now.  I would prefer the
checks for a == NULL (and b == NULL) to be immediately after
they are set from long_invert.  There's no reason to defer
the check/failure is there?

Thanks!

----------------------------------------------------------------------

Comment By: Hye-Shik Chang (perky)
Date: 2006-03-07 08:58

Message:
Logged In: YES 
user_id=55188

Thanks for the review, Neal!

Object/object.c: I think a reference to v will be leaked
then.  Then must we add a flag variable to track v?

Objects/weakrefobject.c: That sounds more attractive!

Objects/unicodeobject.c: The current code have v in two
places; inner scope of the ucnhash routine and the function
scope.  I think function scope v needs to defref'ed. (and,
inner v needs to be renamed?)

Objects/longobject.c: If a->ob_size < 0, long_invert() is
assigned to a while the function can return NULL.  So I
thought some kind of NULL checking is needed.

Objects/stringobject.c: A macro SPLIT_APPEND uses onError
label actually.

I'll update the patch with your helpful comments soon.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-03-07 08:20

Message:
Logged In: YES 
user_id=33168

Thanks Perky!

I haven't gotten access to the report yet.  All these look
fine, with the following exceptions:

Python/modsupport.c:  This looks like it's a false positive,
presumably because of if ((v = PyList_New(n)) == NULL).  I
changed that code and cleaned up the conditions below.  I
think the original code was more complex than it needs to
be.  I checked in this change to modsupport.c.

Objects/object.c:  To be consistent with PyObject_Str() this
should return u"<NULL>".  The current code is wrong, but the
patch just returns NULL.  I *think* the correct thing to do
is set v instead of res, then if that result is NULL, return
NULL, ie:

  v = PyString_FromString("<NULL>");
  if (v == NULL)
    return NULL;

Objects/weakrefobject.c:  Your addition is correct, but I
wonder if you also need to restore the error before
returning like the code below?

        if (restore_error)
            PyErr_Restore(err_type, err_value, err_tb);

Objects/unicodeobject.c: [block 1945]  the current code is
correct, but perhaps a bit confusing.  goto ucnhashError is
called in 3 places I see.  The first 2 places, v is not set
yet.  In the third place, v was already DECREF'd.  Perhaps
after the DECREF, we should do: v = NULL; ?

All other unicode changes are correct.

Objects/longobject.c:  I disagree with these changes, but
the code is broken.  I don't believe we should be checking
for a or b being NULL.  I think the code should be this (the
first line is just for context):

        z = _PyLong_New(size_z);
        if (z == NULL)
                return NULL;

The callers take care of DECREF'ing a and b and we didn't
allocate either of them in this function.

Objects/stringobject.c:  Since list is always NULL if going
to onError, the DECREF is not needed.  I would get rid of
the whole onError label and just return NULL if the list
alloc fails.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1444030&group_id=5470


More information about the Patches mailing list