GNOME Bugzilla – Bug 671259
gvfsd-cdda crashed with signal 5 in _g_dbus_oom()
Last modified: 2012-08-07 14:50:06 UTC
This causes that sound-juicer can only rip one CD and then emits a message that says: 'Could not read the CD Sound Juicer could not read the track listing on this CD. Reason: Cannot access CD: Error while getting peer-to-peer dbus connection: The name :1.129 was not provided by any .service files' But gvfs-cdda dies before this. See: https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/819304 https://bugs.launchpad.net/ubuntu/+source/sound-juicer/+bug/622213 and especially https://bugzilla.gnome.org/show_bug.cgi?id=663459 where there is the argument why this is thought to be an issue with gvfs-cdda
This bug also occurs when an Audio-CD is inserted and rhythmbox is started: Rhythmbox recognizes that an Audio-CD is inserted, shows it, then the CD is automatically unmounted, disappears from Rhythmbox and a message appears that gvfsd-cdda crashed. This bug is critical since it prevents several programs from reading Audio-CDs!
This completly disables Rhythmbox from reading ANY Audio-CD! Please help!
Workaround for this: Use Banshee (instead of Rhythmbox) or RipOff (instead of Sound Juicer). They both work since they are using different software to read Audio-CDs.
Please tell me what information you need to fix this! I noticed something else: When I use RipOff (http://sourceforge.net/projects/ripoffc/) to rip an Audio-CD and open Rhythmbox while the CD is ripped, Rhythmbox correctly recognizes the CD and gvfsd doesn't crash!
Hm, well my last comment was wrong. I used a different Audio-CD and that one actually works. So I got this bug on two Audio-CDs so far. All CDs I tested are CDs I bought at a store, so nothing I burned myself. The ones that caused the bug to appear are both of the same label. Maybe it's some kind of copy protection thing? Not sure what's important in this case, label or distributor.
Created attachment 218311 [details] [review] Proposed patch to fix the bug
Appears to have been caused by CD-Text including characters outside ASCII range. GFileInfo and D-Bus assume UTF-8. Latter choked with invalid string.
Review of attachment 218311 [details] [review]: ::: daemon/gvfsbackendcdda.c @@ +172,3 @@ cdtext = cdio_get_cdtext(cdio, 0); if (cdtext) { + cdda_backend->album_title = g_strdup (cdtext_get_const (CDTEXT_TITLE, cdtext)); You'll need to change the type of all those (->album_title, ->album_artist, etc.) if it's actually const now. @@ +929,3 @@ if (track->title) + { + char *title_utf8 = g_convert (track->title, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL); Check whether this is valid UTF-8 and only convert it if needed. And you should do that in a function. @@ +972,3 @@ + if (cdda_backend->album_title) + { + // CD-text doesn't specify encoding. In case outside ascii, assume latin-1. No C++-style comments please.
Review of attachment 218311 [details] [review]: ::: daemon/gvfsbackendcdda.c @@ +172,3 @@ cdtext = cdio_get_cdtext(cdio, 0); if (cdtext) { + cdda_backend->album_title = g_strdup (cdtext_get_const (CDTEXT_TITLE, cdtext)); They are not. This part only fixed memory leaks. Earlier cdtext_get() does internally strdup from the const data and then the g_strdup() created an actually stored string -> libcdio string was not freed. @@ +929,3 @@ if (track->title) + { + char *title_utf8 = g_convert (track->title, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL); I'm converting to UTF-8, not from. For avoiding conversions, I could iterate over the input string whether it has any highest bits set, i.e. not being plain ASCII, but I'm not at all sure if such optimization would be worth it. But function like cdtext_string_to_utf8() would remove these redundant conversion calls. @@ +972,3 @@ + if (cdda_backend->album_title) + { + // CD-text doesn't specify encoding. In case outside ascii, assume latin-1. Sure.
Created attachment 218365 [details] [review] Proposed patch v2 Gatherered similar multi-parameter g_convert() calls to a single function for conversion. Avoiding C++ comment style.
Review of attachment 218365 [details] [review]: ::: daemon/gvfsbackendcdda.c @@ +186,3 @@ if (cdtext) { + track->title = g_strdup (cdtext_get_const (CDTEXT_TITLE, cdtext)); + track->artist = g_strdup (cdtext_get_const (CDTEXT_PERFORMER, cdtext)); All those memory leak fixes need to be in a separate patch. @@ +896,3 @@ +static char * +cdtext_string_to_utf8(const char *string) You shouldn't trample on some other library's namespace. Prefix it with "gvfs_cdda_" for example. @@ +899,3 @@ +{ + /* CD-text doesn't specify encoding. In case outside ascii, assume latin-1. */ + return g_convert (string, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL); You should check whether the string is valid UTF-8 before converting it. If it's already valid UTF-8, don't touch it. See g_utf8_validate(). @@ +936,3 @@ if (track->title) + { + char *title_utf8 = cdtext_string_to_utf8 (track->title); Couldn't we call the utf-8 mashing functions directly when setting the strings, rather than every time we access the files? Instead of: cdda_backend->album_title = g_strdup (cdtext_get_const (CDTEXT_TITLE, cdtext)); we would have: cdda_backend->album_title = gvfs_cdda_get_utf8_for_type (cdtext, type); static char * gvfs_cdda_get_utf8_for_type (cdtext, type) ...
Review of attachment 218365 [details] [review]: ::: daemon/gvfsbackendcdda.c @@ +899,3 @@ +{ + /* CD-text doesn't specify encoding. In case outside ascii, assume latin-1. */ + return g_convert (string, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL); Input is not expected to be UTF-8 except by the ASCII subset. According to CD-text Wikipedia page original Sony authoring software supported ISO-8859-1 and "MS-JIS". Then again latin-1 extended string might be bitwise a valid UTF-8 string but the interpretation would be wrong. For avoiding conversion, I'm not able to spot any existing function to validate whether a string is plain ASCII. @@ +936,3 @@ if (track->title) + { + char *title_utf8 = cdtext_string_to_utf8 (track->title); Was initially about to directly convert the strings, but noticed create_header() has some initial support for writing metadata into the header as well as a TODO entry existing for that. I don't expect WAV file metadata to support UTF-8 and with such, a conversions would be needed anyway (set_info_for_track() calls create_header() to calculcate file size). Or alternatively forthcoming WAV metadata support would need new variables in track struct for the original CD-text data. For current features direct conversion would be cleaner so if you think we should just forget metadata for now, I can make such change. Btw, source code referring to 404 URL on the header format.
Created attachment 218653 [details] [review] cdda: Fix abort() with CD-Text outside ASCII CD-Text doesn't specify encoding, but in practice some discs have strings with extended characters. Using those directly will make D-Bus choke with invalid data. Fixed by assuming latin-1. This also fixes a memory leak in the cdtext_get() usage.
If somebody ever wrote metadata support for the WAVE headers, I'm pretty sure we'd be able to use UTF-8 for it anyway. If the above patch works for you, we'll commit that.
The above patch works ok.
How can I test the patch?
Attachment 218653 [details] pushed as 5624012 - cdda: Fix abort() with CD-Text outside ASCII