The following function leak when the code goes thru dbus_g_method_return_error, some memory is allocated is g_set_error but not freed in dbus_g_method_return_error. gboolean sniffer_stop(Sniffer *pSniffer, const gchar *pcName, DBusGMethodInvocation *pMethodInvocation) { g_return_val_if_fail(pSniffer, FALSE); g_return_val_if_fail(pcName, FALSE); g_return_val_if_fail(pMethodInvocation, FALSE); g_return_val_if_fail(pSniffer->pCaptureHashTable, FALSE); GError *pError = NULL; Capture *pCapture = sniffer_get_capture(pSniffer, pcName); /* Check name */ if(pCapture == NULL){ g_log(LOG_DOMAIN_CAPTURE_CHILD, G_LOG_LEVEL_DEBUG, "sniffer_stop: %s does not exist", pcName); g_set_error(&pError, CAPTURE_ERROR_DOMAIN, CAPTURE_ERROR_NO_SUCH_CAPTURE_NAME, CAPTURE_ERROR_NO_SUCH_CAPTURE_NAME_STRING, pcName); dbus_g_method_return_error(pMethodInvocation, pError); return FALSE; } /*Other stuff here*/ }
Created attachment 7096 [details] [review] dbus-glib-heefre-20060920.diff Patch for these bugs: https://bugs.freedesktop.org/attachment.cgi?bugid=8350 https://bugs.freedesktop.org/show_bug.cgi?id=8318 Plus: * dbus_g_method_return_error and dbus_g_method_return parameters checks, the rest of the code is usually sloppy: pointers are never check for NULL. * dbus_bus_register called inside dbus_g_connection_open
I'm not convinced by this patch. I'll look deeper into this one.
Which part of the patch are you concerned with ?
well, 1) it doesn't seem to fix the memory leak 2) it changes semantics of api functions 3) it seems to be checking for failure cases that can't happen
1) If it doesn't seem to fix the memory leak, please find another way to fix the memory leaks. 2) Which semantic ? 3) It happened, otherwise, it wouldn't have been modified.
1) the memory leak is actually in your code, you should be freeing the g_error *you* created. 2) dbus_g_connection_open. Your change may be correct, but because you've conflated your patch for this with a patch for a (nonexistant) memleak and with no description of the problem, I'm not going to take it seriously. 3) These failure cases should be seperate bugs, and come with either a description of how and why they happened, or a testcase. The problem may be deep or may be an issue with how you're using the api.
What about all g_return_if_fail which has been added and the check of reply in dbus_g_method_return_error ?
Please document dbus_g_method_return_error so that the caller has to explicitily free the error.
The GError was allocated by g_set_error in your code.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.