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 726467 - Incorrectly allows storing non-numeric strings in TPOS frames of ID3 tags
Incorrectly allows storing non-numeric strings in TPOS frames of ID3 tags
Status: RESOLVED FIXED
Product: easytag
Classification: Other
Component: general
master
Other Linux
: Normal normal
: 2.1
Assigned To: EasyTAG maintainer(s)
EasyTAG maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-03-16 17:12 UTC by mail
Modified: 2014-04-13 05:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
esaytag 2.1.10 CD contains "1" instead of "1 - Ses chansons" (15.49 KB, image/png)
2014-03-16 17:12 UTC, mail
  Details
easytag 2.1.7 CD contains actual TPOS data "1 - Ses chansons" (18.79 KB, image/png)
2014-03-16 17:14 UTC, mail
  Details
This patch should fix the above bug. (4.02 KB, patch)
2014-04-11 13:22 UTC, Abhinav
rejected Details | Review
This patch will allow only numeric characters to be stored to ID3 Disc Number (3.44 KB, patch)
2014-04-11 18:45 UTC, Abhinav
needs-work Details | Review
Did changes (3.66 KB, patch)
2014-04-12 10:41 UTC, Abhinav
needs-work Details | Review
Did all the changes (3.71 KB, patch)
2014-04-12 12:41 UTC, Abhinav
accepted-commit_now Details | Review

Description mail 2014-03-16 17:12:38 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
Comment 1 mail 2014-03-16 17:14:19 UTC
Created attachment 272078 [details]
easytag 2.1.7 CD contains actual TPOS data "1 - Ses chansons"
Comment 2 David King 2014-03-16 17:22:05 UTC
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.
Comment 3 mail 2014-03-16 18:46:15 UTC
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.
Comment 4 David King 2014-03-18 22:12:38 UTC
Good point. That is a bug that should be fixed.
Comment 5 Abhinav 2014-04-11 13:22:04 UTC
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.
Comment 6 David King 2014-04-11 13:39:47 UTC
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.
Comment 7 Abhinav 2014-04-11 18:45:12 UTC
Created attachment 274126 [details] [review]
This patch will allow only numeric characters to be stored to ID3 Disc Number
Comment 8 David King 2014-04-11 19:17:00 UTC
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.
Comment 9 Abhinav 2014-04-12 10:41:02 UTC
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/'.
Comment 10 David King 2014-04-12 11:41:43 UTC
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.
Comment 11 Abhinav 2014-04-12 12:41:29 UTC
Created attachment 274160 [details] [review]
Did all the changes
Comment 12 David King 2014-04-12 13:31:16 UTC
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).
Comment 13 Abhinav 2014-04-13 05:38:57 UTC
I did the changes and pushed the patch to master in the commit 
40e2787c8dbb75f36f0b2c69cdf6270aa5df7b28 .