GNOME Bugzilla – Bug 748604
local-metadata: Don't crash on files named wsb.wmv
Last modified: 2015-06-11 11:13:56 UTC
If you have a file named wsb.wmv, grilo just crashes, taking totem down with it.
Created attachment 302535 [details] [review] local-metadata: Ignore sanitization if the whole filename is blacklisted If the whole filename ends up blacklisted, then just take it as-is.
Comment on attachment 302535 [details] [review] local-metadata: Ignore sanitization if the whole filename is blacklisted Looks good, thanks!
Created attachment 302554 [details] [review] tests: Add a test for video_sanitise_string() crasher
Attachment 302535 [details] pushed as 85fc7cb - local-metadata: Ignore sanitization if the whole filename is blacklisted Attachment 302554 [details] pushed as 34cf295 - tests: Add a test for video_sanitise_string() crasher
Alas, this is not yet fixed. See https://bugs.archlinux.org/task/44724 (0.2.14-3 is 0.2.14 with the patch from attachment 302535 [details] [review]). Seems that line_end can become null after subsequent g_utf8_find_prev_char? I also noticed that g_unichar_isalnum is used on a utf-8 gchar instead of a gunichar. Seems terribly wrong to me. I'm currently patching the code.
Created attachment 303618 [details] [review] tests: Add another test for a video_sanitise_string() Adds a failing test using the crashing file name from the Arch bug.
Created attachment 303619 [details] [review] local-metadata: Fixes to video_sanitise_string non-alnum handling - Extract loop condition into a helper function - Use g_utf8_get_char to properly convert to unichar - Be more defensive about g_utf8_find_prev_char returning NULL
I would like to include these patches in the Debian packages, but I would like to have the last one by Jan Alexander Steffens reviewed first, otherwise grilo-plugins is still crashing. Bastian, is that patch fine? Thanks!
Review of attachment 303619 [details] [review]: ::: src/local-metadata/grl-local-metadata.c @@ +248,3 @@ + gunichar uchar; + + if (str == NULL) { No need for braces for one-line conditionals. @@ +254,3 @@ + uchar = g_utf8_get_char (str); + + if (g_unichar_isalnum (uchar)) { Ditto. @@ +258,3 @@ + } + + if (uchar == '!' || uchar == '?' || uchar == '.') { Ditto.
Review of attachment 303618 [details] [review]: This looks fine, but I can't make that crash... I'm fine with fixing "breaks with non-ASCII UTF-8" bugs, but a test case should fail before the patch is applied, otherwise the point is moot.
(In reply to Bastien Nocera from comment #10) > Review of attachment 303618 [details] [review] [review]: > > This looks fine, but I can't make that crash... Thanks for the quick review! I can make it crash if I try to open a file named [XVID-ITA].avi from totem.
(In reply to Alberto Garcia from comment #11) > (In reply to Bastien Nocera from comment #10) > > Review of attachment 303618 [details] [review] [review] [review]: > > > > This looks fine, but I can't make that crash... > > Thanks for the quick review! > > I can make it crash if I try to open a file named [XVID-ITA].avi from > totem. With current grilo-plugins master? (which means one with Olivier's patch, but not Jan Alexander's). Are you using 32-bit or 64-bit system? What's the backtrace of that crash?
(In reply to Bastien Nocera from comment #12) > > I can make it crash if I try to open a file named [XVID-ITA].avi > > from totem. > With current grilo-plugins master? (which means one with Olivier's > patch, but not Jan Alexander's). No, this is 0.2.14 + Olivier's patch, on a 64-bit system. It looks like no one's checking if this is non-NULL: line_end = g_utf8_find_prev_char (line, line_end); Program received signal SIGSEGV, Segmentation fault. 0x00007fffc48c04f8 in video_sanitise_string (str=0x5555580ad9e0 "[XVID-ITA]") at grl-local-metadata.c:284 1: line = (gchar *) 0x5555580ad9e0 "[XVID-ITA]" 2: line_end = (gchar *) 0x0 (gdb) bt
+ Trace 235145
Perhaps prev_char had a bug that got resolved on master?
Created attachment 305047 [details] [review] Remove uppercase versions of blacklisted words I haven't seen any relevant changes in master since 0.2.14. I checked a bit the code of this function, though: - The input string is "[XVID-ITA]". - After checking the list of blacklisted words, line_end is "XVID-ITA]" ("xvid" is blacklisted). - Then there's line_end = g_utf8_find_prev_char(), so line_end becomes "[XVID-ITA]" After that the loop search backwards for the first alphanumeric char. Since we're at the beginning of the string already there's nothing to search, line_end becomes null and the process crashes. Jan's patch solves the crash because it checkes for null in every iteration of the loop. On a related note, the list of blacklisted words is matched using strcasestr(), so I don't see the point of having the same strings in both uppercase and lowercase.
All pushed to master. Just to note that the lua source that implements that functionality carries on working ;)