[vlc-devel] [RFC PATCH 09/13] omxil: fix possible memory corruption

[Martin Storsjö]

On Thu, 26 Jun 2014, Thomas Guillem wrote:

> For output buffers, pInputPortPrivate is already used by the OMX_FIFO.
>
> It was safe for the current state. Indeed, the output fifo had never more than
> one elements, so pInputPortPrivate was always NULL, and then pBuffer was never
> overwritten in OmxFillBufferDone.
> ---
> modules/codec/omxil/omxil.c |    6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/modules/codec/omxil/omxil.c b/modules/codec/omxil/omxil.c
> index ea3d7e2..75196b3 100644
> --- a/modules/codec/omxil/omxil.c
> +++ b/modules/codec/omxil/omxil.c
> @@ -1281,7 +1281,7 @@ static picture_t *DecodeVideo( decoder_t *p_dec, block_t **pp_block )
>
>             OMX_FIFO_GET(&p_sys->out.fifo, p_header);
>             p_header->pAppPrivate = p_next_pic;
> -            p_header->pInputPortPrivate = p_header->pBuffer;
> +            p_header->pOutputPortPrivate = p_header->pBuffer;
>             p_header->pBuffer = p_next_pic->p[0].p_pixels;
>         }
>         else
> @@ -1744,9 +1744,9 @@ static OMX_ERRORTYPE OmxFillBufferDone( OMX_HANDLETYPE omx_handle,
>              (int)omx_header->nFilledLen, FromOmxTicks(omx_header->nTimeStamp) );
> #endif
>
> -    if(omx_header->pInputPortPrivate)
> +    if(omx_header->pOutputPortPrivate)
>     {
> -        omx_header->pBuffer = omx_header->pInputPortPrivate;
> +        omx_header->pBuffer = omx_header->pOutputPortPrivate;
>     }
>     OMX_FIFO_PUT(&p_sys->out.fifo, omx_header);
>
> -- 
> 1.7.10.4

This is wrong.

The pOutputPortPrivate field is private to the output port of the codec - 
we can't use it for anything, since the codec might be using this field 
for storing other data associated with the buffer.

I don't see any problem with the current code though. The fifo only uses 
pInputPortPrivate while the buffer actually is stored within the fifo. 
When we take the buffer out from the fifo, we can reuse pInputPortPrivate 
for anything we want - only once we put the buffer back into the fifo, the 
field will be overwritten, but this doesn't matter since we don't read it 
when we take it out from the fifo again.

So I don't really see any issue here, and your patch would certainly break 
things.

Do you agree, or can you explain the flow where memory would be corrupted?

In your commit message, you said:

> It was safe for the current state. Indeed, the output fifo had never more than
> one elements, so pInputPortPrivate was always NULL, and then pBuffer was never
> overwritten in OmxFillBufferDone.

You have the control flow backwards here - the pInputPortPrivate field 
only gets touched while it is in the fifo, i.e. between the OMX_FIFO_PUT 
call, until the OMX_FIFO_GET call. After OMX_FIFO_GET, we should consider 
pInputPortPrivate overwritten to any random value - but at that point we 
don't read it, we only overwrite it to store the pBuffer pointer. Then we 
pass the buffer to the codec, which will not touch the pInputPortPrivate 
field, and when the codec returns the buffer to us, we'll move the value 
back from pInputPortPrivate to pBuffer, and give it to OMX_FIFO_PUT which 
then again overwrites pInputPortPrivate.

// Martin