[FFmpeg-devel] [PATCH] RPL demuxer

[Michael Niedermayer]

On Fri, Nov 02, 2007 at 04:33:45PM +0100, Christian Ohm wrote:
> 
> Here's a patch for a RPL demuxer. It plays landing.rpl from the samples,
> and several other files from Warzone 2100 (sound only, since there's no
> video decoder yet).
> 
> -- 
> If you think education is expensive, try ignorance.
> 		-- Derek Bok, president of Harvard

[...]

> +	uint8_t *name;
> +	uint8_t *copyright;
> +	uint8_t *author;

unused remove them and all other unused fields as well

[...]
> +	sl = av_malloc(sizeof(int) * 21); // the RPL header has 21 strings
> +	so = av_malloc(sizeof(int) * 21);

no you dont need *alloc


> +
> +	buffer = av_malloc(2048);
> +	get_buffer(pb, buffer, 2048);
> +
> +	b = buffer;
> +
> +	for (i = 0; i < 21; i++)
> +		so[i] = 0;
> +
> +	for (i = 0; i < 21; i++)
> +	{
> +		while (*b++ != '\x0A')
> +			x++;
> +		*(b - 1) = '\x00';

missing end of buffer checks


> +		sl[i] = ++x;
> +		for (j = i; j < 21;)
> +			so[++j] += x;
> +		x = 0;
> +	}

what a mess

write 1 function to read an integer from ByteIOContext and
remove _ALL_ *alloc and buffers
then use this function and for strings get_strz()


[...]
> +	rpl->index = av_malloc(chunknum * sizeof(RPLFrame));

exploitable interger overflow, and there are MANY more

[...]
> +	vst = av_new_stream(s, 0);
> +	if (!vst)
> +		return AVERROR(ENOMEM);
> +	rpl->video_stream_index = vst->index;

its 0 no need to store it


> +	rpl->vst = vst;

its s->streams[0] no need to store it


[...]
> +	av_set_pts_info(vst, 32, 1000, 1000 * rpl->fps);

this is incorrect, use av_d2q()

[...]
> +		rpl->audio_stream_index = ast->index;
> +		rpl->ast = ast;

same commet as for vst*

[...]
> +static int rpl_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> +	RPLContext *rpl = s->priv_data;
> +	RPLFrame *fr;
> +	ByteIOContext *pb = &s->pb;
> +	static int i = 0, video = 1;

what a mess, this is not thread safe

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20071103/3b83f017/attachment.pgp>