| Message ID | 1555317042.30431.3.camel@acc.umu.se |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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.
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
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
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 [...]
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