After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 672329 - memory leaks in gutils.c and glib tests
memory leaks in gutils.c and glib tests
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.31.x
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-03-18 07:52 UTC by Ravi Sankar Guntur
Modified: 2012-08-22 20:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix memory leaks in gutils, protocol and strfunc tests (4.53 KB, patch)
2012-03-18 07:57 UTC, Ravi Sankar Guntur
needs-work Details | Review
glib fix memory leaks in gutils, glib tests protocol and strfuncs. Also fixes bounds_check tc in strfuncs (5.31 KB, patch)
2012-03-19 15:54 UTC, Ravi Sankar Guntur
accepted-commit_now Details | Review
glib: fix memory leaks in gutils, protocol, and strfuncs tests (5.31 KB, patch)
2012-05-17 04:38 UTC, Matthias Clasen
committed Details | Review
Fix regression when TMPDIR/TMP are unset (809 bytes, patch)
2012-08-22 18:50 UTC, Colin Walters
committed Details | Review

Description Ravi Sankar Guntur 2012-03-18 07:52:08 UTC
memory leaks in,

glib/gutils.c
glib/tests/protocol.c
glib/tests/strfuncs.c
Comment 1 Ravi Sankar Guntur 2012-03-18 07:57:27 UTC
Created attachment 210034 [details] [review]
fix memory leaks in gutils, protocol and strfunc tests

please review.
Comment 2 Christian Persch 2012-03-18 11:58:33 UTC
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 :-)
Comment 3 Matthias Clasen 2012-03-18 14:13:27 UTC
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.
Comment 4 Ravi Sankar Guntur 2012-03-19 15:54:17 UTC
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.
Comment 5 Matthias Clasen 2012-05-13 05:11:11 UTC
Review of attachment 210094 [details] [review]:

looks good
Comment 6 Matthias Clasen 2012-05-17 04:38:10 UTC
The following fix has been pushed:
aded15c glib: fix memory leaks in gutils, protocol, and strfuncs tests
Comment 7 Matthias Clasen 2012-05-17 04:38:13 UTC
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>
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-05-21 02:52:31 UTC
- g_tmp_dir = g_strdup ("/tmp");
+ g_free (g_tmp_dir);
+ g_tmp_dir = g_strdup (g_getenv ("/tmp"));

Nope.
Comment 9 Colin Walters 2012-08-22 18:50:34 UTC
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.
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-08-22 18:53:42 UTC
Review of attachment 222178 [details] [review]:

Sure.
Comment 11 Colin Walters 2012-08-22 20:06:31 UTC
Attachment 222178 [details] pushed as 0b6fdff - Fix regression when TMPDIR/TMP are unset