Hi Jelmer, On Sep 19, 2011, at 11:22 PM, Jelmer Vernooij wrote:
>> 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. Oh, I see it now. Right, Py_None will get incref'd by virtue of it being stored in func. It's a little confusing the way it's written, but agreed that it's safe. >> * client.c:256: py_cancel_check should be incref'd? > >It's a C function, I don't see why it could go away. Right (hence the question mark). Probably, technically more correct to hold an explicit reference to it, but agreed that it's likely safe. >> * 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. Cool. Thanks for closing bug 632225. One more Python 2.7 blocker down! But of course, I couldn't stop now, so here are a few other minor things that jumped out at me. (Aside: FWIW, I've always said that the Python C API is the *perfect* use case for goto. You'll see it all over Python's own C code. :). editor.c, py_txdelta_window_handler(): when PyList_New() fails, and opts is NULL, you'll return without releasing the GIL. You should probably also check the return value for the Py_BuildValue() and PyList_SetItem() calls in this function. These are all probably unlikely to fail, so I'm just being paranoid (IME, not a bad trait to have when it comes to Python's C API. :). wc.c, py_status(): when Pool(NULL) returns NULL, you'll leak ret. Technically, this will also happen when dup_status is NULL, but the PyErr_NoMemory will probably kill you anyway. The above also happens in adm_init(). adm_entries_read(): check return from PyDict_SetItemString(). adm_get_prop_diffs(): py_propchanges can leak if the Py_BuildValue() returns NULL, or if py_orig_props ends up NULL. Also, check the PyList_SetItem() return value. repos.c, repos_init(): ret gets leaked if Pool(NULL) returns NULL. fs_root_paths_changed(): Check return value from PyDict_SetItemString(). util.c, PyOS_tmpfile(): I think tmpfile_fn gets leaked, since PyObject_GetAttrString() returns a new reference, which you throw away after the PyObject_CallObject(). prop_hash_to_dict() check return value from PyDict_SetItemString(). Similarly in pyify_changed_paths() and pyify_changed_paths2(). Also in pyify_changed_paths2(), when pyval is NULL, py_changed_paths is leaked. More return value checks missing in py_svn_log_wrapper() and py_dirent(). (Yeah, I know I'm being a PITA :). stream_init(): ret is leaked when Pool(NULL) returns NULL. (Is this basically an svn OOM error?). py_file_rev_handler() and py_ra_file_rev_handler(): I think ret is leaked if you don't enter the if-clause. _ra.c: Similarly, I think it's possible to leak ret from py_open_tmp_file(). E.g You get into the PyFile_Check() else-if-clause, but *fp is not NULL. ret doesn't get decref'd. (OTOH, I don't know the logic of apr_file_from_object() so it might be an impossible situation.) get_commit_editor(): commit_callback gets leaked if you take one of the early exits. ra_get_dir(): Check return code from PyDict_SetItemString(). py_dirents can leak if py_props == NULL. ra_get_locks(): ret can leak if pyval == NULL. Also, check return value from PyDict_SetItemString(). (similarly for ra_get_locations, merge_rangelist_to_list, mergeinfo_to_dict, ra_mergeinfo) get_username_prompt_provider(): auth can leak if Pool(NULL) returns NULL. py_simple_prompt(): ret can leak in the early exit error conditions. Same for: * py_ssl_client_cert_pw_prompt() * py_ssl_client_cert_prompt() * get_ssl_client_cert_pw_prompt_provider() * get_ssl_client_cert_prompt_provider() * get_username_provider() * get_ssl_server_trust_file_provider() * get_ssl_client_cert_file_provider() * get_ssl_client_cert_pw_file_provider() * get_windows_simple_provider() * get_windows_ssl_server_trust_provider() * get_keychain_simple_provider() get_ssl_server_trust_prompt_provider(): auth leaked when Pool(NULL) returns NULL. py_cb_get_simple_provider_prompt(): ret leaks from the else-clause (it's never decref'd even when it's not NULL).
signature.asc
Description: PGP signature
