[FFmpeg-devel] mxfdec: Constrain run-in to 64k

Submitted by Tomas Härdin on April 15, 2019, 8:30 a.m.

Details

Message ID 1555317042.30431.3.camel@acc.umu.se
State New
Headers show

Commit Message

Tomas Härdin April 15, 2019, 8:30 a.m.
This isn't likely to be a huge problem, but it allows us to reason more
about run-in. It also exposes my gripe about klv_read_packet() using
mxf_read_sync()

/Tomas

Comments

Carl Eugen Hoyos April 15, 2019, 10:40 a.m.
2019-04-15 10:30 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> This isn't likely to be a huge problem, but it allows us to reason more
> about run-in. It also exposes my gripe about klv_read_packet() using
> mxf_read_sync()

Does this patch have an effect on one of our samples?

Carl Eugen
Marton Balint April 15, 2019, 9:03 p.m.
On Mon, 15 Apr 2019, Tomas Härdin wrote:

> This isn't likely to be a huge problem, but it allows us to reason more
> about run-in. It also exposes my gripe about klv_read_packet() using
> mxf_read_sync()

I would allow 65536 bytes as well for run-in, even if that is against the 
standard. The MXF book for example says this:

run-in - a sequence of up to 65536 bytes at the front of the file that 
preceeds the first partition pack.

So I guess there is a chance of seeing files with 65536 byte run-in in the 
wild, and those files used to work...

Regards,
Marton
Tomas Härdin April 15, 2019, 10 p.m.
mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
> > 2019-04-15 10:30 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> > This isn't likely to be a huge problem, but it allows us to reason more
> > about run-in. It also exposes my gripe about klv_read_packet() using
> > mxf_read_sync()
> 
> Does this patch have an effect on one of our samples?

It passes FATE if that's what you mean. The intent is rather to pre-
emptively stop the ability for new broken muxers to pop up, which would
end up straying from the spec because mxfdec.c is being too forgiving

/Tomas
Tomas Härdin April 15, 2019, 10:03 p.m.
mån 2019-04-15 klockan 23:03 +0200 skrev Marton Balint:
> 
> On Mon, 15 Apr 2019, Tomas Härdin wrote:
> 
> > This isn't likely to be a huge problem, but it allows us to reason more
> > about run-in. It also exposes my gripe about klv_read_packet() using
> > mxf_read_sync()
> 
> I would allow 65536 bytes as well for run-in, even if that is against the 
> standard. The MXF book for example says this:
> 
> run-in - a sequence of up to 65536 bytes at the front of the file that 
> preceeds the first partition pack.

So I see. Hm.. We could report that to the publisher as an error in the
book. But I doubt they'd do a reprint for such a minor error.

> So I guess there is a chance of seeing files with 65536 byte run-in in the 
> wild, and those files used to work...

This is potentially true, and would cause unnecessary annoyance. But
staying close to the spec is also of interest. Hm...

/Tomas
Carl Eugen Hoyos April 15, 2019, 10:41 p.m.
2019-04-16 0:00 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
>> > 2019-04-15 10:30 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
>> > This isn't likely to be a huge problem, but it allows us to reason more
>> > about run-in. It also exposes my gripe about klv_read_packet() using
>> > mxf_read_sync()
>>
>> Does this patch have an effect on one of our samples?
>
> It passes FATE if that's what you mean.

No.

> The intent is rather to pre-emptively stop the ability for new
> broken muxers to pop up, which would end up straying
> from the spec because mxfdec.c is being too forgiving

As such, and if I don't misunderstand, this sounds to me
like an explanation why this patch should not be pushed.

Carl Eugen
Tomas Härdin April 16, 2019, 12:44 p.m.
tis 2019-04-16 klockan 00:41 +0200 skrev Carl Eugen Hoyos:
> > 2019-04-16 0:00 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> > mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
> > > > > > > > 2019-04-15 10:30 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> > > > This isn't likely to be a huge problem, but it allows us to reason more
> > > > about run-in. It also exposes my gripe about klv_read_packet() using
> > > > mxf_read_sync()
> > > 
> > > Does this patch have an effect on one of our samples?
> > 
> > It passes FATE if that's what you mean.
> 
> No.
> 
> > The intent is rather to pre-emptively stop the ability for new
> > broken muxers to pop up, which would end up straying
> > from the spec because mxfdec.c is being too forgiving
> 
> As such, and if I don't misunderstand, this sounds to me
> like an explanation why this patch should not be pushed.

If you want to encourage spec violating behavior in professional
muxers, sure. But for anyone who has to work with MXF, the ability to
tell the other engineer "no, go fix your damn muxer" is important. The
field is already a huge mess of broken muxers and demuxers. We should
make sure our own yard is in order

As always, I'm going to point out that we'd be better off using bmxlib

/Tomas
Paul B Mahol April 16, 2019, 1:16 p.m.
On 4/16/19, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> tis 2019-04-16 klockan 00:41 +0200 skrev Carl Eugen Hoyos:
>> > 2019-04-16 0:00 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
>> > mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
>> > > > > > > > 2019-04-15 10:30 GMT+02:00, Tomas Härdin
>> > > > > > > > <tjoppen@acc.umu.se>:
>> > > > This isn't likely to be a huge problem, but it allows us to reason
>> > > > more
>> > > > about run-in. It also exposes my gripe about klv_read_packet() using
>> > > > mxf_read_sync()
>> > >
>> > > Does this patch have an effect on one of our samples?
>> >
>> > It passes FATE if that's what you mean.
>>
>> No.
>>
>> > The intent is rather to pre-emptively stop the ability for new
>> > broken muxers to pop up, which would end up straying
>> > from the spec because mxfdec.c is being too forgiving
>>
>> As such, and if I don't misunderstand, this sounds to me
>> like an explanation why this patch should not be pushed.
>
> If you want to encourage spec violating behavior in professional
> muxers, sure. But for anyone who has to work with MXF, the ability to
> tell the other engineer "no, go fix your damn muxer" is important. The
> field is already a huge mess of broken muxers and demuxers. We should
> make sure our own yard is in order
>
> As always, I'm going to point out that we'd be better off using bmxlib

I can not follow your logic.
Carl Eugen Hoyos April 16, 2019, 1:29 p.m.
2019-04-16 14:44 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> tis 2019-04-16 klockan 00:41 +0200 skrev Carl Eugen Hoyos:
>> > 2019-04-16 0:00 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
>> > mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
>> > > > > > > > 2019-04-15 10:30 GMT+02:00, Tomas Härdin
>> > > > > > > > <tjoppen@acc.umu.se>:
>> > > > This isn't likely to be a huge problem, but it allows us to reason
>> > > > more
>> > > > about run-in. It also exposes my gripe about klv_read_packet() using
>> > > > mxf_read_sync()
>> > >
>> > > Does this patch have an effect on one of our samples?
>> >
>> > It passes FATE if that's what you mean.
>>
>> No.
>>
>> > The intent is rather to pre-emptively stop the ability for new
>> > broken muxers to pop up, which would end up straying
>> > from the spec because mxfdec.c is being too forgiving
>>
>> As such, and if I don't misunderstand, this sounds to me
>> like an explanation why this patch should not be pushed.
>
> If you want to encourage spec violating behavior in professional
> muxers, sure. But for anyone who has to work with MXF, the ability to
> tell the other engineer "no, go fix your damn muxer" is important.

Apart from: This is - fortunately - not what we do for all other
formats (we wouldn't be here for a very long time if we did):
Why don't you print a warning?

> The field is already a huge mess of broken muxers and
> demuxers. We should make sure our own yard is in order

This can only be true for muxers (and encoders), never for
demuxers (and decoders).

Carl Eugen
Tomas Härdin April 16, 2019, 3:38 p.m.
tis 2019-04-16 klockan 15:29 +0200 skrev Carl Eugen Hoyos:
> > 2019-04-16 14:44 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> > tis 2019-04-16 klockan 00:41 +0200 skrev Carl Eugen Hoyos:
> > > > > > > > 2019-04-16 0:00 GMT+02:00, Tomas Härdin <tjoppen@acc.umu.se>:
> > > > mån 2019-04-15 klockan 12:40 +0200 skrev Carl Eugen Hoyos:
> > > > > > > > > > 2019-04-15 10:30 GMT+02:00, Tomas Härdin
> > > > > > > > > > <tjoppen@acc.umu.se>:
> > > > > > 
> > > > > > This isn't likely to be a huge problem, but it allows us to reason
> > > > > > more
> > > > > > about run-in. It also exposes my gripe about klv_read_packet() using
> > > > > > mxf_read_sync()
> > > > > 
> > > > > Does this patch have an effect on one of our samples?
> > > > 
> > > > It passes FATE if that's what you mean.
> > > 
> > > No.
> > > 
> > > > The intent is rather to pre-emptively stop the ability for new
> > > > broken muxers to pop up, which would end up straying
> > > > from the spec because mxfdec.c is being too forgiving
> > > 
> > > As such, and if I don't misunderstand, this sounds to me
> > > like an explanation why this patch should not be pushed.
> > 
> > If you want to encourage spec violating behavior in professional
> > muxers, sure. But for anyone who has to work with MXF, the ability to
> > tell the other engineer "no, go fix your damn muxer" is important.
> 
> Apart from: This is - fortunately - not what we do for all other
> formats (we wouldn't be here for a very long time if we did):
> Why don't you print a warning?

Eh, that doesn't really accomplish much.

I guess I'll have to look at this from the positive side: more people
writing broken muxers means more consulting hours for the rest of us!

/Tomas
Michael Niedermayer April 16, 2019, 10:30 p.m.
On Tue, Apr 16, 2019 at 12:03:48AM +0200, Tomas Härdin wrote:
> mån 2019-04-15 klockan 23:03 +0200 skrev Marton Balint:
> > 
> > On Mon, 15 Apr 2019, Tomas Härdin wrote:
> > 
> > > This isn't likely to be a huge problem, but it allows us to reason more
> > > about run-in. It also exposes my gripe about klv_read_packet() using
> > > mxf_read_sync()
> > 
> > I would allow 65536 bytes as well for run-in, even if that is against the 
> > standard. The MXF book for example says this:
> > 
> > run-in - a sequence of up to 65536 bytes at the front of the file that 
> > preceeds the first partition pack.
> 

> So I see. Hm.. We could report that to the publisher as an error in the
> book. But I doubt they'd do a reprint for such a minor error.

But if a reprint ever happens it is more likely to contain reported
issues than unreported ones. So i agree its a good idea to report it

thx

[...]

Patch hide | download patch | download mbox

From c2d66c4aa3105e33f8485234ca760da699cdfb4d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <tjoppen@acc.umu.se>
Date: Sun, 14 Apr 2019 21:18:35 +0200
Subject: [PATCH] mxfdec: Constrain run-in to 64k

S377m says we should.
Fix use of magic 14s while we're at it.
---
 libavformat/mxfdec.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 8c65a2bbcf..6af760c5c4 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -282,6 +282,7 @@  typedef struct MXFContext {
     int local_tags_count;
     uint64_t footer_partition;
     KLVPacket current_klv_data;
+#define MXF_MAX_RUN_IN 65535    /* S377m section 5.5 */
     int run_in;
     MXFPartition *current_partition;
     int parsing_backward;
@@ -383,10 +384,10 @@  static int64_t klv_decode_ber_length(AVIOContext *pb)
     return size;
 }
 
-static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size)
+static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size, int64_t max_read)
 {
     int i, b;
-    for (i = 0; i < size && !avio_feof(pb); i++) {
+    for (i = 0; i < size && !avio_feof(pb) && max_read > 0; i++, max_read--) {
         b = avio_r8(pb);
         if (b == key[0])
             i = 0;
@@ -399,7 +400,7 @@  static int mxf_read_sync(AVIOContext *pb, const uint8_t *key, unsigned size)
 static int klv_read_packet(KLVPacket *klv, AVIOContext *pb)
 {
     int64_t length, pos;
-    if (!mxf_read_sync(pb, mxf_klv_key, 4))
+    if (!mxf_read_sync(pb, mxf_klv_key, 4, INT64_MAX))
         return AVERROR_INVALIDDATA;
     klv->offset = avio_tell(pb) - 4;
     memcpy(klv->key, mxf_klv_key, 4);
@@ -3149,11 +3150,13 @@  static int mxf_read_header(AVFormatContext *s)
 
     mxf->last_forward_tell = INT64_MAX;
 
-    if (!mxf_read_sync(s->pb, mxf_header_partition_pack_key, 14)) {
+    if (!mxf_read_sync(s->pb, mxf_header_partition_pack_key,
+            sizeof(mxf_header_partition_pack_key),
+            MXF_MAX_RUN_IN + sizeof(mxf_header_partition_pack_key))) {
         av_log(s, AV_LOG_ERROR, "could not find header partition pack key\n");
         return AVERROR_INVALIDDATA;
     }
-    avio_seek(s->pb, -14, SEEK_CUR);
+    avio_seek(s->pb, -sizeof(mxf_header_partition_pack_key), SEEK_CUR);
     mxf->fc = s;
     mxf->run_in = avio_tell(s->pb);
 
@@ -3591,6 +3594,10 @@  static int mxf_probe(const AVProbeData *p) {
     /* Must skip Run-In Sequence and search for MXF header partition pack key SMPTE 377M 5.5 */
     end -= sizeof(mxf_header_partition_pack_key);
 
+    if (end - bufp > MXF_MAX_RUN_IN) {
+        end = bufp + MXF_MAX_RUN_IN;
+    }
+
     for (; bufp < end;) {
         if (!((bufp[13] - 1) & 0xF2)){
             if (AV_RN32(bufp   ) == AV_RN32(mxf_header_partition_pack_key   ) &&
-- 
2.11.0