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 753817 - subparse: SIGSEGV with abnormal file
subparse: SIGSEGV with abnormal file
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal minor
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-19 12:31 UTC by Eunhae Choi
Modified: 2015-08-28 18:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
subparse: add initializing code of err (1.28 KB, patch)
2015-08-19 12:34 UTC, Eunhae Choi
none Details | Review
subparse: use g_clear_error instead of g_error_free (2.81 KB, patch)
2015-08-19 12:59 UTC, Eunhae Choi
committed Details | Review

Description Eunhae Choi 2015-08-19 12:31:00 UTC
When I checked subparse with the abnormal smi file, SIGSEGV is occurred and I got below log message.


eunhae@eunhae:~/gst/master/gst-plugins-base$ gst-launch-1.0 filesrc location=/home/eunhae/contents/subtitle/test5.smi ! subparse ! fakesink
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
0:00:00.029180129 30209  0x962b750 WARN                subparse gstsubparse.c:452:convert_encoding:<subparse0> could not convert string from 'UTF-8' to UTF-8: Invalid byte sequence in conversion input
0:00:00.029258839 30209  0x962b750 WARN                subparse gstsubparse.c:486:convert_encoding:<subparse0> could not convert string from 'ISO-8859-15' to UTF-8: /usr/lib/i386-linux-gnu/gconv/ISO8859-15.so
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
Caught SIGSEGV
Spinning.  Please run 'gdb gst-launch-1.0 30209' to continue debugging, Ctrl-C to quit, or Ctrl-\ to dump core.
Comment 1 Eunhae Choi 2015-08-19 12:34:14 UTC
Created attachment 309570 [details] [review]
subparse: add initializing code of err

If a error is occurred in convert function with detected_encoding, the 'err' should be set to NULL after free().

Even if the converting succeed at the second time, it is handled as an error because the 'err' is not NULL. And it causes invalid pointer access.
Comment 2 Sebastian Dröge (slomo) 2015-08-19 12:45:03 UTC
Review of attachment 309570 [details] [review]:

Good to go almost, can you update the patch? Thanks :)

::: gst/subparse/gstsubparse.c
@@ +487,3 @@
         encoding, err->message);
     g_error_free (err);
+    err = NULL;

Use g_clear_error (&err) here instead. It calls free and sets the pointer to NULL for safety
Comment 3 Sebastian Dröge (slomo) 2015-08-19 12:45:37 UTC
And with that I mean using g_clear_error() *everywhere* where g_error_free() is currently used in subparse.
Comment 4 Eunhae Choi 2015-08-19 12:59:25 UTC
Created attachment 309579 [details] [review]
subparse: use g_clear_error instead of g_error_free

Thank you for your review. I've added new patch according to the comments.
Comment 5 Sebastian Dröge (slomo) 2015-08-19 13:22:09 UTC
Note that if you want to change it elsewhere to g_clear_error() too, one bug report and one commit per module :)

commit b1f78b5d23db1700eaabfc647806ba1ace9f00b3
Author: Eunhae Choi <eunhae1.choi@samsung.com>
Date:   Wed Aug 19 21:19:05 2015 +0900

    subparse: use g_clear_error instead of g_error_free
    
    To avoid invalid pointer accees the err pointer should be set to NULL.
    By using g_clear_error() it calls free and clear the pointer.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=753817
Comment 6 Tim-Philipp Müller 2015-08-19 13:26:58 UTC
Thanks for the patch.

For future reference, a commit message summary like:

  subparse: fix invalid pointer access

  Use g_clear_error() instead of g_error_free() to
  avoid...

would have been better. This makes it easier to see for people what was fixed. The summary line should contain what was fixed, the message can contain details of how it was done or why it was done like that. Thanks :)