| Message ID | 20190411230920.6630-5-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | 3dee6c09970dd0defad91e8270f66a4be3b6570d |
| Headers | show |
fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint: > KLV length is BER encoded (variable size), but the code assumed the encoding to > always use 4 bytes. > > Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf. > > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavformat/mxfdec.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index 6f0f87763d..a69f2f1996 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s) > goto end; > avio_seek(s->pb, file_size - length, SEEK_SET); > if (klv_read_packet(&klv, s->pb) < 0 || > - !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) || > - klv.length != length - 20) > + !IS_KLV_KEY(klv.key, mxf_random_index_pack_key)) > goto end; > + if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) { > + av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n"); > + goto end; > + } Looks OK. Aside: Looking at klv_read_packet(), I'm suspicious of its use of mxf_read_sync(). That feels like something that only belongs in mxf_read_header(), to deal with run-in. Baptiste added it in cabe2527ef3. FATE passes without it. /Tomas
On Sun, 14 Apr 2019, Tomas Härdin wrote: > fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint: >> KLV length is BER encoded (variable size), but the code assumed the encoding to >> always use 4 bytes. >> >> Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf. >> >> > Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavformat/mxfdec.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> index 6f0f87763d..a69f2f1996 100644 >> --- a/libavformat/mxfdec.c >> +++ b/libavformat/mxfdec.c >> @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s) >> goto end; >> avio_seek(s->pb, file_size - length, SEEK_SET); >> if (klv_read_packet(&klv, s->pb) < 0 || >> - !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) || >> - klv.length != length - 20) >> + !IS_KLV_KEY(klv.key, mxf_random_index_pack_key)) >> goto end; >> + if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) { >> + av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n"); >> + goto end; >> + } > > Looks OK. Thanks, applied. > > Aside: Looking at klv_read_packet(), I'm suspicious of its use of > mxf_read_sync(). That feels like something that only belongs in > mxf_read_header(), to deal with run-in. Baptiste added it in > cabe2527ef3. FATE passes without it. I wonder if it is there to allow some byte based seeking or handle damaged files more gracefully? I agree it is strange a bit, maybe Baptise can chip in... Regards, Marton
sön 2019-04-28 klockan 22:17 +0200 skrev Marton Balint: > > On Sun, 14 Apr 2019, Tomas Härdin wrote: > > > fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint: > > > KLV length is BER encoded (variable size), but the code assumed the encoding to > > > always use 4 bytes. > > > > > > Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf. > > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > > > > --- > > > libavformat/mxfdec.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > index 6f0f87763d..a69f2f1996 100644 > > > --- a/libavformat/mxfdec.c > > > +++ b/libavformat/mxfdec.c > > > @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s) > > > goto end; > > > avio_seek(s->pb, file_size - length, SEEK_SET); > > > if (klv_read_packet(&klv, s->pb) < 0 || > > > - !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) || > > > - klv.length != length - 20) > > > + !IS_KLV_KEY(klv.key, mxf_random_index_pack_key)) > > > goto end; > > > + if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) { > > > + av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n"); > > > + goto end; > > > + } > > > > Looks OK. > > Thanks, applied. > > > > > Aside: Looking at klv_read_packet(), I'm suspicious of its use of > > mxf_read_sync(). That feels like something that only belongs in > > mxf_read_header(), to deal with run-in. Baptiste added it in > > cabe2527ef3. FATE passes without it. > > I wonder if it is there to allow some byte based seeking or handle damaged > files more gracefully? I agree it is strange a bit, maybe Baptise can chip > in... I *suspect* it has something to do with seeking in frame wrapped files with crappy or non-existing indexes. But then it would be best if it were used only in mxf_read_seek() /Tomas
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 6f0f87763d..a69f2f1996 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -3126,9 +3126,12 @@ static void mxf_read_random_index_pack(AVFormatContext *s) goto end; avio_seek(s->pb, file_size - length, SEEK_SET); if (klv_read_packet(&klv, s->pb) < 0 || - !IS_KLV_KEY(klv.key, mxf_random_index_pack_key) || - klv.length != length - 20) + !IS_KLV_KEY(klv.key, mxf_random_index_pack_key)) goto end; + if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) { + av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n"); + goto end; + } avio_skip(s->pb, klv.length - 12); mxf->footer_partition = avio_rb64(s->pb);
KLV length is BER encoded (variable size), but the code assumed the encoding to always use 4 bytes. Fixes parsing Random Index Pack in samples/MXF/issue2160/PW0805A0V01.4C5B5636.EFA330.mxf. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavformat/mxfdec.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)