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 172781 - ripping fails for excessively long file names
ripping fails for excessively long file names
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: ripping
2.16.x
Other All
: Normal normal
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
Depends on:
Blocks:
 
 
Reported: 2005-04-06 10:16 UTC by James Henstridge
Modified: 2008-01-11 17:22 UTC
See Also:
GNOME target: ---
GNOME version: 2.9/2.10


Attachments
Adds a test case and shortens the filename if needed (1.74 KB, patch)
2006-11-13 06:29 UTC, Adam Petaccia
needs-work Details | Review
Makes sure that we obey file system character limits (2.69 KB, patch)
2007-01-31 16:19 UTC, Adam Petaccia
needs-work Details | Review
Use values from limits.h (2.42 KB, patch)
2007-02-01 16:22 UTC, Adam Petaccia
needs-work Details | Review
Truncate the filename is needed (2.41 KB, patch)
2007-05-03 15:51 UTC, Adam Petaccia
needs-work Details | Review
sj-filelength.patch (5.12 KB, patch)
2008-01-06 22:17 UTC, Ed Catmur
committed Details | Review

Description James Henstridge 2005-04-06 10:16:39 UTC
Please describe the problem:
If the generated file name for a track is too long for the operating system to
handle (4096 bytes on Linux), the track is silently skipped when ripping the album.

Steps to reproduce:
1. Insert a CD
2. Enter a really long title for a track (or do nothing if the MusicBrainz data
gives a really long title).
3. click the "Extract" button.


Actual results:
The tracks with okay filenames get ripped.  The tracks with long filenames get
skipped.

Expected results:
The tracks with long file names should either have the file name truncated, or
the user should be prompted to pick an alternative file name (the user shouldn't
be prompted to change the track title, since it can probably be included
unchanged in the metadata).

Does this happen every time?
For affected track names, yes.

Other information:
Comment 1 James Henstridge 2005-04-06 10:22:40 UTC
I was wrong about the maximum file name length.  I didn't run into the path name
max, but the file name max (for one component of a file name).

For Linux, this is 255 bytes, which is easier to run into (e.g. with mix albumn
tracks that contain multiple songs).  I suppose it is even easier to run into
with  non-english tracks due to UTF-8 encoding.
Comment 3 Adam Petaccia 2006-11-13 06:29:24 UTC
Created attachment 76461 [details] [review]
Adds a test case and shortens the filename if needed

I can confirm that this is a bug, even though the wikipedia link is no longer valid.  This patch checks to make sure that the string is less than 250 chars, and if it isn't, it gives an error dialog, and shortens it for the user.
Comment 4 Ed Catmur 2006-11-13 19:58:55 UTC
Yeah, it got deleted. Still visible at http://en.wikipedia.org/wiki/User:Gbeeker/List_of_songs_with_particularly_long_titles

Thanks for the patch; looking at it now.
Comment 5 Ed Catmur 2006-11-13 23:07:39 UTC
Problems:

1. You're assuming that 255 bytes is the filename length limit. This is fs-dependent; reiser4... well, OK, almost all filesystems have the 255-byte limit. In truth, no-one's going to be using a filesystem with a lesser limit (even vfat supports 255 bytes) and anyone using a filesystem with a longer limit is unlikely to care over much. But 255 should be made a #define constant.
2. You're assuming that the profile extension is a maximum of 4 bytes. Not necessarily the case; instead compare the file name to the maximum component length less the extension and '.'.
3. You're solving the wrong problem; it's each component of the path that needs to be at most 255 bytes, not the path itself. Split realfile on '/'s and truncate each excess component to 255 bytes or (for the last component) 255 bytes less the extension and '.'.
4. There is zero point notifying the user with an error dialog; there's nothing they can do about it. Write a message(s) to stderr (use g_warning etc.)
5. The error message could be better. Not your fault; your English is far better than my Italian. Correct the above points and I'll help with the translation.
Comment 6 Ross Burton 2006-11-19 12:58:45 UTC
As per the review by Ed, marking patch as needs-work.
Comment 7 Adam Petaccia 2006-12-29 17:51:24 UTC
I'm sorry, I'm somewhat new to programming in just C, and I've had some problems locating the correct function to split the string, and I would like to avoid "reinventing the wheel". I've seen strtok(), but the references I've seen say that's its not thread safe, and for other reasons, should generally be avoided.  

Is there a glib function I should be using?  Or should I just try to go through this by hand (scan string for /, memcpy() it to variable, move on, etc)?
Comment 8 Ed Catmur 2007-01-03 04:14:07 UTC
g_strsplit(). See:
http://developer.gnome.org/doc/API/2.0/glib/glib-String-Utility-Functions.html#g-strsplit

Why do you need to, though? You should be working within filepath_parse_pattern(), where the path is constructed.

Also note MAXPATHLEN, typically 1024, although again the chances of hitting that are very slim.
Comment 9 Adam Petaccia 2007-01-31 16:19:23 UTC
Created attachment 81601 [details] [review]
Makes sure that we obey file system character limits

I haven't had a chance to test this, to be honest, and I wont for a while, but this should work;  Thanks to Ed Catmur for the help.
Comment 10 Ross Burton 2007-01-31 17:07:06 UTC
Instead of defining the path limit to be 1024, include limits.h and use PATH_MAX and NAME_MAX.  On Linux:

#define NAME_MAX         255    /* # chars in a file name */
#define PATH_MAX        4096    /* # chars in a path name including nul */

Note that on Hurd, as far as I recall, PATH_MAX is undefined as there isn't a maximum.
Comment 11 Adam Petaccia 2007-02-01 16:22:54 UTC
Created attachment 81674 [details] [review]
Use values from limits.h

I put the pathname limit in an #ifdef statement, going on what was said about GNU/Hurd not having it defined, because it didn't have the limitation.  Other than that, it should be good, now.

2007-02-01 Adam Petaccia <adam@tpetaccia.com>
	* src/sj-extracting.h:
	* src/sj-extracting.c:
	Don't write out files larger than the OS can handle
Comment 12 Ross Burton 2007-03-26 09:54:09 UTC
Comment on attachment 81674 [details] [review]
Use values from limits.h

You are creating a new GString for extension, then just accessing the ->str attribute, and then freeing it with g_free (which won't work).  Apart from the bug, why not just use a char*?
Comment 13 Adam Petaccia 2007-03-31 20:22:39 UTC
Yes, the gfree() part is a bug.  The reason for GString is to gain access to the ->len attribute, for determining the max length.  Also, as noted above, file extensions don't always have 3 letters + '.' (eg: .flac), so I thought it safer to do that, as getting the extension length just seemed more correct.
Comment 14 Ross Burton 2007-04-26 10:12:57 UTC
If you can fix the free bug and use a normal string with strlen(), I'll commit this.
Comment 15 Adam Petaccia 2007-05-03 15:51:27 UTC
Created attachment 87460 [details] [review]
Truncate the filename is needed

Sorry about the delay, this patch addresses the all bugs previously mentioned.  Thank you for the needed direction.

2007-05-03  Adam Petaccia <adam@tpetaccia.com>
	* src/sj-extracting.c:
	Don't silently fail if the filename is too long. Instead, truncate
	the filename.
Comment 16 Ross Burton 2007-05-13 09:32:54 UTC
The if PATHMAX block of code is wrong:

sj-extracting.c: In function ‘build_filename’:
sj-extracting.c:140: warning: passing argument 1 of ‘g_string_truncate’ from incompatible pointer type
sj-extracting.c:140: warning: assignment from incompatible pointer type

The line in question is:

+  uri = g_string_truncate (uri, PATH_MAX - 1 - tmp->len);

uri is a GnomeVFSURI, not a GString.

Also the PATH_MAX truncation happens after the extension is appended, which means that the extension will always be clobbered.  Instead can you check for a length that will exceed PATH_MAX before appending the extension?
Comment 17 Ed Catmur 2008-01-06 22:17:04 UTC
Created attachment 102290 [details] [review]
sj-filelength.patch

Fixed up patch.
Comment 18 Ross Burton 2008-01-07 20:47:44 UTC
Great, please commit.
Comment 19 Ross Burton 2008-01-11 17:22:38 UTC
Committed, thanks.