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 671259 - gvfsd-cdda crashed with signal 5 in _g_dbus_oom()
gvfsd-cdda crashed with signal 5 in _g_dbus_oom()
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: cdda backend
1.11.x
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2012-03-03 15:27 UTC by Walter Garcia-Fontes
Modified: 2012-08-07 14:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch to fix the bug (4.35 KB, patch)
2012-07-08 22:16 UTC, Pekka Vuorela
needs-work Details | Review
Proposed patch v2 (4.47 KB, patch)
2012-07-09 18:58 UTC, Pekka Vuorela
needs-work Details | Review
cdda: Fix abort() with CD-Text outside ASCII (3.24 KB, patch)
2012-07-12 18:04 UTC, Bastien Nocera
committed Details | Review

Description Walter Garcia-Fontes 2012-03-03 15:27:55 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
Comment 1 Hendrik Knackstedt 2012-05-22 09:43:59 UTC
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!
Comment 2 Hendrik Knackstedt 2012-05-23 20:55:41 UTC
This completly disables Rhythmbox from reading ANY Audio-CD! Please help!
Comment 3 Hendrik Knackstedt 2012-05-24 06:47:28 UTC
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.
Comment 4 Hendrik Knackstedt 2012-05-24 17:06:09 UTC
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!
Comment 5 Hendrik Knackstedt 2012-05-24 17:22:04 UTC
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.
Comment 6 Pekka Vuorela 2012-07-08 22:16:13 UTC
Created attachment 218311 [details] [review]
Proposed patch to fix the bug
Comment 7 Pekka Vuorela 2012-07-08 22:17:15 UTC
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.
Comment 8 Bastien Nocera 2012-07-08 22:59:22 UTC
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.
Comment 9 Pekka Vuorela 2012-07-08 23:23:21 UTC
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.
Comment 10 Pekka Vuorela 2012-07-09 18:58:04 UTC
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.
Comment 11 Bastien Nocera 2012-07-10 09:21:08 UTC
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)
...
Comment 12 Pekka Vuorela 2012-07-10 23:08:50 UTC
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.
Comment 13 Bastien Nocera 2012-07-12 18:04:34 UTC
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.
Comment 14 Bastien Nocera 2012-07-12 18:10:44 UTC
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.
Comment 15 Pekka Vuorela 2012-07-13 10:27:42 UTC
The above patch works ok.
Comment 16 Hendrik Knackstedt 2012-07-26 08:31:47 UTC
How can I test the patch?
Comment 17 Bastien Nocera 2012-08-07 14:49:59 UTC
Attachment 218653 [details] pushed as 5624012 - cdda: Fix abort() with CD-Text outside ASCII