[FFmpeg-devel] [PATCH 1/2] wmavoice: truncate spillover_nbits if too large

[Ronald S. Bultje]

Hi,

On Sun, Jan 1, 2017 at 5:55 PM, Andreas Cadhalpun <
andreas.cadhalpun at 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 at googlemail.com <mailto:andreas.cadhalpun at 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 at googlemail.com
> <mailto:Andreas.Cadhalpun at 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