[FFmpeg-devel] [PATCH] lavu/buffer: add release function

[Lukasz Marek]

On 24.02.2014 02:18, Michael Niedermayer wrote:
> On Sun, Feb 23, 2014 at 11:19:23PM +0100, Lukasz Marek wrote:
>> new function allows to unref buffer and obtain its data.
>>
>> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
>> ---
>>   libavutil/buffer.c | 26 ++++++++++++++++++++++++++
>>   libavutil/buffer.h | 12 ++++++++++++
>>   2 files changed, 38 insertions(+)
>>
>> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
>> index e9bf54b..a68b0be 100644
>> --- a/libavutil/buffer.c
>> +++ b/libavutil/buffer.c
>> @@ -117,6 +117,32 @@ void av_buffer_unref(AVBufferRef **buf)
>>       }
>>   }
>>
>> +int av_buffer_release(AVBufferRef **buf, uint8_t **data)
>> +{
>> +    AVBuffer *b;
>> +
>> +    if (!buf || !*buf) {
>> +        if (data)
>> +            *data = NULL;
>> +        return 0;
>> +    }
>> +    b = (*buf)->buffer;
>> +    av_freep(buf);
>> +
>> +    if (!avpriv_atomic_int_add_and_fetch(&b->refcount, -1)) {
>> +        if (data)
>> +            *data = b->data;
>> +        else
>> +            b->free(b->opaque, b->data);
>> +        av_freep(&b);
>
>> +    } else if (data) {
>> +        *data = av_memdup(b->data, b->size);
>> +        if (!*data)
>> +            return AVERROR(ENOMEM);
>
> this is not safe
>
> you decreased the ref count and afterwards copy
> but between the 2 the memory could have been deallocated

Yep,
I attached updated patach - hopely better, and one more which is not 
relevant to the first one, but kinda trivial, so don't want to spam too 
much.

I assumed you talked about unref from other thread while memory is being 
copied. It is true it is not safe, but I think there are already rece 
condition in buffer.c.

For example there is AVBufferRef pointer shared across 2 threads
Thread A has reference and calls unref
Thread B has no reference and calls ref
In case thread A enter buffer release "if" (refcount drops to 0) and 
thread B increases reference during freeing memory then thread B will 
have reference to released buffer if Im not wrong.

Also av_buffer_is_writable seems to be API pitfall. Other thread may 
increase ref count and buffer will not be writable anymore.

If I'm not mistaken than maybe I should open ticket for it?

-- 
Best Regards,
Lukasz Marek

You can avoid reality, but you cannot avoid the consequences of avoiding 
reality. - Ayn Rand
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavu-buffer-add-release-function.patch
Type: text/x-patch
Size: 2356 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140225/b885227f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-lavu-buffer-do-not-touch-refcount-directly.patch
Type: text/x-patch
Size: 777 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140225/b885227f/attachment-0001.bin>