GNOME Bugzilla – Bug 753817
subparse: SIGSEGV with abnormal file
Last modified: 2015-08-28 18:27:35 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.
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.
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
And with that I mean using g_clear_error() *everywhere* where g_error_free() is currently used in subparse.
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.
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
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 :)