After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 628176 - [basetransform] Problems with buffer handling in inplace mode
[basetransform] Problems with buffer handling in inplace mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal critical
: 0.10.31
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-28 09:06 UTC by Sebastian Dröge (slomo)
Modified: 2010-09-23 16:30 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Sebastian Dröge (slomo) 2010-08-28 09:06:46 UTC
+++ This bug was initially created as a clone of Bug #628174 +++

==812== Source and destination overlap in memcpy(0xb9846e0, 0xb9846e0, 622080)
==812==    at 0x4027865: memcpy (mc_replace_strmem.c:497)
==812==    by 0x4552926: gst_base_transform_handle_buffer (string3.h:52)
==812==    by 0x4553489: gst_base_transform_chain (gstbasetransform.c:2308)
==812==    by 0x45C31F4: gst_pad_chain_data_unchecked (gstpad.c:4182)
==812==    by 0x45C3C53: gst_pad_push_data (gstpad.c:4411)
==812==    by 0x7EAFB41: handle_slice (gstmpeg2dec.c:1040)
==812==    by 0x7EB0511: gst_mpeg2dec_chain (gstmpeg2dec.c:1227)
==812==    by 0x45C31F4: gst_pad_chain_data_unchecked (gstpad.c:4182)
==812==    by 0x45C3C53: gst_pad_push_data (gstpad.c:4411)
==812==    by 0x7D7F819: gst_queue_loop (gstqueue.c:1109)
==812==    by 0x45F1460: gst_task_func (gsttask.c:271)
==812==    by 0x45F2B36: default_func (gsttaskpool.c:68)
==812==    by 0x476BD0B: ??? (in /lib/libglib-2.0.so.0.2400.1)
==812==    by 0x4769DEE: ??? (in /lib/libglib-2.0.so.0.2400.1)
==812==    by 0x47F696D: start_thread (pthread_create.c:300)
==812==    by 0x48D7A4D: clone (clone.S:130)


This is the memcpy() in gstbasetransform.c:2186:
>      if (inbuf != *outbuf) {
>        /* different buffers, copy the input to the output first, we then do an
>         * in-place transform on the output buffer. */
>        memcpy (GST_BUFFER_DATA (*outbuf), GST_BUFFER_DATA (inbuf),
>            GST_BUFFER_SIZE (inbuf));
>      }

If the data pointers are the same but the buffer pointer are not the same, one of them must be a subbuffer of the other one and both are not writable at all, or even worse, they're completely unrelated but share the same memory.
Comment 1 Wim Taymans 2010-09-23 16:09:51 UTC
(In reply to comment #0)
> 
> This is the memcpy() in gstbasetransform.c:2186:
> >      if (inbuf != *outbuf) {
> >        /* different buffers, copy the input to the output first, we then do an
> >         * in-place transform on the output buffer. */
> >        memcpy (GST_BUFFER_DATA (*outbuf), GST_BUFFER_DATA (inbuf),
> >            GST_BUFFER_SIZE (inbuf));
> >      }
> 
> If the data pointers are the same but the buffer pointer are not the same, one
> of them must be a subbuffer of the other one and both are not writable at all,
> or even worse, they're completely unrelated but share the same memory.

Your reasoning would be correct if it weren't for the FIXME in gst_base_transform_prepare_output_buffer(). the problem is currently that when the prepare_output_buffer function reuses the input buffer as output buffer (which would increase the refcount of in/out), we decrease that refcount later again in order to keep the reused buffer writable.

I don't quite remember how this abomination came to live but it think it was a natural result of the refcounting practices for a function like prepare_output_buffer(). It should have taken ownership of the input buffer instead of how it was designed before.

So, you can end up in above situation without anything being really wrong.. 

I think the best thing is to simply check for the pointers as well.
Comment 2 Wim Taymans 2010-09-23 16:30:09 UTC
commit 90d65cb446dbdaa681351a4dc0a65e77ac5fa330
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Thu Sep 23 18:18:54 2010 +0200

    basetransform: avoid useless memcpy
    
    Because of the awkward refcounting in prepare_output_buffer, we might end up
    with writable buffers that point to the same data. Check for those cases so that
    we avoid a useless memcpy and keep valgrind quiet.
    
    Fixes #628176