GNOME Bugzilla – Bug 672329
memory leaks in gutils.c and glib tests
Last modified: 2012-08-22 20:06:34 UTC
memory leaks in, glib/gutils.c glib/tests/protocol.c glib/tests/strfuncs.c
Created attachment 210034 [details] [review] fix memory leaks in gutils, protocol and strfunc tests please review.
The *before the patch* code in tests/strfuncs.c don't seem correct to me; it seems to assume g_ascii_strup/down is in-place while that's not the case. The changes to the code in this patch may fix the resulting leak, but they don't make the code correct :-)
Review of attachment 210034 [details] [review]: ::: glib/gutils.c @@ +668,3 @@ + g_free (g_tmp_dir); + g_tmp_dir = g_strdup (g_getenv ("TMP")); + } Since g_free (NULL) is harmless, I'd suggest to avoid all the extra branches, and just do if (x == NULL || *x = 0) { g_free (x); x = ... } ::: glib/tests/protocol.c @@ +217,3 @@ + g_free (msg->nums); + g_strfreev (msg->strings); + g_free (msg); This should probably use g_test_log_msg_free @@ +329,1 @@ } And here too ::: glib/tests/strfuncs.c @@ +1184,3 @@ + g_free (g_ascii_strdown (string, -1)); + g_free (g_ascii_strup (string, -1)); + g_free (g_ascii_strup (string, -1)); As Christian said, we need to determine what this was supposed to test, and then fix it to actually test that, before fixing memory leaks.
Created attachment 210094 [details] [review] glib fix memory leaks in gutils, glib tests protocol and strfuncs. Also fixes bounds_check tc in strfuncs please review.
Review of attachment 210094 [details] [review]: looks good
The following fix has been pushed: aded15c glib: fix memory leaks in gutils, protocol, and strfuncs tests
Created attachment 214232 [details] [review] glib: fix memory leaks in gutils, protocol, and strfuncs tests https://bugzilla.gnome.org/show_bug.cgi?id=672329 Signed-off-by: Ravi Sankar Guntur <ravi.g@samsung.com>
- g_tmp_dir = g_strdup ("/tmp"); + g_free (g_tmp_dir); + g_tmp_dir = g_strdup (g_getenv ("/tmp")); Nope.
Created attachment 222178 [details] [review] Fix regression when TMPDIR/TMP are unset We should just be returning /tmp as a default, not calling g_getenv ("/tmp") which makes no sense.
Review of attachment 222178 [details] [review]: Sure.
Attachment 222178 [details] pushed as 0b6fdff - Fix regression when TMPDIR/TMP are unset