I did a very quick review of the C code here (branched from debianlp:subvertpy on Launchpad, so should be the sid package), and found a number of questionable things. I don't know the library code or the svn API very well, so it's entirely possible that some or all of these are false alarms. It's also possible I missed some things. I list them here because they made me go "hmm" and I think they're worth a second look.
Most issues I think are not that serious, e.g. leaking a refcount in a rare error condition, either by not decref'ing an object in a short-circuit exit, or not checking return values for Python API calls. I'll list these farther down. The most serious issue appears to be in util.c in config_hash_from_object(). Potentially serious: Notice how when config is a dictionary, you set `dict = config` but when it's not, you PyObject_GetAttrString() the config's __dict__. This sets up a situation where outside this conditional, you are inconsistent with dict's refcount. The if-clause has one lower reference than the else-clause because PyObject_GetAttrString returns a new reference. Thus it's possible that PyDict_Check and PyDict_Next are operating on an object for which the client doesn't hold sufficient references. You're also leaking dict when you've gone through the else-clause. Suggestion: incref dict in the if-clause and add a decref of dict before the return at the end of the function. client.c:318 client_set_notify_funct() where notify_baton2 has inconsistent refcounting between the if-clause and else-clause. When you go through the if-clause, notify_baton2 steals a reference to Py_None, which is going to get decref'd in client_dealloc(). The Py_XDECREF there does not protect you from the extra decref of Py_None! You can see that if you go through the else-clause, func will get incref'd, but if you go through the if-clause, you actually leak func! Suggestion: incref notify_baton2 instead of func. client_proplist, inside the props->nelts loop, the PyList_Append return value is not checked. Not likely to fail, but more seriously, `value` is leaked. Py_BuildValue returns a new reference, but PyList_Append does not steal its reference. Thus you leak every `value`. editor.c:58, new_editor_object: you (probably, haven't checked the call-sites) need to incref commit_callback, since it gets decref'd in py_editor_dealloc. Looks like the refcounts are unmatched. Other problems: * wrap_py_commit_items(): leaks ret when the PyList_SetItem returns on line 118. * client.c:145: PyList_Append return value unchecked, probably harmless though a (void) should be added. * client.c:167: similar for PyDict_SetItemString * client.c:256: py_cancel_check should be incref'd? * client.c, multiple locations: PyArg_Parse* does not incref 'O' arguments, so be careful about calling back into Python with any such parsed object. You need to ensure your library increfs such objects before a potential call back into Python, and then decrefs it when you're done with the Python call. I haven't audited all the places this happens, so this could be a false alarm (i.e. you don't call back into Python). * client.c:925: prop_list leaks in the error condition return * client.c:1037: leaks ret when PyList_SetItem returns non-zero. * client.c:1086: leaks entry_dict in the error return path * client.c:1251: PyList_SetItem return value not checked * client.c:1279: PyDict_SetItemString return value not checked * client.c:1473: leaks `data` when data->pool is NULL. I'm still reviewing the code, so if I find anything else I'll send a follow up. Cheers, -Barry
signature.asc
Description: PGP signature
