[FFmpeg-cvslog] r17362 - trunk/libavcodec/mpegvideo_xvmc.c

[Michael Niedermayer]

On Mon, Feb 16, 2009 at 12:33:16PM +0100, Diego Biurrun wrote:
> On Mon, Feb 16, 2009 at 09:50:32AM +0200, Ivan Kalvachev wrote:
> > On 2/16/09, diego <subversion at mplayerhq.hu> wrote:
> > >
> > > Log:
> > > Replace two asserts by checks and error messages.
> > >
> > > --- trunk/libavcodec/mpegvideo_xvmc.c	Mon Feb 16 02:59:51 2009	(r17361)
> > > +++ trunk/libavcodec/mpegvideo_xvmc.c	Mon Feb 16 03:02:49 2009	(r17362)
> > > @@ -312,11 +312,14 @@ void ff_xvmc_decode_mb(MpegEncContext *s
> > >
> > > -    assert(render->filled_mv_blocks_num     <= render->allocated_mv_blocks);
> > > -    assert(render->next_free_data_block_num <= render->allocated_data_blocks);
> > > -    /* The above conditions should not be able to fail as long as this function
> > > -     * is used and the following 'if ()' automatically calls a callback to free
> > > -     * blocks. */
> > > +
> > > +    if (render->filled_mv_blocks_num > render->allocated_mv_blocks)
> > > +        av_log(s->avctx, AV_LOG_ERROR,
> > > +               "Not enough space to store mv blocks allocated.\n");
> > > +
> > > +    if (render->next_free_data_block_num > render->allocated_data_blocks)
> > > +        av_log(s->avctx, AV_LOG_ERROR,
> > > +               "Offset to next data block exceeds number of allocated data
> > > blocks.\n");
> > >
> > >
> > >      if (render->filled_mv_blocks_num == render->allocated_mv_blocks)
> > 
> > This is wrong. Revert it.
> > 
> > Asserts are used only to check for conditions that are impossible to occur
> > if the logic of the program is correct.
> > Failing  assert means somewhere (not necessary where the assert is)
> > have gone wrong.
> 
> Then how do you interpret the following part of Michael's review?
> 
>   >    assert(render->filled_mv_blocks_num <= render->total_number_of_mv_blocks);
>   >    assert(render->next_free_data_block_num <= render->total_number_of_data_blocks);
> 
>   5 points
>   replace all assert() checks on all user settable fields by
>   normal if() av_log() whatever else is needed checks
> 
> It seems clear to me that he wants those asserts replaced by checks like
> the ones I implemented.

no, i want them replaced with "whatever is needed" your code does not do that,
instead it replaces the assert() with nothing. If the assert ever was false
your code is strictly worse as the condition likely implicates a buffer
overflow.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090217/13e8c334/attachment.pgp>