[MPlayer-dev-eng] [PATCH] fix potential bug in loader/dshow/outputpin.c

[Roberto Togni]

On Sat, 10 Feb 2007 15:13:06 +0600
"Vladimir Voroshilov" <voroshil at gmail.com> wrote:

> Hi, All.
> 
> DirectShow AM_MEDIA_TYPE structure has pbFormat pointer to additional
> media type's data.
> 
> 1. Some methods does copying of this structure without copying
> additional data, pointed by pbFormat. As result two structures will
> point to the same block of additional data.
> Freeing original and copyed structure could cause double free of the
> same memory region.

In some part of the patch you check for the existence of the additional
data block by looking at the pointer (amt->pbFormat not NULL)

> @@ -845,7 +863,16 @@
>   */
>  static void COutputPin_SetNewFormat(COutputPin* This, const
> AM_MEDIA_TYPE* amt) {
> +    if(This->type.pbFormat) free(This->type.pbFormat);
>      This->type = *amt;
> +    if(amt->pbFormat){
> +        This->type.pbFormat=malloc(amt->cbFormat);
> +        if(!This->type.pbFormat) {
[...]

in other parts you check the length of the data block (amt->cbFormat
not 0)

> @@ -977,6 +1005,15 @@
>      This->refcount = 1;
>      This->remote = 0;
>      This->type = *amt;
> +    if(amt->cbFormat>0){
> +        This->type.pbFormat=malloc(amt->cbFormat);
> +        if(!This->type.pbFormat){

Is this wanted? If you're really paranoid you should check both :)


> 2. some methods does freeing of AM_MEDIA_TYPE without freeing
> additional data (memory leak)
> 3. malloc return does not beeing checked for NULL value (out of
> memory). Patch fixes all 3 issues.
> 

Anyway the patch looks ok to me.

I'll try to review your other  dshow patches, but as long as they don't
break anything and nobody comment I think you can apply them, you're
the only one working on that code now.

Ciao,
 Roberto