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 755480 - TRY_GET leaks string pointer
TRY_GET leaks string pointer
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-editing-services
git master
Other Linux
: Normal normal
: 1.6.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-23 16:36 UTC by Justin Kim
Modified: 2015-09-28 14:16 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
TRY_GET leaks string pointer (1.90 KB, patch)
2015-09-23 16:37 UTC, Justin Kim 		none Details | Review
fix warning (2.39 KB, patch)
2015-09-24 02:00 UTC, Justin Kim 		none Details | Review
TRY_GET_STRING (2.84 KB, patch)
2015-09-25 09:17 UTC, Justin Kim 		committed Details | Review

Description Justin Kim 2015-09-23 16:36:52 UTC 
TRY_GET is very useful macro, but it fails to manage string pointer because it uses gst_structure_get.

https://developer.gnome.org/gstreamer/stable/gstreamer-GstStructure.html#gst-structure-get

The return value of gst_structure_get_string, otherwise, doesn't need to be freed.
Comment 1 Justin Kim 2015-09-23 16:37:41 UTC 
Created attachment 311959 [details] [review]
TRY_GET leaks string pointer
Comment 2 Thibault Saunier 2015-09-23 17:04:37 UTC 
Review of attachment 311959 [details] [review]:

Getting:

ges-structured-interface.c: In function ‘_ges_add_clip_from_struct’:
ges-structured-interface.c:352:132: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
ges-structured-interface.c:352:21: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

now :)
Comment 3 Justin Kim 2015-09-24 02:00:34 UTC 
Created attachment 311993 [details] [review]
fix warning

you are sharp in review :)
Comment 4 Thibault Saunier 2015-09-24 07:46:22 UTC 
Review of attachment 311993 [details] [review]:

::: ges/ges-structured-interface.c
@@ -349,3 +350,3 @@
   if (layer_priority == -1)
     TRY_GET ("layer", G_TYPE_INT, &layer_priority, -1);
-  TRY_GET ("type", G_TYPE_STRING, &type_string, "GESUriClip");
+  TRY_GET ("type", G_TYPE_STRING, &type_string, g_strdup ("GESUriClip"));

Looks wrong to g_strdup the def value, I think it is leaked if we do not use it.
Comment 5 Justin Kim 2015-09-24 08:41:53 UTC 
Review of attachment 311993 [details] [review]:

::: ges/ges-structured-interface.c
@@ +440,2 @@
 beach:
+  g_free (type_string);

@Thibault, in here, it is freed. User should do free after calling TRY_GET for string.

OR, we might define TRY_GET_STRING for the case of string.

Any advice?
Comment 6 Justin Kim 2015-09-24 08:49:08 UTC 
Oops, you right.
g_strdup ("GESUriClip") will be leaked if "type" has value.

I think it would be better to define TRY_GET_STRING. What is your opinion?
Comment 7 Thibault Saunier 2015-09-24 09:49:34 UTC 
(In reply to Justin J. Kim from comment #6)
> Oops, you right.
> g_strdup ("GESUriClip") will be leaked if "type" has value.
> 
> I think it would be better to define TRY_GET_STRING. What is your opinion?

++ :)
Comment 8 Justin Kim 2015-09-25 09:17:38 UTC 
Created attachment 312119 [details] [review]
TRY_GET_STRING