Issue1444030
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.
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) * | 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) * | 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) * | 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) * | 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) * | 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) * | 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:15 | admin | set | github: 42986 |
2006-03-06 11:03:30 | hyeshik.chang | create |