| Message ID | b8de4168-3507-c9c0-8ce7-75feec55b47b@googlemail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi, On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > This fixes triggering the av_assert0(ret <= tmp.size). > > The problem was reintroduced by commit > 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by > 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e. > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavcodec/wmavoice.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c > index cd5958c7bc..1bfad46b2e 100644 > --- a/libavcodec/wmavoice.c > +++ b/libavcodec/wmavoice.c > @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext > *ctx, void *data, > * continuing to parse new superframes in the current packet. */ > if (s->sframe_cache_size > 0) { > int cnt = get_bits_count(gb); > + if (cnt + s->spillover_nbits > avpkt->size * 8) { > + av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d > larger than remaining packet size %d, truncating.\n", > + s->spillover_nbits, avpkt->size * 8 - cnt); > + s->spillover_nbits = avpkt->size * 8 - cnt; > + } This litters stderr uselessly. It doesn't help a user or developer accomplish anything. Let me explain, because this is probably confusing. If a library integrator (e.g., the developer of gst-ffmpeg) is playing with wmavoice and forgets to set block_align, the codec should error out. Ideally, it does this saying block_align is zero, so the developer knows what to do. But this is not strictly required and we typically don't do this. However, the above can only happen during corrupt streams (or fuzzing). The user cannot fix this. So any detailed error is useless. Therefore, we should not display any detailed error (or warning) message. Ronald
On 01.01.2017 23:27, Ronald S. Bultje wrote: > On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com <mailto:andreas.cadhalpun@googlemail.com>> wrote: > > This fixes triggering the av_assert0(ret <= tmp.size). > > The problem was reintroduced by commit > 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by > 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e. > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com <mailto:Andreas.Cadhalpun@googlemail.com>> > --- > libavcodec/wmavoice.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c > index cd5958c7bc..1bfad46b2e 100644 > --- a/libavcodec/wmavoice.c > +++ b/libavcodec/wmavoice.c > @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, void *data, > * continuing to parse new superframes in the current packet. */ > if (s->sframe_cache_size > 0) { > int cnt = get_bits_count(gb); > + if (cnt + s->spillover_nbits > avpkt->size * 8) { > + av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d larger than remaining packet size %d, truncating.\n", > + s->spillover_nbits, avpkt->size * 8 - cnt); > + s->spillover_nbits = avpkt->size * 8 - cnt; > + } > > > This litters stderr uselessly. It doesn't help a user or developer accomplish > anything. Let me explain, because this is probably confusing. > > If a library integrator (e.g., the developer of gst-ffmpeg) is playing with > wmavoice and forgets to set block_align, the codec should error out. Ideally, > it does this saying block_align is zero, so the developer knows what to do. > But this is not strictly required and we typically don't do this. > > However, the above can only happen during corrupt streams (or fuzzing). > The user cannot fix this. So any detailed error is useless. I don't think it's useless, as it tells the user that there is a problem with the file. He might just test if ffmpeg can decode it to see if it is corrupt. > Therefore, we should not display any detailed error (or warning) message. So what would you do instead? Best regards, Andreas
Hi, On Sun, Jan 1, 2017 at 5:55 PM, Andreas Cadhalpun < andreas.cadhalpun@googlemail.com> wrote: > On 01.01.2017 23:27, Ronald S. Bultje wrote: > > On Sun, Jan 1, 2017 at 5:18 PM, Andreas Cadhalpun < > andreas.cadhalpun@googlemail.com <mailto:andreas.cadhalpun@googlemail.com>> > wrote: > > > > This fixes triggering the av_assert0(ret <= tmp.size). > > > > The problem was reintroduced by commit > > 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by > > 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e. > > > > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com > <mailto:Andreas.Cadhalpun@googlemail.com>> > > --- > > libavcodec/wmavoice.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c > > index cd5958c7bc..1bfad46b2e 100644 > > --- a/libavcodec/wmavoice.c > > +++ b/libavcodec/wmavoice.c > > @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext > *ctx, void *data, > > * continuing to parse new superframes in the current > packet. */ > > if (s->sframe_cache_size > 0) { > > int cnt = get_bits_count(gb); > > + if (cnt + s->spillover_nbits > avpkt->size * 8) { > > + av_log(ctx, AV_LOG_WARNING, "Number of spillover > bits %d larger than remaining packet size %d, truncating.\n", > > + s->spillover_nbits, avpkt->size * 8 - cnt); > > + s->spillover_nbits = avpkt->size * 8 - cnt; > > + } > > > > > > This litters stderr uselessly. It doesn't help a user or developer > accomplish > > anything. Let me explain, because this is probably confusing. > > > > If a library integrator (e.g., the developer of gst-ffmpeg) is playing > with > > wmavoice and forgets to set block_align, the codec should error out. > Ideally, > > it does this saying block_align is zero, so the developer knows what to > do. > > But this is not strictly required and we typically don't do this. > > > > However, the above can only happen during corrupt streams (or fuzzing). > > The user cannot fix this. So any detailed error is useless. > > I don't think it's useless, as it tells the user that there is a problem > with > the file. He might just test if ffmpeg can decode it to see if it is > corrupt. > > > Therefore, we should not display any detailed error (or warning) message. > > So what would you do instead? I'd just remove the message, but otherwise what you're doing (truncate spillover_nbits) seems fine. You could also return AVERROR_INVALIDDATA, the player will resume playback with a new packet. Make sure (in this particular case) that you reset the internal cache before erroring out (i.e. assign s->sframe_cache_size = 0). Ronald
On 02.01.2017 04:14, Ronald S. Bultje wrote: > On Sun, Jan 1, 2017 at 5:55 PM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > So what would you do instead? > > I'd just remove the message, but otherwise what you're doing (truncate spillover_nbits) seems fine. OK. While I think that the message could have been useful, the important thing is that the code doesn't crash. So I've just pushed the patch without the log message. Best regards, Andreas
diff --git a/libavcodec/wmavoice.c b/libavcodec/wmavoice.c index cd5958c7bc..1bfad46b2e 100644 --- a/libavcodec/wmavoice.c +++ b/libavcodec/wmavoice.c @@ -1923,6 +1923,11 @@ static int wmavoice_decode_packet(AVCodecContext *ctx, void *data, * continuing to parse new superframes in the current packet. */ if (s->sframe_cache_size > 0) { int cnt = get_bits_count(gb); + if (cnt + s->spillover_nbits > avpkt->size * 8) { + av_log(ctx, AV_LOG_WARNING, "Number of spillover bits %d larger than remaining packet size %d, truncating.\n", + s->spillover_nbits, avpkt->size * 8 - cnt); + s->spillover_nbits = avpkt->size * 8 - cnt; + } copy_bits(&s->pb, avpkt->data, size, gb, s->spillover_nbits); flush_put_bits(&s->pb); s->sframe_cache_size += s->spillover_nbits;
This fixes triggering the av_assert0(ret <= tmp.size). The problem was reintroduced by commit 7b27dd5c16de785297ce4de4b88afa0b6685f61d and originally fixed by 2a4700a4f03280fa8ba4fc0f8a9987bb550f0d1e. Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/wmavoice.c | 5 +++++ 1 file changed, 5 insertions(+)