Issue 1444030: Several minor fixes for NULL checks, etc. (original) (raw)
Created on 2006-03-06 11:03 by hyeshik.chang, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (6)
Author: Hyeshik Chang (hyeshik.chang) * 
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
Author: Neal Norwitz (nnorwitz) *
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"". 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(""); 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.
Author: Hyeshik Chang (hyeshik.chang) *
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.
Author: Neal Norwitz (nnorwitz) *
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!
Author: Hyeshik Chang (hyeshik.chang) *
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!
Author: Neal Norwitz (nnorwitz) *
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.