[Ffmpeg-devel] patch for AVI files

[Michael Niedermayer]

Hi

On Fri, Feb 10, 2006 at 03:04:28PM +0530, Dilip Kumar Rai wrote:
> Hello Developers,
> I am a developer working at AOL Bangalore on Audio Video search technology,
> I came to know about FFmpeg when I was evaluating various softwares for
> extracting meta data information (e.g. Title, Author, Genre etc..) from
> media files. I think FFmpeg is very good in handling many file formats and
> codecs, however we had to drop FFmpeg against native API's because it failed
> to extract metadata for many file formats. 
> Nevertheless, I have decided to bridge this gap by providing missing
> functionality to FFmpeg. This fix is first in the series. I hope that this
> will add significant values to the product.
>  
> Nature of Fix: Added code for writing/reading INFO tags in AVI files.
>  
> Description: With this fix it will be possible to extract and embeds INFO
> tags like, title, author, creation date etc... in a AVI file. With this fix
> generated files will be at least 28 bytes larger. 

besides the tabs, trailing whitespace and typos (nah i dont care about the
typos) ...


[...]

> +		case MKTAG('I','C','O','P'):
> +	    	{
> +				//Coptright
> +				get_buffer(pb, s->copyright, size);
> +	    	}
> +	    	break;

the {} on their own lines are unneeded (not a real issue but still its
spreads the code over many pages and makes it hard to read IMHO)
the comment is redunant as its just the variable name (i wont accept that)
the indention doesnt follow the ffmpeg 4 space rule
theres a heap overflow


[...]
> +				char szDate[16];
> +				get_buffer(pb,szDate,size);

exploitable stack overflow


[...]

>  }
> +/**
> +Writes 'LIST INFO' tags to AVI streams.
> +Presently supported INFO tags are INAM, ICOP, ICMS, ICMT, ICRD, IPRP, IGNR, ISFT. More tags can be added later....
> + at param pb [IN, OUT] ByteIOContext* 
> + at param s [IN] AVFormatContext*
> +*/
> +static void write_info_tags(ByteIOContext *pb, const AVFormatContext *s)
> +{
> +    int len;
> +    offset_t list_start=start_tag(pb,"LIST");
> +    put_tag(pb,"INFO");
> +
> +    len=strlen(s->title);
> +    if(len>0)
> +    {
> +	len++;
> +	len+=(len&1);
> +	put_tag(pb,"INAM");
> +	put_le32(pb,len);
> +	put_buffer(pb,s->title,len);
> +    }
> +
> +    //Coptright
> +    len=strlen(s->copyright);
> +    if(len>0)
> +    {
> +	len++;
> +	len+=(len&1);
> +	put_tag(pb,"ICOP");
> +	put_le32(pb,len);
> +	put_buffer(pb, s->copyright, len);
> +    }

this is a redundant mess, this should be put into a function or table, not
repeated over and over again
and are you sure the rounded up length should be stored?

[...]

-- 
Michael