From 081d6eaf7656680e477244ba863afc930c09d6c0 Mon Sep 17 00:00:00 2001 From: Tsuda Kageyu Date: Tue, 22 Dec 2015 14:54:07 +0900 Subject: [PATCH] More robust checks for invalid MPEG frame headers. (again) --- taglib/mpeg/mpegheader.cpp | 25 +++++++++++++++++++++++-- taglib/mpeg/mpegheader.h | 10 +++++++++- taglib/mpeg/mpegproperties.cpp | 27 ++++++++++++++++++--------- tests/data/invalid-frames.mp3 | Bin 8192 -> 8164 bytes tests/data/invalid-frames2.mp3 | Bin 0 -> 7898 bytes tests/test_mpeg.cpp | 31 ++++++++++++++++++++----------- 6 files changed, 70 insertions(+), 23 deletions(-) create mode 100644 tests/data/invalid-frames2.mp3 diff --git a/taglib/mpeg/mpegheader.cpp b/taglib/mpeg/mpegheader.cpp index 01b37705..73d2196b 100644 --- a/taglib/mpeg/mpegheader.cpp +++ b/taglib/mpeg/mpegheader.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -70,7 +71,13 @@ public: MPEG::Header::Header(const ByteVector &data) : d(new HeaderPrivate()) { - parse(data); + debug("MPEG::Header::Header() - This constructor is no longer used."); +} + +MPEG::Header::Header(File *file, long offset) : + d(new HeaderPrivate()) +{ + parse(file, offset); } MPEG::Header::Header(const Header &h) : @@ -162,8 +169,11 @@ MPEG::Header &MPEG::Header::operator=(const Header &h) // private members //////////////////////////////////////////////////////////////////////////////// -void MPEG::Header::parse(const ByteVector &data) +void MPEG::Header::parse(File *file, long offset) { + file->seek(offset); + const ByteVector data = file->readBlock(4); + if(data.size() < 4) { debug("MPEG::Header::parse() -- data is too short for an MPEG frame header."); return; @@ -288,6 +298,17 @@ void MPEG::Header::parse(const ByteVector &data) if(d->isPadded) d->frameLength += paddingSize[layerIndex]; + // Check if the frame length has been calculated correctly, or the next frame + // header is near the end of this frame. + + file->seek(offset + d->frameLength); + const ByteVector nextSynch = file->readBlock(2); + + if(nextSynch.size() < 2 || !firstSyncByte(nextSynch[0]) || !secondSynchByte(nextSynch[1])) { + debug("MPEG::Header::parse() -- Frame length seems to be wrong."); + return; + } + // Now that we're done parsing, set this to be a valid frame. d->isValid = true; diff --git a/taglib/mpeg/mpegheader.h b/taglib/mpeg/mpegheader.h index a55cac09..cd77d37a 100644 --- a/taglib/mpeg/mpegheader.h +++ b/taglib/mpeg/mpegheader.h @@ -31,6 +31,7 @@ namespace TagLib { class ByteVector; + class File; namespace MPEG { @@ -48,9 +49,16 @@ namespace TagLib { public: /*! * Parses an MPEG header based on \a data. + * + * \deprecated */ Header(const ByteVector &data); + /*! + * Parses an MPEG header based on \a file and \a offset. + */ + Header(File *file, long offset); + /*! * Does a shallow copy of \a h. */ @@ -155,7 +163,7 @@ namespace TagLib { Header &operator=(const Header &h); private: - void parse(const ByteVector &data); + void parse(File *file, long offset); class HeaderPrivate; HeaderPrivate *d; diff --git a/taglib/mpeg/mpegproperties.cpp b/taglib/mpeg/mpegproperties.cpp index 989c9065..8507f38d 100644 --- a/taglib/mpeg/mpegproperties.cpp +++ b/taglib/mpeg/mpegproperties.cpp @@ -163,8 +163,7 @@ void MPEG::Properties::read(File *file) return; } - file->seek(firstFrameOffset); - Header firstHeader(file->readBlock(4)); + Header firstHeader(file, firstFrameOffset); while(!firstHeader.isValid()) { firstFrameOffset = file->nextFrameOffset(firstFrameOffset + 1); @@ -173,8 +172,7 @@ void MPEG::Properties::read(File *file) return; } - file->seek(firstFrameOffset); - firstHeader = Header(file->readBlock(4)); + firstHeader = Header(file, firstFrameOffset); } // Check for a VBR header that will help us in gathering information about a @@ -207,14 +205,25 @@ void MPEG::Properties::read(File *file) d->bitrate = firstHeader.bitrate(); - long streamLength = file->length() - firstFrameOffset; + long lastFrameOffset = file->lastFrameOffset(); + if(lastFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find an MPEG frame in the stream."); + return; + } - if(file->hasID3v1Tag()) - streamLength -= 128; + Header lastHeader(file, lastFrameOffset); - if(file->hasAPETag()) - streamLength -= file->APETag()->footer()->completeTagSize(); + while(!lastHeader.isValid()) { + lastFrameOffset = file->previousFrameOffset(lastFrameOffset); + if(lastFrameOffset < 0) { + debug("MPEG::Properties::read() -- Could not find a valid last MPEG frame in the stream."); + return; + } + lastHeader = Header(file, lastFrameOffset); + } + + const long streamLength = lastFrameOffset - firstFrameOffset + lastHeader.frameLength() * 2; if(streamLength > 0) d->length = static_cast(streamLength * 8.0 / d->bitrate + 0.5); } diff --git a/tests/data/invalid-frames.mp3 b/tests/data/invalid-frames.mp3 index a6dc1117c4bd168901f24e866c3595f798d8224a..c076712c029bb734f1627817660a4b2a4fdf8925 100644 GIT binary patch delta 7 OcmZp0cw)cdi97%fZUdJ9 delta 36 scmaE2-{7#}iM-7J--~k2H?Yd4M_lIj%JOH?wo2@f77?A9y=JdA05Yo%{r~^~ diff --git a/tests/data/invalid-frames2.mp3 b/tests/data/invalid-frames2.mp3 new file mode 100644 index 0000000000000000000000000000000000000000..01976fc54f8da0d764c156a20156998f0b829991 GIT binary patch literal 7898 zcmXZeWmMA-)Cce%8>2S5#t0cXdUOf{28?us2#Pp5L_$KqV8G~bq@!eXScHY5jt&7S z34@Rhl~O>7=kN18_su=`ocrpY&$;KD@6i3_*b0Ven1II)GT`O!D)`n45s2P4(x?Aw z2O5E%iTJhOq(X~lVzc4&0J96ApekU%4tb4c%sl_UOQ)y5<%8awocx{rd8-X$oDRK5 z4}Td6w~-(au4+SIxt>Bw0A0$k;r}oHf1CO7^-XB$(C)kcn@`|CH<##3QoAV=PeLL+ z;ARvHW{?*`GYIjYZg?&sU2;ojd4a20yEvAJYYo%E!KuVhCjvU667CAHq4X z;KAlk1Ybrx?MxpAe}Kq3mF8|`Lt?BVA<|$sG&|((*AdN7Y9Kvb1B#yp8>YhI7?*$U z+$udkzzWcaykr##Ohd2=1Q10DjGXQSuf6h8xfN1OKnKxQ8euLB6OoW;Fr^oqLzgbf z?{iAwWr{8It1h^W2|$2`F~fOu4*@J7Fi#A+6o(>s!~ynOvh1gaWi=DQwGLsdFxo&0 z*tda|V;;?V`nQSxl*sIp-26OsU2hw1>G}bQK|edLk-{fDrwXSR#cOh|b9ZI|LpFSlyQD zJcdA|XP}EEDi9eNw6`SUXbrlwZUZ(m^W8_TWG$6I=CUlvF%^Xv?#hq+MNHJPC^qKk zqC2TiKAEyXW-8BrxV5G4*hz1WxsjA8T{hKw`H5@lu)G!S7X#c~wb?`nPb7LdzdC44)z zIlhET#it}aGq^JKZGUyqx?b!0`{nDoH398!?t};Bu+}-s@Vok%zd5;{W2Sq36EC_{ zA$?~1%I6I?&4`$B2wy50fc=Ok4_$K}lrez~`q{G%2}F(!ucVhV__kvNtw3#*_Ai3N zo$ZwtR(9Cp4tYK+HY2qz2ls(1SQ1T5md$ZcwRgN@-kzI3y_ldv-J(aLS6$l0>|(zO z#F;Idu~T3S@=#^erj{d-hlqeM128sW7=swM0vf}7c1&0?Yn#VB*1FK&YLovmW^TVA z@!4;#GOKxYF@C2{rx!P6T%|8AUd;YDUjN;lYh#kQU?a(W2p+=(g+XL|Liz*B25RKa z7FfO-v8AMPs+EB(B1)Z6vpB!`FnzJP4Y+N{JrDXhTd}r! zceO4L_m5l*k%`O9*!m_BIQ5J4yIF|ia(SoY`~_ga^Dj4UwywtLF41u^R|4SddrGQj zCE^Xi-KzZ#;26n2%7JKARMvOp2RysXVzR0?B<#6qtWUXAaiUWk9&vJU4AmS82oCC{ zIbW-;iX0@_Y67Z~4IfJrcM^@g#787C(Q8K&#jQ~oZo_tW!=a2&MJ=8segJ6|t?DR0 zjxcRUwj1&(6NQ9G?RPzJArQvME6gf%(7DufI6Ym(`-})1S^*(?w-^#x5`>2mX&-}(Ax3hAz_w_0@y>Y6lrhxn@NoMJG0G%{Va0Ulgiz`l!p$vP$ zN3n=aBwU3KF3U&>r%aWDpTj;3Pl|Jpb6x=O2Xymw%y|e|Cd2$VDFJuplyveL zxE@^_DOiB>M-oBZ^tt=j60t(>Otv{z(^Lg71-_;j0e48dp*G;FW2gw}b2_K;L#(r< zHHK{H<112=3VIVkORAh~8sM^|?FO$HbjiG$FxxP%WEw?2!6b6QoQ&MkU;E^6>E2Zc=03*22 z7nl$AHxcau1CA1jY82`a@r6)3U57LRM9VqdWvXa0*>Qm3vzPCE^{-`liRn@j5zs{- zR$Ua=Pa~?%m-bkxcb9;V3>& zt~wM`XB;#0&1K8N0{xt(@*U$owNuT}dv0_75z8evcC?^+bAlJ7ippR04mX^JTvAKd zi}o8FPqfPo%U61vAD;N<@Z?g#4=az`Va8&6VWS;i09qn;Kd2yr*+5CE z0iGWTq1Q{T+I3V2iz95RK^Y^>H(?($tdZ;05au!voCzcZ7gON@CCVSbSi>6}v2BQ} zPb)@~yp9v4F?ixMTX+(3foZ8fkE`K=JGkq-{N2=)2-PQ<{)JaFA8TK13_BA&_`~4z zA32@$PEnH&MTLf2@wEaA!R?(K&iuG*bMGx+$koAC0l$NjN!?dzCU@UDq&xyi2LgZ# zu}4lk7R>r>TJ6!TSuxe9Osi3}3a51(hEmPgB95j`IZ+&EfK~Q zlRswN#74%Q5gu1}R^CUX7`W`%lLK;xfueVSd&TrS?VAhk;p!$3oL3o!wh#aa2t#Ba z1XmS}`jeS9s{&o*JQ4J_T_5>kn7l?(bu)}E-|o#F{hSOFa-CkB)cJAtLjT*X`0;Z& zyw_Hq`uw~(4fb@p+>}t=ofAaI2MNOCdld9$I;DTp`>Nc)U#q#Z^59iS>$!XA=Aar3 z@aPDfcK3SEw27!Yi zqQkwBs#3LE20{2QSlU3Td^uK(vq|a={*|k`;&nMDVU1>F(h-dLfarH#?~6N#C^3HF ztD5h_3`f7F<^V^FCvg2^cI2;5Cdrynh$58wC9@abH23n7il+ZitQFWxkk54AFY^Ts z4FfU(Fk?%!_L}px4nQ_9WNvFS0L4vQ{ z{*k-H)(KO^rxjWR6LEj+gBX0z)#+MTH#j=)Ny)H2=Nq}t&v&N!?X#a2x1(}E!Pijb ztPh#42gl5+A1=Q$aP2o!E{(c(Tz)XWJDs{z@>cQ=OZCBw%Ilw}&-+6Eavb%|gxs9U zP~+1}Yraul9H1P$!2d+Y!u>tGzb^9u1@4AfOHQLi){aLdv%qmW)>ZDOs$}m=P&_U8 zOBHpIfj-hp%^hv>XY;&S$-oz{3JsB0bVsCgxpw}umE5b}T)(C2bZdj@vhX^VE7G?* zhE<(Sp7FvhEX)=^K4tsG*rDzeP;(=BVq6LEr~v@zT%Sq$Z0?G5+7a@qki8@mzyh*t zqZ0g^L1Ie&G73g(jGDAs#aTGhrJ)vNIn}77XM+){@pJRZo0_6qxs-(i3P3{6+pEDifEPU5bUZ4*!qzCh zf=wq+ZL{)s2H+Xwuc;0!-O@%s-&b!oRz$yN?5FJ}-^=Fq}UW7-oEd#waiggVvy-3g{# zVt$?^Afd}sN*;dqK`b1`WO()AYI3^9wW9(Sh7UMTzZT|ae&?&2Ufb%1t3CtAcx4B! z-)G#9e(Bvp-;AmW)qZBBkG*Z$a>aD`VdJkmvjvCUWvrN2kYCrk&XM*wfr;78+x<7cE-uRnj(RKNA|xK7fQuVp{t5Z zHaIYYqf2m0wN)>{Oq$8gr?Q(?>77IcL>rKJrw)j%cB%G<;J3AL@`7x-Nf#xL@LPUa z$@bRh-s;>==VLt|WcEU2_kHH#UDw-n-t3L{>rLv0so{9CJCxW-#oEPVQ!IV>sr>et zwJ&70AJuS}K5_i`S$o^8?BMiPMEnNZqbA<}q|CL&4g@ic8ClU@&gS-0k@VuE6&@W{ z4Q0RqQb?Xk=ad5vDZRYyEftlUp)6P6C;zR9PfR(|`%IYqOvZe`s(Dlu`q=MU0B^qk zah-aRPTaRivBc?-PNt6{XR!%dGpK{FFmEIFkBuccroG266wLxmLiHtdKgB)Oo##)U zrveZgs2E+cRFEpL;H!;5K^ck69TY=Rnj6=8l0)qXhhh4>w0j-3qj&KucSd4d>hJBx zlr&B0s!XFL{*N7e&Hm3)KS%ng2g+sgOV#d(lB3O7z0kLX1>>G^lP|j6O|sZw@fO+F zP_`|(*zsJkW=K@O*#l9of6(3i274u~51c5WhWw>51~v#f%auhBFaOfu(p35C`|bj>KhE~CCjjT_xvv-YD`nvP6ZN80Hy)^B2k^^jbE1D5EryDe4+(n*M@m$D7$_m z9@u^UlXva8)G!`2HdotRe|*;QLZ_Vv{7cg_-376GAT3WrIuZOKPV~mrCLX=dtqa=s zE22s&%>1XiPki2Ye2ttb9XN;$Th*6!nd@X`_0U^Q%i!JX!oy(cNSF5=z?hWKKXOBC zi)?s1!2mvv*t&k>|H!d#|3_}_!6;N(#=4a^66H;!bK5ZIo zT<3MESqdfc^0>s8E5=@T6uMl?3yW$tWq(N{^eK-307UKZ<0j?~;uO)(BxXEAl`~ia zspz$UuivQV6e>9p%c31Fuame+I_e;=TA^;4#@5xLJdCx!q{^eNv0wC9K zn}!z{Fk^{1-!YVWL}7$yn{L)Tn}vv5ht+~U;p4|qRvrgg(~Fo-SE*= zItg1ilGdqEXY@r6dNKgY85xR?E7F|E^{;4V$a9r7l#Uj340oxW_oPb z5@jwvL<>cYx_wb5N@=?l5f(psntTdK+p&G_GFTB@{o}>DlE&gS%PMYxrTpM*4 zt!3kMQ4Jx13-)QVEe0`g@fEa65DeFH*Mm|t{9zh}jo?yFA&`zxBs9VVKtsZWoUu`_ zJ`9U8MVTQuuP-Rh1PENZBq0`Zfv{LEEOEzJ?u|xE?6+lTd8SrLU%HZiy?m~e&zZBm zk{5Hyyzk@9dj1WPmOZ=uJ{Saj)#bW`}f{>i2t_ zEDsPE8sMrLobg6bwe5(f9t5{2u+QL~NUVQ}btnFJIFZu}fw9 zEoYTYaig!r*wv}NVxDh3*)6x`8}`hjNpT19OGz*%L}6{+VZG(Wyt@2x_3zI$4@>MJ z$CJM&Laqool4ia38E4y?Td9uGOL*?NZo`IDj$F^4WpE{LdTkwgilZ0eCe55+A=Z)G z!6@yFCUo|hsFbI_%;6z*z&N>azsj+#{wfh-Ov6a;$7qsHmx!Xgp>P>XeR6LBsPd10 z@8+YIPlatYJrvb-zpcWIX+f66wp&AcLu z`&lo`hPaGx`5JR7B)W&VZJCsaxgK?O`nY7jJlo7n&x==;4rTy&+cwO~uVdMps8`7~<{^f#`_%FUtweTICx@G2_jQiElc ztMT9HHj)z827`6w%wBOnkaL_;FJ-R)Qsl{9|o!MM)G{i;cLlMIR*2w*UCyG zp~1%5I-GXdiCYr&YJ)>+xU+xrFI2jW2M(@A8%LCWZ@Zo0O8rRjrN-%L#g)l;3Ux_G zdL5`pR|jaH7{td;-PL(n33gD1UuUhoH`M7s1Z=k0=rq&Rv0mBLPZ`|B6Mr!91Sp_- zIbT62aH_0>47$tJhur)8b&Ti32H zkbm{9LSE6c?_?8wtzq&!sE~P6^NU;QJ@F&XLWTf0$*nhNl38wmWx2jSw_4K~aESyP zj|=OpsJba>&}qybJm~+N@kVBSfjR;w->F*fNt5Ol17VA*i9%2-E3J6@__Gy|txOxW zxSFC)g^0awtgo_$<$8lSP?mDXBw0YLX~q&1ViSMxw)je4L@1@TXA>_ZY$ z1q%~~{nn$>grz*$D9;qbo+%G~=r+0t&!a$;D7|`mGaOJ@0Dy@5M{bdAk<-wwC%u}(`H5Qf0@;N&j)xlU z#L*>1hv7C!2MfnOhjOj%vhH(I8D`U;s%!Gj;`11(Nfp5>vgSABUKBSynw$yzp8V*` z+PdQd=aN8K3leHGXdQPwSR}#Vuk*I?dqv5>yQ=^|N9?Gs$n>;wFk^T=n98ndh3#8y z2ImawQ_u~))1oU`0x^4jR=lOSM z#LL%D7Y?4>zkbav;m71&T!V(G`1YbIp@W{Ljjw>wqaJHeDn6xYHjgPq%c<%-8(zqE zh~EN7pmXS@EZ)z^y5+}V^KZS_HVyORyVr&}|K|?!)snA;e_dI@rJ+fGoh^33>Qesa zaUx;UyL}vQAeIV0S{IKmam6_pxFvXGTONY08pCScPSby#+9D9|wjb^ORoK7Cd;7w( zTQO%O)!Aj+M=^S=hLm4nqxE{%KB&Yk2uG(kqqXw9Q3l#ftIO zHfMhW!F5)LhIhbzAB8*C#hmW`h1qAeHa2rbIN8e5yU8rI08p^9qB4m))>lKPLIhAH zhhoiCJ97FXR1TXdHfY0&5(EF`aSJf8VKpwq19u{c3h)Y#!TJTBwDvNn3`E4m@%LF2 z8X*lT!8|POK80gE*lt+6d|iO;h~LBOS2TV<m$SzuUQp<39*mq833q!FhzEc@099d%)D5o?{;d^Xp`xm{Y=7&&JY7K-irxy9 z4MTrK+l`b9Q{0Jds}8<=95NYy1#o4#tv{dEfklfkD-C5dZsAvIn3rXhHAI3epBVKVjx= zvrIt0&x|t@-APJrxmr$mL-S;boA#2N2#yd#uG*LWiW^Mn-cYuaCdT9V-lF}HIUnn2 zr;#5ov>A$Ddbat37^b2-n$5bBOuSfTo~-&fe)-6{e#fJ`>(auzKc~Nxs%I_ZgK~8j r%M}T&07H7GfNCY74O@XI8F;0~C<}9FWoxFON4=CI;oi*I!m$4Vc;YoD literal 0 HcmV?d00001 diff --git a/tests/test_mpeg.cpp b/tests/test_mpeg.cpp index 92a7fc3a..9b972382 100644 --- a/tests/test_mpeg.cpp +++ b/tests/test_mpeg.cpp @@ -22,7 +22,8 @@ class TestMPEG : public CppUnit::TestFixture CPPUNIT_TEST(testAudioPropertiesXingHeaderVBR); CPPUNIT_TEST(testAudioPropertiesVBRIHeader); CPPUNIT_TEST(testAudioPropertiesNoVBRHeaders); - CPPUNIT_TEST(testSkipInvalidFrames); + CPPUNIT_TEST(testSkipInvalidFrames1); + CPPUNIT_TEST(testSkipInvalidFrames2); CPPUNIT_TEST(testVersion2DurationWithXingHeader); CPPUNIT_TEST(testSaveID3v24); CPPUNIT_TEST(testSaveID3v24WrongParam); @@ -93,35 +94,43 @@ public: CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); long last = f.lastFrameOffset(); - - f.seek(last); - MPEG::Header lastHeader(f.readBlock(4)); + MPEG::Header lastHeader(&f, last); while (!lastHeader.isValid()) { - last = f.previousFrameOffset(last); - - f.seek(last); - lastHeader = MPEG::Header(f.readBlock(4)); + lastHeader = MPEG::Header(&f, last); } - CPPUNIT_ASSERT_EQUAL(28213L, last); + CPPUNIT_ASSERT_EQUAL(28004L, last); CPPUNIT_ASSERT_EQUAL(209, lastHeader.frameLength()); } - void testSkipInvalidFrames() + void testSkipInvalidFrames1() { MPEG::File f(TEST_FILE_PATH_C("invalid-frames.mp3")); CPPUNIT_ASSERT(f.audioProperties()); CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); - CPPUNIT_ASSERT_EQUAL(393, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(392, f.audioProperties()->lengthInMilliseconds()); CPPUNIT_ASSERT_EQUAL(160, f.audioProperties()->bitrate()); CPPUNIT_ASSERT_EQUAL(2, f.audioProperties()->channels()); CPPUNIT_ASSERT_EQUAL(44100, f.audioProperties()->sampleRate()); CPPUNIT_ASSERT(!f.audioProperties()->xingHeader()); } + void testSkipInvalidFrames2() + { + MPEG::File f(TEST_FILE_PATH_C("invalid-frames2.mp3")); + CPPUNIT_ASSERT(f.audioProperties()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->length()); + CPPUNIT_ASSERT_EQUAL(0, f.audioProperties()->lengthInSeconds()); + CPPUNIT_ASSERT_EQUAL(314, f.audioProperties()->lengthInMilliseconds()); + CPPUNIT_ASSERT_EQUAL(192, 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"));