From 0f2e62796c0d5589f85be66f50fccf7905477d56 Mon Sep 17 00:00:00 2001 From: Sophist Date: Thu, 3 Apr 2014 13:54:46 +0100 Subject: [PATCH 1/6] Fix error on malformed TRCK tag. --- NEWS.txt | 2 +- picard/formats/id3.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/NEWS.txt b/NEWS.txt index 6880a76bf..e60c9408d 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -34,7 +34,7 @@ * Support dropping image directly from Google image results to cover art box * Add %_musicbrainz_tracknumber% to hold track # as shown on MusicBrainz release web-page e.g. vinyl/cassette style A1, A2, B1, B2 - + * Fixed a bug where Picard crashed if a MP3 file had a malformed TRCK tag (PICARD-112) Version 1.2 - 2013-03-30 * Picard now requires at least Python 2.6 diff --git a/picard/formats/id3.py b/picard/formats/id3.py index 1d402373f..25de35686 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -241,10 +241,10 @@ class ID3File(File): metadata['musicbrainz_recordingid'] = frame.data.decode('ascii', 'ignore') elif frameid == 'TRCK': value = frame.text[0].split('/') - if len(value) > 1: - metadata['tracknumber'], metadata['totaltracks'] = value[:2] - else: + if len(value) == 1 and value[0].isdigit(): metadata['tracknumber'] = value[0] + elif len(value) == 2 and value[0].isdigit() and value[1].isdigit(): + metadata['tracknumber'], metadata['totaltracks'] = value elif frameid == 'TPOS': value = frame.text[0].split('/') if len(value) > 1: From 388a48219ebe8fd097cdc9da5fd8f76c3d6b5e6f Mon Sep 17 00:00:00 2001 From: Sophist Date: Thu, 3 Apr 2014 13:57:54 +0100 Subject: [PATCH 2/6] Provide error log on invalid format. --- picard/formats/id3.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/picard/formats/id3.py b/picard/formats/id3.py index 25de35686..3522a5592 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -245,6 +245,8 @@ class ID3File(File): metadata['tracknumber'] = value[0] elif len(value) == 2 and value[0].isdigit() and value[1].isdigit(): metadata['tracknumber'], metadata['totaltracks'] = value + else: + log.error("Invalid TRCK value '%s' dropped in %r", frame.text[0], filename) elif frameid == 'TPOS': value = frame.text[0].split('/') if len(value) > 1: From 02168dc50d5df68fd0ea1e7a825a038b580598cb Mon Sep 17 00:00:00 2001 From: Sophist Date: Thu, 3 Apr 2014 18:03:26 +0100 Subject: [PATCH 3/6] Provide same checks for TPOS --- picard/formats/id3.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/picard/formats/id3.py b/picard/formats/id3.py index 3522a5592..1d8bf4706 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -249,10 +249,12 @@ class ID3File(File): log.error("Invalid TRCK value '%s' dropped in %r", frame.text[0], filename) elif frameid == 'TPOS': value = frame.text[0].split('/') - if len(value) > 1: - metadata['discnumber'], metadata['totaldiscs'] = value[:2] - else: + if len(value) == 1 and value[0].isdigit(): metadata['discnumber'] = value[0] + elif len(value) == 2 and value[0].isdigit() and value[1].isdigit(): + metadata['discnumber'], metadata['totaldiscs'] = value + else: + log.error("Invalid TPOS value '%s' dropped in %r", frame.text[0], filename) elif frameid == 'APIC': extras = { 'desc': frame.desc, From 09f726fcdca38ce6771437f5165962281908b9a5 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Fri, 4 Apr 2014 12:07:50 +0200 Subject: [PATCH 4/6] Match TRCK and TPOS using regex and reduce code redundancy. --- picard/formats/id3.py | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/picard/formats/id3.py b/picard/formats/id3.py index 1d8bf4706..6f67fe82c 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -20,6 +20,7 @@ import mutagen.apev2 import mutagen.mp3 import mutagen.trueaudio +import re from collections import defaultdict from mutagen import id3 from picard import config, log @@ -175,6 +176,11 @@ class ID3File(File): __other_supported_tags = ("discnumber", "tracknumber", "totaldiscs", "totaltracks") + __tag_subvalues = { + 'TRCK': ('tracknumber', 'totaltracks'), + 'TPOS': ('discnumber', 'totaldiscs') + } + __tag_subvalues_re = re.compile(r'^(\d+)(?:/(\d+))?$') def __init__(self, filename): super(ID3File, self).__init__(filename) @@ -239,22 +245,17 @@ class ID3File(File): metadata.add(name, unicode(frame.text)) elif frameid == 'UFID' and frame.owner == 'http://musicbrainz.org': metadata['musicbrainz_recordingid'] = frame.data.decode('ascii', 'ignore') - elif frameid == 'TRCK': - value = frame.text[0].split('/') - if len(value) == 1 and value[0].isdigit(): - metadata['tracknumber'] = value[0] - elif len(value) == 2 and value[0].isdigit() and value[1].isdigit(): - metadata['tracknumber'], metadata['totaltracks'] = value + elif frameid in ('TRCK', 'TPOS'): + # frame is a numeric string, eventually extended with a "/" character + # and a numeric string containing the total number. + # E.g: "1" or "1/2" + m = self.__tag_subvalues_re.search(frame.text[0]) + if m: + metadata[self.__tag_subvalues[frameid][0]] = m.group(1) + if m.group(2) is not None: + metadata[self.__tag_subvalues[frameid][1]] = m.group(2) else: - log.error("Invalid TRCK value '%s' dropped in %r", frame.text[0], filename) - elif frameid == 'TPOS': - value = frame.text[0].split('/') - if len(value) == 1 and value[0].isdigit(): - metadata['discnumber'] = value[0] - elif len(value) == 2 and value[0].isdigit() and value[1].isdigit(): - metadata['discnumber'], metadata['totaldiscs'] = value - else: - log.error("Invalid TPOS value '%s' dropped in %r", frame.text[0], filename) + log.error("Invalid %s value '%s' dropped in %r", frameid, frame.text[0], filename) elif frameid == 'APIC': extras = { 'desc': frame.desc, From 8f91a9f7eb20dec07351c28a51583f2354574adc Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Fri, 4 Apr 2014 15:06:41 +0200 Subject: [PATCH 5/6] Use re named groups to match values and metadata keys. --- picard/formats/id3.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/picard/formats/id3.py b/picard/formats/id3.py index 6f67fe82c..bd40679c4 100644 --- a/picard/formats/id3.py +++ b/picard/formats/id3.py @@ -176,11 +176,10 @@ class ID3File(File): __other_supported_tags = ("discnumber", "tracknumber", "totaldiscs", "totaltracks") - __tag_subvalues = { - 'TRCK': ('tracknumber', 'totaltracks'), - 'TPOS': ('discnumber', 'totaldiscs') + __tag_re_parse = { + 'TRCK': re.compile(r'^(?P\d+)(?:/(?P\d+))?$'), + 'TPOS': re.compile(r'^(?P\d+)(?:/(?P\d+))?$') } - __tag_subvalues_re = re.compile(r'^(\d+)(?:/(\d+))?$') def __init__(self, filename): super(ID3File, self).__init__(filename) @@ -245,15 +244,12 @@ class ID3File(File): metadata.add(name, unicode(frame.text)) elif frameid == 'UFID' and frame.owner == 'http://musicbrainz.org': metadata['musicbrainz_recordingid'] = frame.data.decode('ascii', 'ignore') - elif frameid in ('TRCK', 'TPOS'): - # frame is a numeric string, eventually extended with a "/" character - # and a numeric string containing the total number. - # E.g: "1" or "1/2" - m = self.__tag_subvalues_re.search(frame.text[0]) + elif frameid in self.__tag_re_parse.keys(): + m = self.__tag_re_parse[frameid].search(frame.text[0]) if m: - metadata[self.__tag_subvalues[frameid][0]] = m.group(1) - if m.group(2) is not None: - metadata[self.__tag_subvalues[frameid][1]] = m.group(2) + for name, value in m.groupdict().iteritems(): + if value is not None: + metadata[name] = value else: log.error("Invalid %s value '%s' dropped in %r", frameid, frame.text[0], filename) elif frameid == 'APIC': From 0a87b62ee7045faff71a2eb1096b833ca7a7a211 Mon Sep 17 00:00:00 2001 From: Sophist Date: Fri, 4 Apr 2014 20:25:02 +0100 Subject: [PATCH 6/6] Update NEWS --- NEWS.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.txt b/NEWS.txt index e60c9408d..39d6d44e3 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -34,7 +34,7 @@ * Support dropping image directly from Google image results to cover art box * Add %_musicbrainz_tracknumber% to hold track # as shown on MusicBrainz release web-page e.g. vinyl/cassette style A1, A2, B1, B2 - * Fixed a bug where Picard crashed if a MP3 file had a malformed TRCK tag (PICARD-112) + * Fixed a bug where Picard crashed if a MP3 file had malformed TRCK or TPOS tags (PICARD-112) Version 1.2 - 2013-03-30 * Picard now requires at least Python 2.6