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

[Thomas Guillem]

Ahhh ok, I got it wrong, I didn't use pOutputPortPrivate /
pInputPortPrivate with my player and I read the headers too quicky...
Now that you explained, it's all clear.

I did that patch because of PATCH 13/13.
I needed to add an other FIFO so I searched for an other unused pointer in
OMX_BUFFERHEADERTYPE and I found pOutputPortPrivate. Then I see (but
wrongly) that pOutputPortPrivate should have been used too here.

So I guess I have to rework the Patch 13/13 in order to use an other
pointer for the FIFO.


2014-06-29 22:33 GMT+02:00 Martin Storsjö <martin at martin.st>:

> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>

-- 


This email and any files transmitted with it are confidential and are 
intended solely for the use of the individual or entity to which they are 
addressed. Access to this e-mail by anyone else is unauthorised. If you are 
not the intended recipient, any disclosure, copying, distribution or any 
action taken or omitted to be taken in reliance on it, is prohibited. 
E-mail messages are not necessarily secure. Archos does not accept 
responsibility for any changes made to this message after it was sent.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140629/f52506f2/attachment.html>