From 5b7ee5bc1a5366e26172ec688fd3c9a3d180b14f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 8 Jan 2016 01:57:18 +0900 Subject: [PATCH 01/18] Initialize all the private data members. --- taglib/mpeg/mpegheader.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 71e00663..7f82a607 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -42,6 +42,7 @@ public: version(Version1), layer(0), protectionEnabled(false), + bitrate(0), sampleRate(0), isPadded(false), channelMode(Stereo), @@ -308,7 +309,7 @@ void MPEG::Header::parse(File *file, long offset, bool checkLength) bool nextFrameFound = false; file->seek(offset + d->frameLength); - const ByteVector nextSynch = file->readBlock(4); + const ByteVector nextSynch = file->readBlock(16); for(int i = 0; i < static_cast(nextSynch.size()) - 1; ++i) { if(firstSyncByte(nextSynch[i]) && secondSynchByte(nextSynch[i + 1])) { From 7a5fb7d672a966344bd632b9fcc8326d0fa7c58b Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 8 Jan 2016 02:22:44 +0900 Subject: [PATCH 02/18] Revert some unnecessary changes. --- taglib/mpeg/mpegheader.cpp | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 7f82a607..53c559fa 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -306,19 +306,10 @@ void MPEG::Header::parse(File *file, long offset, bool checkLength) if(checkLength) { - bool nextFrameFound = false; - file->seek(offset + d->frameLength); - const ByteVector nextSynch = file->readBlock(16); + const ByteVector nextSynch = file->readBlock(2); - for(int i = 0; i < static_cast(nextSynch.size()) - 1; ++i) { - if(firstSyncByte(nextSynch[i]) && secondSynchByte(nextSynch[i + 1])) { - nextFrameFound = true; - break; - } - } - - if(!nextFrameFound) { + if(nextSynch.size() < 2 || !firstSyncByte(nextSynch[0]) || !secondSynchByte(nextSynch[1])) { debug("MPEG::Header::parse() -- Calculated frame length did not match the actual length."); return; } From 1254534ed4cba60d170145c026bd6ed28a6796a7 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 8 Jan 2016 02:30:17 +0900 Subject: [PATCH 03/18] Another workaround for broken MPEG headers. --- taglib/mpeg/mpegutils.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/taglib/mpeg/mpegutils.h b/taglib/mpeg/mpegutils.h index 78cee280..5a7e0e6a 100644 --- a/taglib/mpeg/mpegutils.h +++ b/taglib/mpeg/mpegutils.h @@ -48,7 +48,9 @@ namespace TagLib inline bool secondSynchByte(unsigned char byte) { - return ((byte & 0xE0) == 0xE0); + // 0xFF is possible in theory, but it's very unlikely be a header. + + return (byte != 0xFF && ((byte & 0xE0) == 0xE0)); } } From 3dc873b7d0a58184c936ca9e7b43d7cfd7ae927a Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Fri, 8 Jan 2016 20:08:04 +0900 Subject: [PATCH 04/18] Check if two consecutive MPEG audio frames are consistent. This fixes reading the audio properties of some MP3 files reported by a Kodi user. This is basically the same check as FFmpeg does. --- taglib/mpeg/mpegheader.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 53c559fa..1b93560b 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -307,10 +307,24 @@ void MPEG::Header::parse(File *file, long offset, bool checkLength) if(checkLength) { file->seek(offset + d->frameLength); - const ByteVector nextSynch = file->readBlock(2); + const ByteVector nextData = file->readBlock(4); - if(nextSynch.size() < 2 || !firstSyncByte(nextSynch[0]) || !secondSynchByte(nextSynch[1])) { - debug("MPEG::Header::parse() -- Calculated frame length did not match the actual length."); + if(nextData.size() < 4) { + debug("MPEG::Header::parse() -- Could not read the next frame header."); + return; + } + + // The MPEG versions, layers and sample rates of the two frames should be + // consistent. Otherwise, we assume that either or both of the frames are + // broken. + + const unsigned int HeaderMask = 0xfffe0c00; + + const unsigned int header = data.toUInt(0, true) & HeaderMask; + const unsigned int nextHeader = nextData.toUInt(0, true) & HeaderMask; + + if(header != nextHeader) { + debug("MPEG::Header::parse() -- The next frame was not consistent with this frame."); return; } } From b8e82a7775e3f635192feff579caef745c3b0782 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 9 Jan 2016 10:13:07 +0900 Subject: [PATCH 05/18] Amend an outdated comment. --- taglib/mpeg/mpegheader.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 1b93560b..e678f15b 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -299,13 +299,15 @@ void MPEG::Header::parse(File *file, long offset, bool checkLength) if(d->isPadded) d->frameLength += paddingSize[layerIndex]; - // Check if the frame length has been calculated correctly, or the next frame - // synch bytes are right next to the end of this frame. - - // We read some extra bytes to be a bit tolerant. - if(checkLength) { + // Check if the frame length has been calculated correctly, or the next frame + // header is right next to the end of this frame. + + // The MPEG versions, layers and sample rates of the two frames should be + // consistent. Otherwise, we assume that either or both of the frames are + // broken. + file->seek(offset + d->frameLength); const ByteVector nextData = file->readBlock(4); @@ -314,10 +316,6 @@ void MPEG::Header::parse(File *file, long offset, bool checkLength) return; } - // The MPEG versions, layers and sample rates of the two frames should be - // consistent. Otherwise, we assume that either or both of the frames are - // broken. - const unsigned int HeaderMask = 0xfffe0c00; const unsigned int header = data.toUInt(0, true) & HeaderMask; From fe92f3dffef91685414cb007b43f7796b475bee7 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 9 Jan 2016 13:30:00 +0900 Subject: [PATCH 06/18] Add a test for broken MPEG audio frames. --- .../{invalid-frames.mp3 => invalid-frames1.mp3} | Bin tests/data/invalid-frames3.mp3 | Bin 0 -> 8192 bytes tests/test_mpeg.cpp | 16 +++++++++++++++- 3 files changed, 15 insertions(+), 1 deletion(-) rename tests/data/{invalid-frames.mp3 => invalid-frames1.mp3} (100%) create mode 100644 tests/data/invalid-frames3.mp3 diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames1.mp3 similarity index 100% rename from tests/data/invalid-frames.mp3 rename to tests/data/invalid-frames1.mp3 diff --git a/tests/data/invalid-frames3.mp3 b/tests/data/invalid-frames3.mp3 new file mode 100644 index 0000000000000000000000000000000000000000..6bbd2d39720f4894cbea4358e5b85d6507ce0a0b GIT binary patch literal 8192 zcmeH~XHZjV8-`El2{8nugeoQUvY~^bhJbW1AR=8L^d^M9B1WW1Cm;fX1_)ich=OS7 zEh0!qTnQb4-A{#80p(+7cW3|X%+BoW58dVbIcMH?&Ux?adS>pK(;E_*yP^kkhA=sM zzJ3raZWh%4TI-f6;wOys)QR+D!7aUy~2l)rs`S9W|+kg zR|oJ1D<}E&x~KwD><)+@&xqzEMlIsDE^TF=BLx)pFi+|D2b>CYTOR;=nLzEuspzPg zD-oCvTlx2jqD64TrPE)fhyO~bz5U%eGyt&e0mcLX0Ou3H0RR93Kc?p+0RVs?;4BYr z{}maBoQDR!H3L98^Umqfe<<}AkTBcU)r{XdVHfC;Iw*sDXN@s{PMl@VR1jd$7gl8y z!vZW>pmgS+Ia{`&8oz|ljYV}Fz%?K#bW(O||J^9v`GQry8fS9Z=RMhh!oZ3U+O z`e!keNKe?R|sl4gG?n^RZEZj74019#Q=%<{OPeFDA(|&cjy}=ze-J#V@((Kw@%pM z=;;$ztXH73o@T(QL4Dhf#ka|NMP`Y;jjxyIk|{lp*3fhnF(v8x<&64<>Rf%fO)g9n zj(FyL;tMG;e&P7lze;ep|IHfcHmWGW!oY(g+?N@TVld$|XP0#nvBE{n)ep9UbfOh^ zTMnoH$8fu{U%K6W1G=yb2PljH8o+`migbFAfS|a0Vsm9IQwx&5;NB$T#XY;O0yZ5^ zxM+RhiftR46{35nm@BcWXGm4E_jc&u1(=7Zc0}4lIa&{(0hpnnWO^Z$CpMgeh0m2(UCX$*_%4p z@X^=iX z-IQ}ZrTS0ySN4alczpkd&BLd!)4)KDzFdI;$OZ}YgbX{l_PSPP+b`)%)Tg$}=ZW$z z3K@_s+|1s$(R;}Fm|1FVo<2J^wxuE^l_BryT6_bGI^E46V328_r;AwVC}PS07oFvI z^^pA#_Ho5Z{Obh%^Hhq1enfSCSK!rTUu8#}yac?v=_En;m)$$(3 z7OReQG6rX~6*_+2J5e@KIhGZB^fY|GC(KAU_={)HX12ksUpoE7GlGC{j??_oVvs|_ z9FKE#IW7r;VsUcEHVzdb9GJ4Z*crz0aNEGPO=+By zth&GGP=3mRC8D6P^%n0%{Ba5Fs?mhtxjh?ijayT#UdrBL9JxN3DoOPp*EjAY7Y>_- zs0Y%#5@O;N$;LuVHgL=2k*@4s`&eebSMdu?yqG+ljPM?Wg;RUU*2!akKGuKfDvi8> zS4U=~i!vDm$f6#i&9{cmjsTo&qQWp1NBS8vY34{9XuWA)>SQN14ZOjiH0-k>I0|{9 z_NbjMgi}yqddEo=f-8FH=rK#^A*h9py1ngqm5XT`YwtF}Z)mR<9GQJMC^nQq36P$) zy-pi9oBg3RMjU|Ut3Q@((I>DKgoXU6`8Y>zxhVwra zPThEJClP9EWW-})S=_ISe9L{b3TTtPxndc+zz#aoC8>~zkU(*nXm+zQqI&75^H4)# z4Sp;89rQbkPHB1#8<%2Z30H?(A~YLq+|x}4s8s|sdqB{O?RM1SK&kC}Y$)GC3iOeR z>Jo9sr1py6ht|muTiFunSDQ0~p(7xz<6BV^}VX6tC$=$f125C=Ut6Q-1q(r~Nxjc1MVZLcr1hbt02QK*ukEJV$Zl(>At4uQXjK7h?@KT?m>_>?=sCO46wrt zwj<6>sI#+=cty}B{)m<#bJ{)VMw>dr`@IUzlUCF>md{K3Py=UuQZ%Q18kO;Vuf)B6 znCN}_xZx4`q3^Gqz#6Axv~YW7b;dKoKInS~K676E>PxbXhGJ6N)k|U8rnnnVXXzcV z?Uc0yJqeqWc?EgF!D|Uw`T`i<7J)$H!qrN>)l*ts-bWayOlpoEi=~`Qo|2^saPM<* zk8@0p%P9a5dRSf~s2mEV&jgEiW?u~E37xv_fJ$#0#Va{-nS5;H<7 zS4y~iIz6%QI?~CvtFzrA;b}^Wbqty@-5XY|FJdgx*XQC)| z2+kzX22Ou%Rk0VVnyR)R!1iWJ0G1Qu=oZvpKJB4nmJ@*vBrE2LFhd^-C6fJh$W*<2 zD)u2x+&$FDncyCUcOmH&q5_WS8qjquL**d6s*If`+z3V~m?5(~eB|>>7yq^EF*(A{ z*Ff>da@yZkX$S<5?4m9)A(UK5G49kgmE=o&wr>?lW70?Y`RYYMetg%f%%?1R@#Ynu z9H!UwWDq$#*;=D)ULUpa+ZWC(zQxVCy%iGUZ3@@u#TSz1GEDR`S^H47}ZjMQ_P9>)<3iGx58GKGkDvy)1 z=&DeaMFkq>2HGrWPOeCd7djWHt;SNCGQT+2FHpL4NH*8OkLaS+JuWPnBx~BO9E~!0+t1o zr&khxB|;&&!VM0H6z-_NrwAagT+aidg17;aG5Z0Ly9`a!2@Vz>w8^y$zCG~8{C}cs k%1l1gSh3iOD8atKe3qkl^TK>+94uK79G+RYlength()); CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); @@ -131,6 +132,19 @@ public: CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); } + void testSkipInvalidFrames3() + { + MPEG::File f(TEST_FILE_PATH_C("invalid-frames3.mp3")); + CPPUNIT_ASSERT(f.audioProperties()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(176, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(320, f.audioProperties()->bitrate()); + CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels()); + CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate()); + CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); + } + void testVersion2DurationWithXingHeader() { MPEG::File f(TEST_FILE_PATH_C("mpeg2.mp3")); From 0b62ba15305ddd86108192c62288985c1644cb13 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 28 Jan 2016 12:00:27 +0900 Subject: [PATCH 07/18] Remove the body of deprecated function Ogg::Page::getCopyWithNewPageSequenceNumber(). --- NEWS | 1 + taglib/ogg/oggpage.cpp | 23 +++-------------------- taglib/ogg/oggpage.h | 2 +- 3 files changed, 5 insertions(+), 21 deletions(-) diff --git a/NEWS b/NEWS index a3e5c93a..d9aa57b7 100644 --- a/NEWS +++ b/NEWS @@ -29,6 +29,7 @@ * Marked ByteVector::null and ByteVector::isNull() deprecated. * Marked String::null and ByteVector::isNull() deprecated. * Marked XiphComment::removeField() deprecated. + * Marked Ogg::Page::getCopyWithNewPageSequenceNumber() deprecated. It returns null. * Marked custom integer types deprecated. * Many smaller bug fixes and performance improvements. diff --git a/taglib/ogg/oggpage.cpp b/taglib/ogg/oggpage.cpp index 9f3b3dc4..75aea22a 100644 --- a/taglib/ogg/oggpage.cpp +++ b/taglib/ogg/oggpage.cpp @@ -269,27 +269,10 @@ List Ogg::Page::paginate(const ByteVectorList &packets, return l; } -Ogg::Page* Ogg::Page::getCopyWithNewPageSequenceNumber(int sequenceNumber) +Ogg::Page* Ogg::Page::getCopyWithNewPageSequenceNumber(int /*sequenceNumber*/) { - Page *pResultPage = NULL; - - // TODO: a copy constructor would be helpful - - if(d->file == 0) { - pResultPage = new Page( - d->packets, - d->header.streamSerialNumber(), - sequenceNumber, - d->header.firstPacketContinued(), - d->header.lastPacketCompleted(), - d->header.lastPageOfStream()); - } - else - { - pResultPage = new Page(d->file, d->fileOffset); - pResultPage->d->header.setPageSequenceNumber(sequenceNumber); - } - return pResultPage; + debug("Ogg::Page::getCopyWithNewPageSequenceNumber() -- This function is obsolete. Returning null."); + return 0; } //////////////////////////////////////////////////////////////////////////////// diff --git a/taglib/ogg/oggpage.h b/taglib/ogg/oggpage.h index 3fbf75d7..13e3e7f9 100644 --- a/taglib/ogg/oggpage.h +++ b/taglib/ogg/oggpage.h @@ -91,7 +91,7 @@ namespace TagLib { * \see header() * \see PageHeader::setPageSequenceNumber() * - * \deprecated + * \deprecated Always returns null. */ Page* getCopyWithNewPageSequenceNumber(int sequenceNumber); From 4ba7bb5a8ab6f3e55e277f772244a063ca18cd01 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 28 Jan 2016 12:13:18 +0900 Subject: [PATCH 08/18] Reorganize NEWS to put new features first. --- NEWS | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index d9aa57b7..1fcd90c2 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,8 @@ +TagLib 1.11 (??? ??, 2015) ========================== +1.11 BETA: + * New API for creating FileRef from IOStream. * Added support for ID3v2 PCST and WFED frames. * Added support for pictures in XiphComment. @@ -8,6 +11,7 @@ * Added alternative functions to XiphComment::removeField(). * Added BUILD_BINDINGS build option. * Added ENABLE_CCACHE build option. + * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. * Better handling of duplicate ID3v2 tags in all kinds of files. * Better handling of duplicate tag chunks in WAV files. * Better handling of duplicate tag chunks in AIFF files. @@ -25,7 +29,6 @@ * Fixed possible file corruptions when saving WavPack files. * Fixed updating the comment field of Vorbis comments. * Fixed reading date and time in ID3v2.3 tags. - * Replaced ENABLE_STATIC build option with BUILD_SHARED_LIBS. * Marked ByteVector::null and ByteVector::isNull() deprecated. * Marked String::null and ByteVector::isNull() deprecated. * Marked XiphComment::removeField() deprecated. From 758b7e39ce63884e6e1987dfe2fddc02cd317ca9 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 28 Jan 2016 13:17:56 +0900 Subject: [PATCH 09/18] Update the version to v1.11. --- CMakeLists.txt | 6 +++--- taglib/toolkit/taglib.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 67442291..298ee7e3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -89,9 +89,9 @@ endif() # 2. If any interfaces have been added, removed, or changed since the last update, increment current, and set revision to 0. # 3. If any interfaces have been added since the last public release, then increment age. # 4. If any interfaces have been removed since the last public release, then set age to 0. -set(TAGLIB_SOVERSION_CURRENT 16) -set(TAGLIB_SOVERSION_REVISION 1) -set(TAGLIB_SOVERSION_AGE 15) +set(TAGLIB_SOVERSION_CURRENT 17) +set(TAGLIB_SOVERSION_REVISION 0) +set(TAGLIB_SOVERSION_AGE 16) math(EXPR TAGLIB_SOVERSION_MAJOR "${TAGLIB_SOVERSION_CURRENT} - ${TAGLIB_SOVERSION_AGE}") math(EXPR TAGLIB_SOVERSION_MINOR "${TAGLIB_SOVERSION_AGE}") diff --git a/taglib/toolkit/taglib.h b/taglib/toolkit/taglib.h index 4534f186..5a1204dc 100644 --- a/taglib/toolkit/taglib.h +++ b/taglib/toolkit/taglib.h @@ -29,7 +29,7 @@ #include "taglib_config.h" #define TAGLIB_MAJOR_VERSION 1 -#define TAGLIB_MINOR_VERSION 10 +#define TAGLIB_MINOR_VERSION 11 #define TAGLIB_PATCH_VERSION 0 #if defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 1)) From c2cb9ab8b0cb7a2c9d83d5e5de6ce103b721206c Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Thu, 28 Jan 2016 13:31:22 +0900 Subject: [PATCH 10/18] Update NEWS with the specific release date. --- NEWS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 1fcd90c2..6bba6a09 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -TagLib 1.11 (??? ??, 2015) +TagLib 1.11 (Jan 30, 2016) ========================== 1.11 BETA: From 9976155aa94be08eb0136399f17bf48108929bd5 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 30 Jan 2016 00:51:28 +0900 Subject: [PATCH 11/18] Ignore 'fact' chunk of WAV files if their format is PCM. TagLib reports wrong length of some PCM files with a 'fact' chunk. --- taglib/riff/wav/wavproperties.cpp | 2 +- tests/data/pcm_with_fact_chunk.wav | Bin 0 -> 14756 bytes tests/test_wav.cpp | 17 +++++++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 tests/data/pcm_with_fact_chunk.wav diff --git a/taglib/riff/wav/wavproperties.cpp b/taglib/riff/wav/wavproperties.cpp index df7cdbae..181e3a6c 100644 --- a/taglib/riff/wav/wavproperties.cpp +++ b/taglib/riff/wav/wavproperties.cpp @@ -192,7 +192,7 @@ void RIFF::WAV::Properties::read(File *file) d->sampleRate = data.toUInt(4, false); d->bitsPerSample = data.toShort(14, false); - if(totalSamples > 0) + if(d->format != FORMAT_PCM) d->sampleFrames = totalSamples; else if(d->channels > 0 && d->bitsPerSample > 0) d->sampleFrames = streamLength / (d->channels * ((d->bitsPerSample + 7) / 8)); diff --git a/tests/data/pcm_with_fact_chunk.wav b/tests/data/pcm_with_fact_chunk.wav new file mode 100644 index 0000000000000000000000000000000000000000..a6dc1d6c58bdbd08730cc16c71a940d581910765 GIT binary patch literal 14756 zcmeIup$)=N07cOUt*0ZfMr8#*X@VjI^be$}xIYep@fd^d!MXaXbD2wdkI8+yot9TS z4oP;|=i4QpL(*r+v$nc!j|dPTK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk q1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009F3EU?r^>olength()); + CPPUNIT_ASSERT_EQUAL(3, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(3675, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(32, f.audioProperties()->bitrate()); + CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels()); + CPPUNIT_ASSERT_EQUAL(1000, f.audioProperties()->sampleRate()); + CPPUNIT_ASSERT_EQUAL(16, f.audioProperties()->bitsPerSample()); + CPPUNIT_ASSERT_EQUAL(16, f.audioProperties()->sampleWidth()); + CPPUNIT_ASSERT_EQUAL(3675U, f.audioProperties()->sampleFrames()); + CPPUNIT_ASSERT_EQUAL(1, f.audioProperties()->format()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestWAV); From 8afbf6c92a4a17d512bfb5f49db56a6416e71927 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Sat, 30 Jan 2016 11:13:32 +0900 Subject: [PATCH 12/18] Update NEWS. --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 6bba6a09..1fe13d57 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ TagLib 1.11 (Jan 30, 2016) ========================== + * Better handling of PCM WAV files with a 'fact' chunk. + 1.11 BETA: * New API for creating FileRef from IOStream. From 92a1a00624bf78b620d9b7009965892ff5a5282f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 1 Feb 2016 22:19:43 +0900 Subject: [PATCH 13/18] APE item keys should be ASCII between 0x20 and 0x7E, not UTF-8. --- NEWS | 3 ++- taglib/ape/apeitem.cpp | 18 +++++++++++------- taglib/ape/apetag.cpp | 17 ++++++++++++++++- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index 1fe13d57..59c28d10 100644 --- a/NEWS +++ b/NEWS @@ -2,7 +2,8 @@ TagLib 1.11 (Jan 30, 2016) ========================== * Better handling of PCM WAV files with a 'fact' chunk. - + * Better handling of corrupted APE tags. + 1.11 BETA: * New API for creating FileRef from IOStream. diff --git a/taglib/ape/apeitem.cpp b/taglib/ape/apeitem.cpp index ebfb13df..86d1af97 100644 --- a/taglib/ape/apeitem.cpp +++ b/taglib/ape/apeitem.cpp @@ -34,7 +34,9 @@ using namespace APE; class APE::Item::ItemPrivate { public: - ItemPrivate() : type(Text), readOnly(false) {} + ItemPrivate() : + type(Text), + readOnly(false) {} Item::ItemTypes type; String key; @@ -74,8 +76,9 @@ APE::Item::Item(const String &key, const ByteVector &value, bool binary) : d->type = Binary; d->value = value; } - else + else { d->text.append(value); + } } APE::Item::Item(const Item &item) : @@ -181,9 +184,8 @@ void APE::Item::appendValues(const StringList &values) int APE::Item::size() const { - // SFB: Why is d->key.size() used when size() returns the length in UniChars and not UTF-8? - int result = 8 + d->key.size() /* d->key.data(String::UTF8).size() */ + 1; - switch (d->type) { + int result = 8 + d->key.size() + 1; + switch(d->type) { case Text: if(!d->text.isEmpty()) { StringList::ConstIterator it = d->text.begin(); @@ -250,7 +252,9 @@ void APE::Item::parse(const ByteVector &data) const unsigned int valueLength = data.toUInt(0, false); const unsigned int flags = data.toUInt(4, false); - d->key = String(data.mid(8), String::UTF8); + // An item key can contain ASCII characters from 0x20 up to 0x7E, not UTF-8. + + d->key = String(data.mid(8), String::Latin1); const ByteVector value = data.mid(8 + d->key.size() + 1, valueLength); @@ -288,7 +292,7 @@ ByteVector APE::Item::render() const data.append(ByteVector::fromUInt(value.size(), false)); data.append(ByteVector::fromUInt(flags, false)); - data.append(d->key.data(String::UTF8)); + data.append(d->key.data(String::Latin1)); data.append(ByteVector('\0')); data.append(value); diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index 4b020413..6109d139 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -43,6 +43,20 @@ using namespace TagLib; using namespace APE; +namespace +{ + inline bool isValidItemKey(const String &key) + { + for(String::ConstIterator it = key.begin(); it != key.end(); ++it) { + const int c = static_cast(*it); + if(c < 0x20 || 0x7E < c) + return false; + } + + return true; + } +} + class APE::Tag::TagPrivate { public: @@ -381,7 +395,8 @@ void APE::Tag::parse(const ByteVector &data) APE::Item item; item.parse(data.mid(pos)); - d->itemListMap.insert(item.key().upper(), item); + if(isValidItemKey(item.key())) + d->itemListMap.insert(item.key().upper(), item); pos += item.size(); } From 5350bc85010b429781ec3fb3cb1702c3b2db5e2f Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Mon, 1 Feb 2016 22:46:08 +0900 Subject: [PATCH 14/18] Oops! We already have a function to check APE item keys. --- taglib/ape/apetag.cpp | 50 ++++++++++++++++++++++--------------------- tests/test_apetag.cpp | 5 +++++ 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index 6109d139..fd4a33b7 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include "apetag.h" #include "apefooter.h" @@ -43,20 +44,6 @@ using namespace TagLib; using namespace APE; -namespace -{ - inline bool isValidItemKey(const String &key) - { - for(String::ConstIterator it = key.begin(); it != key.end(); ++it) { - const int c = static_cast(*it); - if(c < 0x20 || 0x7E < c) - return false; - } - - return true; - } -} - class APE::Tag::TagPrivate { public: @@ -283,15 +270,20 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) bool APE::Tag::checkKey(const String &key) { if(key.size() < 2 || key.size() > 16) + return false; + + for(String::ConstIterator it = key.begin(); it != key.end(); it++) { + // only allow printable ASCII including space (32..126) + const int c = static_cast(*it); + if(c < 32 || c > 126) return false; - for(String::ConstIterator it = key.begin(); it != key.end(); it++) - // only allow printable ASCII including space (32..127) - if (*it < 32 || *it >= 128) - return false; - String upperKey = key.upper(); - if (upperKey=="ID3" || upperKey=="TAG" || upperKey=="OGGS" || upperKey=="MP+") - return false; - return true; + } + + const String upperKey = key.upper(); + if(upperKey == "ID3" || upperKey == "TAG" || upperKey == "OGGS" || upperKey == "MP+") + return false; + + return true; } APE::Footer *APE::Tag::footer() const @@ -311,9 +303,15 @@ void APE::Tag::removeItem(const String &key) void APE::Tag::addValue(const String &key, const String &value, bool replace) { + if(!checkKey(key)) { + debug("APE::Tag::addValue() - Couldn't add a value due to an invalid key."); + return; + } + if(replace) removeItem(key); - if(!key.isEmpty() && !value.isEmpty()) { + + if(!value.isEmpty()) { if(!replace && d->itemListMap.contains(key)) { // Text items may contain more than one value if(APE::Item::Text == d->itemListMap.begin()->second.type()) @@ -395,8 +393,12 @@ void APE::Tag::parse(const ByteVector &data) APE::Item item; item.parse(data.mid(pos)); - if(isValidItemKey(item.key())) + if(checkKey(item.key())) { d->itemListMap.insert(item.key().upper(), item); + } + else { + debug("APE::Tag::parse() - Skipped an item due to an invalid key."); + } pos += item.size(); } diff --git a/tests/test_apetag.cpp b/tests/test_apetag.cpp index 71d0be26..b45b516c 100644 --- a/tests/test_apetag.cpp +++ b/tests/test_apetag.cpp @@ -97,6 +97,11 @@ public: CPPUNIT_ASSERT_EQUAL((unsigned int)2, unsuccessful.size()); CPPUNIT_ASSERT(unsuccessful.contains("A")); CPPUNIT_ASSERT(unsuccessful.contains("MP+")); + + CPPUNIT_ASSERT_EQUAL((unsigned int)2, tag.itemListMap().size()); + tag.addValue("VALID KEY", "Test Value 1"); + tag.addValue("INVALID KEY \x7f", "Test Value 2"); + CPPUNIT_ASSERT_EQUAL((unsigned int)3, tag.itemListMap().size()); } void testTextBinary() From 013fbbf22c7b3bd05ebf0c9346e6ba2f52762ee4 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 2 Feb 2016 00:42:08 +0900 Subject: [PATCH 15/18] APE::Tag::addValue() may append a string to non-text items. --- taglib/ape/apetag.cpp | 44 ++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index fd4a33b7..c13262d5 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -303,39 +303,41 @@ void APE::Tag::removeItem(const String &key) void APE::Tag::addValue(const String &key, const String &value, bool replace) { - if(!checkKey(key)) { - debug("APE::Tag::addValue() - Couldn't add a value due to an invalid key."); - return; - } - if(replace) removeItem(key); - if(!value.isEmpty()) { - if(!replace && d->itemListMap.contains(key)) { - // Text items may contain more than one value - if(APE::Item::Text == d->itemListMap.begin()->second.type()) - d->itemListMap[key.upper()].appendValue(value); - // Binary or locator items may have only one value - else - setItem(key, Item(key, value)); - } - else - setItem(key, Item(key, value)); - } + if(value.isEmpty()) + return; + + // Text items may contain more than one value. + // Binary or locator items may have only one value, hence always replaced. + + ItemListMap::Iterator it = d->itemListMap.find(key.upper()); + + if(it != d->itemListMap.end() && it->second.type() == Item::Text) + it->second.appendValue(value); + else + setItem(key, Item(key, value)); } void APE::Tag::setData(const String &key, const ByteVector &value) { removeItem(key); - if(!key.isEmpty() && !value.isEmpty()) - setItem(key, Item(key, value, true)); + + if(value.isEmpty()) + return; + + setItem(key, Item(key, value, true)); } void APE::Tag::setItem(const String &key, const Item &item) { - if(!key.isEmpty()) - d->itemListMap.insert(key.upper(), item); + if(!checkKey(key)) { + debug("APE::Tag::setItem() - Couldn't set an item due to an invalid key."); + return; + } + + d->itemListMap[key.upper()] = item; } bool APE::Tag::isEmpty() const From c04b24a2f5410ee2f3cb7dc2363ec11ccd19473b Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 3 Feb 2016 01:05:56 +0900 Subject: [PATCH 16/18] More efficient handling of broken APE item keys. This also improves the performance when handling intact APE items. --- taglib/ape/apeitem.cpp | 3 ++- taglib/ape/apetag.cpp | 57 +++++++++++++++++++++++++++++------------ taglib/toolkit/tutils.h | 17 ++++++++++++ 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/taglib/ape/apeitem.cpp b/taglib/ape/apeitem.cpp index 86d1af97..45f22da4 100644 --- a/taglib/ape/apeitem.cpp +++ b/taglib/ape/apeitem.cpp @@ -253,8 +253,9 @@ void APE::Item::parse(const ByteVector &data) const unsigned int flags = data.toUInt(4, false); // An item key can contain ASCII characters from 0x20 up to 0x7E, not UTF-8. + // We assume that the validity of the given key has been checked. - d->key = String(data.mid(8), String::Latin1); + d->key = String(&data[8], String::Latin1); const ByteVector value = data.mid(8 + d->key.size() + 1, valueLength); diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index c13262d5..9c8d8217 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -36,14 +36,42 @@ #include #include #include +#include #include "apetag.h" #include "apefooter.h" #include "apeitem.h" + using namespace TagLib; using namespace APE; +namespace +{ + inline bool isKeyValid(const char *key, size_t length) + { + const char *invalidKeys[] = { "ID3", "TAG", "OGGS", "MP+", 0 }; + + if(length < 2 || length > 16) + return false; + + // only allow printable ASCII including space (32..126) + + for(const char *p = key; p < key + length; ++p) { + const int c = static_cast(*p); + if(c < 32 || c > 126) + return false; + } + + for(size_t i = 0; invalidKeys[i] != 0; ++i) { + if(Utils::equalsIgnoreCase(key, invalidKeys[i])) + return false; + } + + return true; + } +} + class APE::Tag::TagPrivate { public: @@ -269,21 +297,11 @@ PropertyMap APE::Tag::setProperties(const PropertyMap &origProps) bool APE::Tag::checkKey(const String &key) { - if(key.size() < 2 || key.size() > 16) + if(!key.isLatin1()) return false; - for(String::ConstIterator it = key.begin(); it != key.end(); it++) { - // only allow printable ASCII including space (32..126) - const int c = static_cast(*it); - if(c < 32 || c > 126) - return false; - } - - const String upperKey = key.upper(); - if(upperKey == "ID3" || upperKey == "TAG" || upperKey == "OGGS" || upperKey == "MP+") - return false; - - return true; + const std::string data = key.to8Bit(false); + return isKeyValid(data.c_str(), data.size()); } APE::Footer *APE::Tag::footer() const @@ -392,16 +410,21 @@ void APE::Tag::parse(const ByteVector &data) unsigned int pos = 0; for(unsigned int i = 0; i < d->footer.itemCount() && pos <= data.size() - 11; i++) { - APE::Item item; - item.parse(data.mid(pos)); - if(checkKey(item.key())) { + const char *key = &data[pos + 8]; + const size_t keyLength = ::strnlen(key, data.size() - pos - 8); + const size_t valLegnth = data.toUInt(pos, false); + + if(isKeyValid(key, keyLength)){ + APE::Item item; + item.parse(data.mid(pos)); + d->itemListMap.insert(item.key().upper(), item); } else { debug("APE::Tag::parse() - Skipped an item due to an invalid key."); } - pos += item.size(); + pos += static_cast(keyLength + valLegnth + 9); } } diff --git a/taglib/toolkit/tutils.h b/taglib/toolkit/tutils.h index 5b5bfd6e..9a81fd8b 100644 --- a/taglib/toolkit/tutils.h +++ b/taglib/toolkit/tutils.h @@ -219,6 +219,23 @@ namespace TagLib return String(); } + /*! + * Returns whether the two strings s1 and s2 are equal, ignoring the case of + * the characters. + * + * We took the trouble to define this one here, since there are some + * incompatible variations of case insensitive strcmp(). + */ + inline bool equalsIgnoreCase(const char *s1, const char *s2) + { + while(*s1 != '\0' && *s2 != '\0' && ::tolower(*s1) == ::tolower(*s2)) { + s1++; + s2++; + } + + return (*s1 == '\0' && *s2 == '\0'); + } + /*! * The types of byte order of the running system. */ From 24575aab233a9202c24a892a6d6cd40d287f71b3 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 3 Feb 2016 20:21:04 +0900 Subject: [PATCH 17/18] Remove strnlen() since some compilers lack it. --- taglib/ape/apetag.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/taglib/ape/apetag.cpp b/taglib/ape/apetag.cpp index 9c8d8217..d49d0700 100644 --- a/taglib/ape/apetag.cpp +++ b/taglib/ape/apetag.cpp @@ -42,7 +42,6 @@ #include "apefooter.h" #include "apeitem.h" - using namespace TagLib; using namespace APE; @@ -411,11 +410,16 @@ void APE::Tag::parse(const ByteVector &data) for(unsigned int i = 0; i < d->footer.itemCount() && pos <= data.size() - 11; i++) { - const char *key = &data[pos + 8]; - const size_t keyLength = ::strnlen(key, data.size() - pos - 8); - const size_t valLegnth = data.toUInt(pos, false); + const int nullPos = data.find('\0', pos + 8); + if(nullPos < 0) { + debug("APE::Tag::parse() - Couldn't find a key/value separator. Stopped parsing."); + return; + } - if(isKeyValid(key, keyLength)){ + const unsigned int keyLength = nullPos - pos - 8; + const unsigned int valLegnth = data.toUInt(pos, false); + + if(isKeyValid(&data[pos + 8], keyLength)){ APE::Item item; item.parse(data.mid(pos)); @@ -425,6 +429,6 @@ void APE::Tag::parse(const ByteVector &data) debug("APE::Tag::parse() - Skipped an item due to an invalid key."); } - pos += static_cast(keyLength + valLegnth + 9); + pos += keyLength + valLegnth + 9; } } From a8ecbbaef4e8a33bdff37f4144e4aeed2cc396dd Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Wed, 3 Feb 2016 20:33:13 +0900 Subject: [PATCH 18/18] Update NEWS. --- NEWS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 59c28d10..787f2d3b 100644 --- a/NEWS +++ b/NEWS @@ -3,7 +3,8 @@ TagLib 1.11 (Jan 30, 2016) * Better handling of PCM WAV files with a 'fact' chunk. * Better handling of corrupted APE tags. - + * Several smaller bug fixes and performance improvements. + 1.11 BETA: * New API for creating FileRef from IOStream.