This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Several minor fixes for NULL checks, etc.
Type: Stage:
Components: None Versions: Python 2.5
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: hyeshik.chang Nosy List: hyeshik.chang, nnorwitz
Priority: low Keywords: patch

Created on 2006-03-06 11:03 by hyeshik.chang, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
patch1.diff hyeshik.chang, 2006-03-06 11:03 first patch
Messages (6)
msg49665 - (view) Author: Hyeshik Chang (hyeshik.chang) * (Python committer) Date: 2006-03-06 11:03
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
msg49666 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-03-06 23:20
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.
msg49667 - (view) Author: Hyeshik Chang (hyeshik.chang) * (Python committer) Date: 2006-03-06 23:58
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.
msg49668 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-03-07 00:09
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!
msg49669 - (view) Author: Hyeshik Chang (hyeshik.chang) * (Python committer) Date: 2006-03-07 16:00
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!
msg49670 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-03-14 06:11
Logged In: YES 
user_id=33168

Ok, I fixed the object.c one, so everything's done here AFAIK.
History
Date User Action Args
2022-04-11 14:56:15adminsetgithub: 42986
2006-03-06 11:03:30hyeshik.changcreate