From e6dee0e05311c7c5c95664e7b5d09fa01b86c65e Mon Sep 17 00:00:00 2001 From: Martin Williams Date: Thu, 26 Aug 2021 17:13:07 +0100 Subject: [PATCH 1/3] ID3: Fix v2.4 extended header handling Skipping extended headers under v2.4 presently fails because they are being handled as if they are identical to v2.3. But they are not: In v2.3, the extended header size excludes itself. In v2.4, the extended header size includes itself, and is a synchsafe integer. --- src/id3.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/id3.c b/src/id3.c index bdd2800..6c774a6 100644 --- a/src/id3.c +++ b/src/id3.c @@ -275,7 +275,22 @@ _id3_parse_v2(id3info *id3) // tested with v2.3-ext-header.mp3 // We don't care about the value of the extended flags or CRC, so just read the size and skip it - ehsize = buffer_get_int(id3->buf); + + if (id3->version_major == 3) { + // v2.3: 'Extended header size' excludes itself + ehsize = buffer_get_int(id3->buf); + } + else { + // v2.4: 'Extended header size' includes itself, and is a synchsafe integer of 4 bytes + ehsize = buffer_get_syncsafe(id3->buf, 4); + // must be at least 4 bytes + if (ehsize < 4 ) { + warn("Error: Invalid ID3 extended header - too short (%s)\n", id3->file); + ret = 0; + goto out; + } + ehsize -= 4; // adjust to v2.3 basis + } // ehsize may be invalid, tested with v2.3-ext-header-invalid.mp3 if (ehsize > id3->size_remain - 4) { From 70a5bca5e28d89a50c45bbac76d10ea2a2ab0167 Mon Sep 17 00:00:00 2001 From: Martin Williams Date: Mon, 30 Aug 2021 20:56:47 +0100 Subject: [PATCH 2/3] ID3: Fix v2.4 extended header handling - Test successful read Add test to read v2.4 file with extended header. The test file 'v2.4-ext-header.mp3' has been generated by preparing a short (3 audio frames) mp3 file tagged under ID3 v2.4 with a single 'Blues' genre frame, and an ID3 CRC extended header. --- src/id3.c | 2 +- t/mp3.t | 10 +++++++++- t/mp3/v2.4-ext-header.mp3 | Bin 0 -> 2319 bytes 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100755 t/mp3/v2.4-ext-header.mp3 diff --git a/src/id3.c b/src/id3.c index 6c774a6..208f15e 100644 --- a/src/id3.c +++ b/src/id3.c @@ -272,7 +272,7 @@ _id3_parse_v2(id3info *id3) goto out; } - // tested with v2.3-ext-header.mp3 + // tested with v2.3-ext-header.mp3 & v2.4-ext-header.mp3 // We don't care about the value of the extended flags or CRC, so just read the size and skip it diff --git a/t/mp3.t b/t/mp3.t index ed960ea..86c3acf 100644 --- a/t/mp3.t +++ b/t/mp3.t @@ -3,7 +3,7 @@ use strict; use Digest::MD5 qw(md5_hex); use File::Spec::Functions; use FindBin (); -use Test::More tests => 396; +use Test::More tests => 397; use Test::Warn; use Audio::Scan; @@ -1077,6 +1077,14 @@ eval { is( $tags->{TCON}, 'Blues', 'v2.3 extended header ok' ); } +# v2.4 extended header +{ + my $s = Audio::Scan->scan_tags( _f('v2.4-ext-header.mp3') ); + my $tags = $s->{tags}; + + is( $tags->{TCON}, 'Blues', 'v2.4 extended header ok' ); +} + # MCDI frame { my $s = Audio::Scan->scan( _f('v2.3-mcdi.mp3') ); diff --git a/t/mp3/v2.4-ext-header.mp3 b/t/mp3/v2.4-ext-header.mp3 new file mode 100755 index 0000000000000000000000000000000000000000..8a3dd44fbf99ffc0c269f838a52d20de02b65764 GIT binary patch literal 2319 zcmeH^>pRm60LOp1HLMF!OP-p`h{nXoEtk1%a~X!?7Adp2j80;;a>(`BY&nJ)8il!~ zJn3|Z(^blb+!Er%6SL{yupwFP)XV;c)A!xy&F97Ei*dD30Gt2-bOZnZ@D8Y=VvNoO zy#N=S&o@?v0>Du-BUC~PA^98r+|L3(3;a(D$hKJuq6~S{U&e(3NJKP$lI(O=9@}xs zDWS{_6MU>;R}rWY`s9q9Q&V1!1k%qfX4>TSToK0BA%ku^n5>WIFI(7qej+{E+gCuP z9;(doJ61ed5$#sq3p2`GWjpP$GhEXJ9fV{ z547{L#u{s*eFqEA!>7hW2ZO`ObT!RLZ?#c7I~_XHeqtl!Y6{u)+0iA<(sNV>x7*|Q ziKqqGH0Ms&$PNCAQtJ?-PtB1d2*cT>=NKk`jXYatCF!c)@>=O@k+sgs&Wl((OW+Cy zS!sNO{#mpJ8@uN!dXMvLb?d~$6{_E2O|u!2dzDo%$8SIBiU1Yp+3e!?YngCg{O2csisQW47N6wFOAjJSc@tk;^1kr7=p&7%b&#cFRMLuy(qgN& zkC#{~eI^`eD`yoQ&X9~!AJ-w zKqV4a&$n5-ljboF{CUaOs;Z6jq7~hEmG2KN0rCdH!+3*(pQ9O}*y!hus`K&I-mL@- zwS~xh06uQnL#vFlPj;RuZy%J~;r(F7#85J*4|7c@9#Ou#&jbQvWUemXLDU@U@aj3+L-q<8fujLya)*FH$CcQ}Lg?D(t>l-f{;@yf_n~TJWB(gSX){1T*P3wcJ z1p|ChkoD9YuLTB_dtvrU>_*F6Wo{hlV0nb8AHgL)7mEGu2#03k&bUdJshYm4N(;UY zDUG1yyXgm**vu37=3Tv?Pg$MF152x9A8f~4D79^t8?#p$LKiB>EKkH}EM~)TD99jc z2G*h;#(bEnC`YfBWB66~sQSAHShb%~sGr>uvPMZf$lZVMEp=?Zp+(VBY^L|q(G8)= zJpOgUr6S2gVwW{>P9}qly@;Rmv5~o};4HpTvhNX*Q-f^nrqM+7) zzXiDYiE7Cvs`mW?J74_%`$NJEIY|U=pTR-jsQBJZf}RRkEz4m^KZ=%VMiLb=6emRX z1F40SCLC6aUGwU|e0dpA)qcB>pYi68hvu8;%o5zogmC4(*Stpk8$MRw3?EZAQ%kaL zBBr<#Cdtm`oplwQr2M*VW0BN+U7Slnz{>~`Fy3}OHk}iLczwC8!+sz>pwqZ$*d+*g zZV-{#hBpMFi>Lg7y##nOLrd>;2Fjq9r}DV1p?gxL>7J2JMb&C@HbGdAxg?5x6@*V* zk4{548p&XJV1BK~Aq`FPG+S_Mzp;AwVHb?p2!(d{x<(2i?T9XZ_oH)IA#B^0;a%ON zLRZRyQo44M?&%9-e>0D+ zySh7$GSNMl$(fH Date: Mon, 30 Aug 2021 21:41:13 +0100 Subject: [PATCH 3/3] ID3: Fix v2.4 extended header handling - Test invalid extended header Add tests to read v2.4 files with invalid extended header. The test file 'v2.4-ext-header-invalid-too-short.mp3' is a copy of test file 'v2.4-ext-header.mp3', but with the extended header size edited to become 3. This too short, it must be a minimum of 4 bytes. The test file 'v2.4-ext-header-invalid.mp3' is a copy of test file 'v2.4-ext-header.mp3', but with the extended header size edited to become 1 more than the size of the entire id3 tag. This is too long. --- src/id3.c | 4 ++-- t/mp3.t | 18 ++++++++++++++++-- t/mp3/v2.4-ext-header-invalid-too-short.mp3 | Bin 0 -> 2319 bytes t/mp3/v2.4-ext-header-invalid.mp3 | Bin 0 -> 2319 bytes 4 files changed, 18 insertions(+), 4 deletions(-) create mode 100755 t/mp3/v2.4-ext-header-invalid-too-short.mp3 create mode 100755 t/mp3/v2.4-ext-header-invalid.mp3 diff --git a/src/id3.c b/src/id3.c index 208f15e..9593567 100644 --- a/src/id3.c +++ b/src/id3.c @@ -283,7 +283,7 @@ _id3_parse_v2(id3info *id3) else { // v2.4: 'Extended header size' includes itself, and is a synchsafe integer of 4 bytes ehsize = buffer_get_syncsafe(id3->buf, 4); - // must be at least 4 bytes + // must be at least 4 bytes - tested with v2.4-ext-header-invalid-too-short.mp3 if (ehsize < 4 ) { warn("Error: Invalid ID3 extended header - too short (%s)\n", id3->file); ret = 0; @@ -292,7 +292,7 @@ _id3_parse_v2(id3info *id3) ehsize -= 4; // adjust to v2.3 basis } - // ehsize may be invalid, tested with v2.3-ext-header-invalid.mp3 + // ehsize may be invalid, tested with v2.3-ext-header-invalid.mp3 & v2.4-ext-header-invalid.mp3 if (ehsize > id3->size_remain - 4) { warn("Error: Invalid ID3 extended header size (%s)\n", id3->file); ret = 0; diff --git a/t/mp3.t b/t/mp3.t index 86c3acf..b6f860c 100644 --- a/t/mp3.t +++ b/t/mp3.t @@ -3,7 +3,7 @@ use strict; use Digest::MD5 qw(md5_hex); use File::Spec::Functions; use FindBin (); -use Test::More tests => 397; +use Test::More tests => 399; use Test::Warn; use Audio::Scan; @@ -1309,13 +1309,27 @@ eval { is( $info->{vbr}, 1, 'Xing without LAME marked as VBR ok' ); } -# File with extended header bit set but no extended header +# v2.3 file with extended header bit set but no extended header { warning_like { Audio::Scan->scan( _f('v2.3-ext-header-invalid.mp3') ); } [ qr/Error: Invalid ID3 extended header size/ ], 'v2.3 invalid extended header ok'; } +# v2.4 file with extended header bit set but invalid extended header size +{ + warning_like { Audio::Scan->scan( _f('v2.4-ext-header-invalid.mp3') ); } + [ qr/Error: Invalid ID3 extended header size/ ], + 'v2.4 invalid extended header ok'; +} + +# v2.4 file with extended header bit set but extended header too short +{ + warning_like { Audio::Scan->scan( _f('v2.4-ext-header-invalid-too-short.mp3') ); } + [ qr/Error: Invalid ID3 extended header - too short/ ], + 'v2.4 extended header too short ok'; +} + # Bug 15895, bad APE tag { my $s; diff --git a/t/mp3/v2.4-ext-header-invalid-too-short.mp3 b/t/mp3/v2.4-ext-header-invalid-too-short.mp3 new file mode 100755 index 0000000000000000000000000000000000000000..516b2358f5a9af2ac063a817bdd043de87565867 GIT binary patch literal 2319 zcmeH^>pRm60LOp1HLMF!OP-p`h{oiWTkd9?%P<_bNSV!LbkeL=4!J&N%drtdqcFFW zC!G#)x=PuQTViqI$z{{QVMDUosh9l?r|-MZo6n2S7wvAL05}5x=m-D+@;ji4im^Hu z^a9^z>b3Y6GEbu=qAlqgth%)3&e;F6fLn34N0+RDtc}&MC z=fpA(bjY!WT`W)|^vM}H=cc?I38bG}%&^Vtxgw0KvkboNXtF+Dux#Pr^@;dsZ(kvm zdZ;qT|5&l0BF3Y<7j`0b0D)nPr86Fm=+1ygllmi=-ARO`1(KEBWzt`Ik9T|2cIeh*gD^&l*nr1Ug?p0>tEWiDvI|5XwbMN_pSLaDwWZb}< zq%2_apC{A-?J6y6CFFlwbcR=#;!p_}eK5(I-pe;UZ8qkI7&Mm98|V0f*Ee1^#Jd%{HXDT%Nn~x(tQFm$e60`e z7Ig5%`K+hrI4v-s+zWG1VmDglDs$tB2g@T({qb%Ixlqh+M>zQ=Ncv5hOx5&VRa(e( zNNFUcz(YUK#C8tPH}C5Ge9HPn9#~o>`(QWTLaA-D+L*c05VlY`W_2P~V=)_!b%hMN zPQzN%!x;}#73FBva&-Ud9@PM3pmqBhh5DH-A#;?-gWUc1-crZroBZf}vhCD<8mb{I zg~z{+zr>O}By`yjW@R$S*oy>#mqkQHPVslY6{Dg}g`5pf%76@cNEGf*GClQ3?I3%l z=wWCbZuQ*(IXU-Oc0--IUAKm@&uNx_vTb00zaP$YBwo2>3ND)88kR1KdB>@ei}SSs z`z^rDPgF}bU2ESjunWZRzdt0*kds7W_Zb}Yi%#g>#OtYm)v_Fy^rQ1L&4{8Ry5fY$ zVIZ}L(uBomv1?u(m@6*>s@iWC@iX51@z8t|m05y)nHZtG_nP-;K*PuCn-OEmW@^bc zO$0I$FCe*?ch*&Kk_+m#jYU%Pb#X2Q0WZTx!g$;Dm~>7o;`QaW4u^q+z)oY~F@= zb$6uGCqD(PMlKf14ZiCa<`_Dfhx!BZ039>k6`ub!NZ>RsR9R CCT#)$ literal 0 HcmV?d00001 diff --git a/t/mp3/v2.4-ext-header-invalid.mp3 b/t/mp3/v2.4-ext-header-invalid.mp3 new file mode 100755 index 0000000000000000000000000000000000000000..305ebb89f93e2712cce229a123b040384da5de64 GIT binary patch literal 2319 zcmeH^>pRm60LOp1HLMF!OP-p`h{oiWTdp(PT!!JeMapb0qmx*z9CCfkmSc#aO<{(V zC!G#)x=PuQTSA<8a@lln*pRGt>Sce!>H9q27vDFZ7oRT%X`ui(0|4mAw?cP76%}K2 zF6afg;d}r9P=*4)Q8OdggcL&ZxA`+a4g56lKQ$oRW-5p>CxWa zLMru8RgT}Wl8MS__lh3aiPU}smL-lDPy_XHvSR3s-P;?$XITkt)98RXGX-0ahjo91k&=?Nm8zEOy$jE0$mo&@HQR&<+ zkKf0m7GP8CJDtNf1S?7{gY;fCC-!_8&OSZIF!^ib**a@UXZ@DfN^i5QWkz;h#N1g5 zS1`y*6BzW(pfy<7Jy+3tTxP0U#>cNv{T6GQ%q+QAnT4~0wv$K%s8HwL^M22cllX|( z{y9l`{4PlE(_>r9g!|$@KlxLf=EQdRBu{>N5K+n>|KgVSMZiTLX*jK8SvE=~t*9t1 zwrKl!iKSA0<}(jy$eNWtt%B#XCxs#u*}cw68#x8aJRD+>#IBT|F=r<7xfGw8s`L$d zLO`KwB60P6s||`YhjA3lNxpJ88|lR>y74OCA6f$B4T6X81_wV!(?hY*&z)4~;%&TJ z2pDQJk?{b0+^RdjD$XI~BZd`6ej(O`1&A^c^QH z_&TI4f>PkFA7ElTM-Z5I_Iy5NeIgGm<;XtRjWtthTdg)`uJA$^sz$9&#AqyL!*Q;V z0oQ3*vw9fgVXC4Wty+%mSKY1Zj|#AEJEKrPvn6DXkob_h|K3|_-+YrFm7iiewV#IO zg(mX_*9n)3B@c<6HpE$(3^Mv6e!|ltyfUZcJKu^?*{YI~4NuH~47y7c?oTp3^vG=> zd!?u$XdQm_-2pi{WDJW}r*7A!VeEan*e}U8ps&vtZ#o>OTsj38&2J4!7sdSJRLRBp zTL1kP;HD?4rJJs`?-y7F;`iSl5@yIrB5?Z*4*EvL_iPgMRKRLkj!XJc`I%-UQ4w8n zT;$N7T108YVYOH_uMW&rlmnc$+eLzmH-9`d-$ZAY;$9|%EAPGLHR8|vSbZ~mRM|`| z$)=H*f+9?iUCcY`D%nW|b=$@wsrkA%mx6$o6Cz;z?RsoFI|lLka%;Ooe|$iPaq*B_ zkmb1nL}n}A5Qr|B^au75;7xQbz0(=420eV0$F01s36;isMmm+8)#Pk~upVfwjoFk&NDG|KB5DTK5my5!xD%prxaZ9BSm zb)yPRDGN&JGJ?i|3V}w;^6Ej~<0;wM1g%$nH0=s?U3638Kd&uo;Tg>7H7y)VHy_kf z)TezvPysLr`gIo3tGX{R8~G-$q`)cn!de4SpbF~@w2yISz$Q5V E0TGsMCjbBd literal 0 HcmV?d00001