[FFmpeg-devel] [PATCH] rmdec.c: correctly skip indexes

[Michael Niedermayer]

On Tue, Dec 30, 2008 at 03:01:35PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Tue, Dec 30, 2008 at 2:31 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > you silently remove a validity check, do you just not give a damn if the
> 
> Sorry, I thought I had made them unsigned, that was the intention.
> When unsigned, the check makes no sense (because the value is always
> positive). I don't care if we seek (unsigned int)-1 forward. If you
> prefer signed (?), I can add the check back.

You might not care about seeking 4gb forward but a user possibly will, 
any seek larger than the filesize is invalid and should be handled
as error, not as oops EOF, exit.

besides even if len is unsigned, i dont see how that would stop the 
out of array write


> 
> > besides you change a 16bit BE to a 32bit BE read, its hard to imagine how
> > both could have made sense, you should explain this assuming your code
> > does make any sense.
> 
> You want me to say the original code is wrong? I mean come on, it
> fails to skip the index and results in garbage at the end of a file,
> it was obviously never tested. Need I say more?
> 
> Ok, proof 1: all other level-1 IDs have a 32-bit size, it's
> incomprehensible that real would choose a 16-bit one for INDX.
> 
> proof 2: mplayer/libmpdemux/demux_real.c:generate_index()
>     i = stream_read_dword_le(demuxer->stream);
>     if ((i == -256) || (i != MKTAG('I', 'N', 'D', 'X')))
>     {
> 	mp_msg(MSGT_DEMUX, MSGL_WARN,"Something went wrong, no index chunk
> found on given address (%d)\n",
> 	    next_header_pos);
> 	index_mode = -1;
>         if (i == -256)
> 	    stream_reset(demuxer->stream);
>     	stream_seek(demuxer->stream, origpos);
> 	return 0;
> 	//goto end;
>     }
> 
>     mp_msg(MSGT_DEMUX, MSGL_V,"Reading index table from index chunk (%d)\n",
> 	next_header_pos);
> 
>     i = stream_read_dword(demuxer->stream);
>     mp_msg(MSGT_DEMUX, MSGL_V,"size: %d bytes\n", i);
> 
> In other words, a dword (32-bit) object size.
[...]
> Shows a 32-bit object size.
> 
> Conclusion: the original code is wrong.

good, iam convinced

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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-devel/attachments/20081230/24ed3269/attachment.pgp>