GNOME Bugzilla – Bug 726467
Incorrectly allows storing non-numeric strings in TPOS frames of ID3 tags
Last modified: 2014-04-13 05:47:35 UTC
Created attachment 272077 [details] esaytag 2.1.10 CD contains "1" instead of "1 - Ses chansons" easytag 2.1.10 miss some data parsing id3 tag TPOS (CD number). It may results in loss of data. easytag 2.1.7 parse it properly. example : mid3v2 -l '/media/Saturne/!MP3/!01 Complete Albums/Vian, Boris/Vian, Boris (2013) L'\''ecume des nuits (1 - Ses chansons)/Vian, Boris - Complainte du progres.mp3' IDv2 tag info for /media/Saturne/!MP3/!01 Complete Albums/Vian, Boris/Vian, Boris (2013) L'ecume des nuits (1 - Ses chansons)/Vian, Boris - Complainte du progres.mp3: TPOS=1 - Ses chansons
Created attachment 272078 [details] easytag 2.1.7 CD contains actual TPOS data "1 - Ses chansons"
As documented in the ID3v2.3 and ID3v2.4 specifications, TPOS is a "numeric string", that is, a string composed of digits only, which may be extended with '/' characters: http://id3.org/id3v2.3.0 http://id3.org/id3v2.4.0-frames Therefore, adding non-numeric (excluding '/') characters to the TPOS frame does not comply with the ID3v2 specification and will not be supported.
easytag (2.1.7 or 2.1.10) allows to enter non digits CD number, and record it into TPOS id3 tag. easytag 2.1.10 scraps non digits characters during tags parsing. It should be better to doesn't allow out of specification CD number.
Good point. That is a bug that should be fixed.
Created attachment 274097 [details] [review] This patch should fix the above bug. There could be 2 ways to solve the problem of copying only numeric characters. 1. Allocate n characters to string and copy only numerics. This will lead to waste of memory. 2. Copy and allocate while copying. I prefer this one and GString does that automatically. So, I decided to use GString.
Review of attachment 274097 [details] [review]: As this is only for ID3v2 tags, it should be done wholly in id3v24_tag.c and id3_tag.c.
Created attachment 274126 [details] [review] This patch will allow only numeric characters to be stored to ID3 Disc Number
Review of attachment 274126 [details] [review]: OK, this is now done in the right place, but I do not think that it is the right approach. There is little point in using varargs, because the function does not need to be generic. A better approach would be to step through the string, checking that each character is either a digit or a '/'. Take the contents of the string up to the first non-allowed character and write that string to the tag, combining the two strings with the '/' as necessary. One approach to do that would be to use strstr() to search for the current character (taken from the disc number string) in the string "0123456789/", and allow the character if the result is a match.
Created attachment 274159 [details] [review] Did changes As you said, to copy characters only until a non-allowed character. I took this approach. Also, I decided to use isdigit() instead of using strstr () and search in '0123456789/'.
Review of attachment 274159 [details] [review]: Looks good to me! Just some minor style issues. Can you also change the commit message to be under 50 characters on the first line? "Add only numeric characters to TPOS in ID3 Tags" should be fine. Then, leave a blank line, and finally, add the link to this bug "https://bugzilla.gnome.org/show_bug.cgi?id=726467". ::: src/id3_tag.c @@ +92,3 @@ + * Returns: a newly allocated string, should be freed using g_free. + */ + Cut this blank line. @@ +101,3 @@ + gstring = g_string_new (""); + p = FileTag->disc_number; + while (*p) Add a blank line before the while(). @@ +116,3 @@ + g_string_append_c (gstring, '/'); + p = FileTag->disc_total; + while (*p) Another blank line here. @@ +305,3 @@ id3_frame = ID3Frame_NewID (ID3FID_PARTINSET); ID3Tag_AttachFrame (id3_tag, id3_frame); + string1 = et_id3tag_get_tpos_from_file_tag (FileTag); This should be indented with spaces, not tabs. ::: src/id3_tag.h @@ +45,3 @@ +gchar * +et_id3tag_get_tpos_from_file_tag (File_Tag *file_tag); Keep this on one line, and make sure that you leave a blank line before the #endif.
Created attachment 274160 [details] [review] Did all the changes
Review of attachment 274160 [details] [review]: Looks good except for a trivial indentation problem. Also, for the commit message, leave a blank line after the first-line summary, then have just the URL on the following line (the third line), for example: " Add only numeric characters to TPOS in ID3 Tags https://bugzilla.gnome.org/show_bug.cgi?id=726467 " Once that is all fixed, please push to master. ::: src/id3_tag.c @@ +305,3 @@ id3_frame = ID3Frame_NewID (ID3FID_PARTINSET); ID3Tag_AttachFrame (id3_tag, id3_frame); + string1 = et_id3tag_get_tpos_from_file_tag (FileTag); It seems like you are not quite indented correctly here (just by 1 space).
I did the changes and pushed the patch to master in the commit 40e2787c8dbb75f36f0b2c69cdf6270aa5df7b28 .