GNOME Bugzilla – Bug 765794
registrychunks: Memory leak
Last modified: 2018-05-07 15:46:40 UTC
Hi, I found memory leak in below mentioned file and function mentioned below. Inside while loop, if the macro "unpack_string" send control to goto lable "fail" if _len == -1. And from there function is returning without free allocated memory of "arr". Please suggest your opinion. And another issue I think is output of g_new0 is not checked anywhere for NULL in case of memory allocation fails. file: gstreamer/gst/gstregistrychunks.c:731 macro: #define unpack_string(inptr, outptr, endptr, error_label) G_STMT_START{\ gint _len = _strnlen (inptr, (endptr-inptr)); \ if (_len == -1) \ goto error_label; \ outptr = g_memdup ((gconstpointer)inptr, _len + 1); \ inptr += _len + 1; \ }G_STMT_END function: static gchar ** gst_registry_chunks_load_plugin_dep_strv (gchar ** in, gchar * end, guint n) { gchar **arr; if (n == 0) return NULL; arr = g_new0 (gchar *, n + 1); while (n > 0) { unpack_string (*in, arr[n - 1], end, fail); --n; } return arr; fail: GST_INFO ("Reading plugin dependency strings failed"); return NULL; } Thanks
g_new0() can't return NULL, if allocation fails it will abort() the process. For the other case, it should do g_free(arr) before return NULL. Do you want to provide a patch? Did you check if there are more instances of the same problem in the file?
Yes. In fact I am in the middle of the finalizing the patch and checking other instances also. I Will upload the patch by today itself or on Monday.
Thanks, we'll wait for your patch then.
Created attachment 327123 [details] [review] memory leak fix patch I introduced another macro "unpack_string_free" to free allocated memory in case of error and to keep other things untouched.
Review of attachment 327123 [details] [review]: I think it would be better to move the freeing to the error label. Now you can free one random pointer in there, but it's not really extensible and in other places we also move this to the error labels
The reason to introduce that new macro is that the variable "factory" is locally declared inside if block and else block seperately and that is not accessible outside the scope. Please note that lable "fail" is outside if else block. Second thing is that inside if block "factory->uri_protocols" and in else block "factory->extensions" is getting freed. So I can not move this "factory" variable to function level. Please suggest.
You could move the variable declarations into the function scope (and initialize to NULL). Also for factory->uri_protocols, isn't it necessary to also free all protocol strings that were successfully parsed until the error?
Created attachment 327205 [details] [review] fix_memory_leak: Review comments incorporated.
Dear Sebastian Dröge , Please provide your review comments on my last uploaded patch. Thanks.
Comment on attachment 327205 [details] [review] fix_memory_leak: Review comments incorporated. Out of curiosity, is this something you ran into in practice or is this to fix static code analyzer / coverity warnings? I don't really understand the changes made to gst_registry_chunks_load_feature() - here any partial string arrays should be freed as part of the g_object_unref(feature) in the fail label (via the factory's finalize function). >@@ -731,6 +752,7 @@ static gchar ** > gst_registry_chunks_load_plugin_dep_strv (gchar ** in, gchar * end, guint n) > { > gchar **arr; >+ guint n_count = n; > > if (n == 0) > return NULL; >@@ -742,6 +764,9 @@ gst_registry_chunks_load_plugin_dep_strv (gchar ** in, gchar * end, guint n) > } > return arr; > fail: >+ for (; n < n_count; ++n) >+ g_free (arr[n]); >+ g_free (arr); > GST_INFO ("Reading plugin dependency strings failed"); > return NULL; > } I think here it'd be enough to simply do g_strfreev(arr).
Created attachment 327555 [details] [review] [PATCH]: registrychunks: Fix memory leaks in error cases. Make sure to free partially allocated memory during error conditions to avoid leaks. https://bugzilla.gnome.org/show_bug.cgi?id=765794 Hello Tim, Thanks for your input. I have updated the patch according to your comments. Please note that, since typefind_factory->extensions[] is getting filled in reverse way, so we need to free one by one from last position. Thanks
Review of attachment 327555 [details] [review]: Please expand the commit description a little. Something like: "registrychunks: Fix memory leaks in error cases Make sure to free partially allocated memory during error conditions to avoid leaks" ::: gst/gstregistrychunks.c @@ +670,3 @@ for (i = tff->nextensions; i > 0; i--) { unpack_string (*in, str, end, fail); + typefind_factory->extensions[i - 1] = str; All these factory to typefind_factory / element_factory changes are unnecessary and make the diff messy. Since the only explicit cleanup to do is for the typefind factory, check GST_IS_TYPE_FIND_FACTORY(feature) in the fail: case and cast the feature variable there. @@ +733,3 @@ + + if (element_factory->uri_protocols) + g_strfreev (element_factory->uri_protocols); This doesn't seem correct. uri_protocols should not be leaking. The GstElementFactory already calls g_strfreev in gst_element_factory_cleanup() called during finalize(). @@ +739,3 @@ + g_free (typefind_factory->extensions[i]); + g_free (typefind_factory->extensions); + } It makes sense to clean up a partially constructed extensions array in this way, but the typefind_factory will also be calling g_strfreev on that during finalize, so it needs to be set to NULL, and the cleanup needs to happen *before* unreffing the factory a few lines above, because at that point it's hopefully already destroyed. @@ +759,3 @@ return arr; fail: + g_strfreev (arr); This seems correct
Have you actually seen these memory leaks in practice, or are these from some static analysis?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!