Hi Barry,
Thanks again for having a look at these. I don't think any of these are
likely to cause issues in practice, but they're still good to fix.
On Mon 19 Sep 2011 10:16:33 PM CEST, Barry Warsaw wrote:
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.
This is dead code. configs as dictionaries have never worked and aren't
used anywhere. We always just pass in None as the config so the default
config gets used. I've removed this bit of code.
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.
I don't follow - further down in that function we Py_INCREF func, so we
always increment the reference count of func if we assign it to
notify_baton2.
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`.
fixed.
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.
The caller is responsible for increasing the ref counter of commit_callback.
Other problems:
* wrap_py_commit_items(): leaks ret when the PyList_SetItem returns on
line
118.
Fixed.
* client.c:145: PyList_Append return value unchecked, probably
harmless though
a (void) should be added.
Fixed.
* client.c:167: similar for PyDict_SetItemString
Fixed.
* client.c:256: py_cancel_check should be incref'd?
It's a C function, I don't see why it could go away.
* 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).
I'm not aware of any places where we do that in client.c.
* client.c:925: prop_list leaks in the error condition return
* client.c:1037: leaks ret when PyList_SetItem returns non-zero.
fixed.
* client.c:1086: leaks entry_dict in the error return path
* client.c:1251: PyList_SetItem return value not checked
fixed.
* client.c:1279: PyDict_SetItemString return value not checked
fixed.
* client.c:1473: leaks `data` when data->pool is NULL.
Fixed.
Cheers,
Jelmer
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
