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 733717 - glmemory allocate size didn't match video_orc_pack_I420 needs if height is odd
glmemory allocate size didn't match video_orc_pack_I420 needs if height is odd
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.4.0
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-25 08:38 UTC by comicfans44
Modified: 2014-07-31 05:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glmemory: use GstVideoInfo everywhere (25.38 KB, patch)
2014-07-29 02:19 UTC, Matthew Waters (ystreet00)
needs-work Details | Review

Description comicfans44 2014-07-25 08:38:07 UTC
(tested with ORC disabled ) 

gst-launch-1.0 videotestsrc  ! video/x-raw, format=\(string\)I420,width=512,height=11 ! glimagesink   
just crash


gst-launch-1.0 videotestsrc  ! video/x-raw, format=\(string\)I420,width=512,height=11 ! videoconvert ! ximagesink
works OK


I found that with ximagesink
gst_video_test_src_fill will be feed with a 
GstBuffer 
n_memery=1
size=9216

but with glimagesink 
GstBuffer
n_memory=3
size=8704 (512*11 + 256*6 + 256*6 allocated in _gl_mem_init)


if height is even ,everything is OK



clang asan report as follows:


==7456==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x9480a080 at pc 0xb67fecfa bp 0x96b787b8 sp 0x96b787b0
WRITE of size 1 at 0x9480a080 thread T1 (videotestsrc0:s)
    #0 0xb67fecf9 in video_orc_pack_I420 /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst-libs/gst/video/tmp-orc.c:1137
    #1 0xb65d9a8b in pack_planar_420 /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst-libs/gst/video/../../../../gst-libs/gst/video/video-format.c:102
    #2 0x9b0a2b0b in convert_hline_generic /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:1202
    #3 0x9b079525 in videotestsrc_convert_tmpline /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:275
    #4 0x9b073793 in gst_video_test_src_smpte /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:423
    #5 0x9b069ee5 in gst_video_test_src_fill /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/gstvideotestsrc.c:929
    #6 0x9e5bf298 in gst_push_src_fill /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:167
    #7 0x9e4a99b2 in gst_base_src_default_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1471
    #8 0x9e5be00f in gst_push_src_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:133
    #9 0x9e47ade6 in gst_base_src_get_range /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2445
    #10 0x9e471ccc in gst_base_src_loop /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2721
    #11 0xb6eb34a3 in gst_task_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttask.c:317
    #12 0xb6ebba83 in default_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttaskpool.c:68
    #13 0xb64a9424 in g_thread_pool_new ??:?
    #14 0xb64a89c9 in g_test_get_filename ??:?
    #15 0x80bd606 in _ZN6__asan10AsanThread11ThreadStartEm ??:?
    #16 0x809c45d in _ZL17asan_thread_startPv asan_interceptors.o:?
    #17 0xb616af6f in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 1)
    #18 0x9e70a70d in clone /build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/i386/clone.S:129

0x9480a080 is located 0 bytes to the right of 1536-byte region [0x94809a80,0x9480a080)
allocated by thread T1 (videotestsrc0:s) here:
    #0 0x80b3a69 in __interceptor_malloc ??:?
    #1 0xb6487be2 in g_malloc ??:?
    #2 0xb724ba85 in gst_gl_memory_setup_buffer /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglmemory.c:1135
    #3 0xb7262ca6 in gst_gl_buffer_pool_alloc /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglbufferpool.c:211
    #4 0xb69b9f9b in do_alloc_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:267
    #5 0xb69b617f in default_acquire_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:1098
    #6 0xb726396f in gst_gl_buffer_pool_acquire_buffer /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglbufferpool.c:250
    #7 0xb69aecef in gst_buffer_pool_acquire_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:1206
    #8 0x9e4aab83 in gst_base_src_default_alloc /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1422
    #9 0x9e5bea7f in gst_push_src_alloc /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:151
    #10 0x9e4a950a in gst_base_src_default_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1462
    #11 0x9e5be00f in gst_push_src_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:133
    #12 0x9e47ade6 in gst_base_src_get_range /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2445
    #13 0x9e471ccc in gst_base_src_loop /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2721
    #14 0xb6eb34a3 in gst_task_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttask.c:317
    #15 0xb6ebba83 in default_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttaskpool.c:68
    #16 0xb64a9424 in g_thread_pool_new ??:?

Thread T1 (videotestsrc0:s) created by T0 here:
    #0 0x809c2fe in pthread_create ??:?
    #1 0xb64c77df in g_private_replace ??:?

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x329013c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x329013d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x329013e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x329013f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x32901400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x32901410:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x32901420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x32901430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x32901440: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x32901450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x32901460: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==7456==ABORTING
Comment 1 Matthew Waters (ystreet00) 2014-07-27 03:50:28 UTC
Ok, so videotestsrc seems to write a 12th Y line of zero's that are never used and GLMemory does not allocate space for.
Comment 2 Sebastian Dröge (slomo) 2014-07-28 07:31:04 UTC
That's wrong, and also:

  gst_video_info_set_format (&info, GST_VIDEO_FORMAT_I420, 512, 11);
  g_print ("%d %d %d\n",
           GST_VIDEO_INFO_COMP_HEIGHT (&info, 0),
           GST_VIDEO_INFO_COMP_HEIGHT (&info, 1),
           GST_VIDEO_INFO_COMP_HEIGHT (&info, 2));
  g_print ("%d\n", info.size);

outputs:

11 6 6
9216


So if videotestsrc writes a 12th line that would be wrong. But the size it uses is correct (and the one used by gl is wrong).


Note the calculations in video-info.c:fill_planes()

      info->stride[0] = GST_ROUND_UP_4 (width);
      info->stride[1] = GST_ROUND_UP_4 (GST_ROUND_UP_2 (width) / 2);
      info->stride[2] = info->stride[1];
      info->offset[0] = 0;
      info->offset[1] = info->stride[0] * GST_ROUND_UP_2 (height);
      info->offset[2] = info->offset[1] +
          info->stride[1] * (GST_ROUND_UP_2 (height) / 2);
      info->size = info->offset[2] +
          info->stride[2] * (GST_ROUND_UP_2 (height) / 2);

We are probably missing the rounding up for the height in gl.
Comment 3 Nicolas Dufresne (ndufresne) 2014-07-28 13:02:52 UTC
Any rational not to use GstVideoInfo on the GL side ? I suppose it's not a big problem if the plane is bigger then what GL is going to write to.
Comment 4 Matthew Waters (ystreet00) 2014-07-28 13:29:26 UTC
(In reply to comment #3)
> Any rational not to use GstVideoInfo on the GL side ? I suppose it's not a big
> problem if the plane is bigger then what GL is going to write to.

We already are using GstVideoInfo.  Each plane has a GLMemory.  The difference is that GST_VIDEO_INFO_COMP_HEIGHT(&info, 0) returns the 11 for the actual height and so GLMemory allocates a sysmem data plane with 11 Y lines whereas the [un]pack video code expects 12 addressable lines.

One simple hack is to always allocate an extra line or two to avoid these issues.  Another would involve adding an extra data_height parameter to all the GLMemory creation routines (I have half a patch for this already).  A third viable option is to add this data size (or height) information to GstVideoInfo/GstVideoFormat itself although I assume that the API is attempting to avoid exposing that for multiview/3D purposes.
Comment 5 Matthew Waters (ystreet00) 2014-07-29 02:19:21 UTC
Created attachment 281897 [details] [review]
glmemory: use GstVideoInfo everywhere

Fourth option, make GLMemory use GstVideoInfo everywhere and patch the GST_VIDEO_INFO_COMP_HEIGHT values for the sysmem data height.
Comment 6 Sebastian Dröge (slomo) 2014-07-29 08:59:52 UTC
The problem is not the component heights, but that plane offsets. With 11 lines there's an extra line of padding between the Y and U plane (into which videotestsrc should also not write).
Comment 7 Nicolas Dufresne (ndufresne) 2014-07-29 12:54:11 UTC
Indeed, to get the aligned default size of a plane, you need to substract the offsets. Careful, because offset zero may not be zero in certain HW context.

/* For planar format, 3 planes */
plane1_size = offset[1] - offset[0];
plane2_size = offset[2] - offset[1];
plane3_size = (offset[0] + size) - offset[2];
Comment 8 Matthew Waters (ystreet00) 2014-07-31 05:21:28 UTC
commit acb38a2a79588e223b7227638c7bd36590d0bfb1
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Thu Jul 31 15:18:04 2014 +1000

    glmemory: use the plane offsets to compute the size of the data pointer
    
    Certain elements expect that there be a certain number of lines
    that they can write into.  e.g. for odd heights, I420, YV12, NV12,
    NV21 (and others) Y lines are expected to have exactly twice the
    number of U/UV lines.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733717

commit 89636392fa3fd2182dd13aa2193aca17edaa102d
Author: Matthew Waters <ystreet00@gmail.com>
Date:   Thu Jul 31 14:07:29 2014 +1000

    glmemory: use GstVideoInfo everywhere
    
    Simplifies a lot of the calling code
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733717