[MPlayer-dev-eng] [PATCH] xvmc: handle shm xvimage allocation error

[Reimar Döffinger]

On Sat, Sep 10, 2011 at 11:58:33PM +0200, Marcin Slusarz wrote:
> - assert xvimage->data_size!=0 because 0 size will lead to memory corruptions
>   (I hit it because of bug in Nouveau's early xvmc implementation)

An assert is not really that much better.
Failing is fine. So is checking data_size at the appropriate place.
But an assert isn't a good enough solution to be worth it IMO.

> -    if ( mLocalDisplay && XShmQueryExtension( mDisplay ) ) Shmem_Flag = 1;
> +    if ( mLocalDisplay && XShmQueryExtension( mDisplay ) )
> +        Shmem_Flag = 0;
>      else
>      {
>          Shmem_Flag = 0;

That is rather pointless if you set it to 0 in both cases.
And the warning about not using Shm will no longer really
correspond to reality after this change.
Lastly, this change causes deallocate_xvimage to always use free
instead of XShmDetach when it should.

> @@ -198,25 +208,41 @@
>      {
>          xvimage = (XvImage *) XvShmCreateImage(mDisplay, xv_port, xv_format,
>                               NULL, xvimage_width, xvimage_height, &Shminfo);
> +        if (!xvimage)
> +            goto noshmimage;
>  
> +        assert(xvimage->data_size);
>          Shminfo.shmid    = shmget(IPC_PRIVATE, xvimage->data_size, IPC_CREAT | 0777);
> +        if (Shminfo.shmid == -1)
> +            goto shmgetfail;
> +
>          Shminfo.shmaddr  = (char *) shmat(Shminfo.shmid, 0, 0);
> +        if ((long)Shminfo.shmaddr == -1)

Documentation says it returns (void *)-1, so you should check for
equality with that really.

> +shmattachfail:
> +    shmdt(Shminfo.shmaddr);
> +shmatfail:
> +    shmctl(Shminfo.shmid, IPC_RMID, 0);
> +shmgetfail:
> +    XFree(xvimage);

This would be much nicer (i.e. require only a single goto)
if initializing shmaddr/shmid/xvimage to appropriate invalid
values, adding a shm_deallocate function that frees those
that have a valid value and then reusing that in deallocate_xvimage
should also get rid of the need for Shmem_Flag.