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 695655 - audiovisualizer: crash while clearing video frame
audiovisualizer: crash while clearing video frame
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.0.5
Other Linux
: Normal normal
: 1.0.6
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-03-11 20:54 UTC by Greg Rutz
Modified: 2013-03-13 01:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Makes Basic Tutorial 7 from the GStreamer SDK work with 1.0 (1.01 KB, patch)
2013-03-11 20:54 UTC, Greg Rutz
none Details | Review
Proposed fix (1.23 KB, patch)
2013-03-11 21:06 UTC, Greg Rutz
reviewed Details | Review
Updated patch based on reviewer comments (1.22 KB, patch)
2013-03-12 03:40 UTC, Greg Rutz
none Details | Review
Fix incorrect patch file from previous submission (1.24 KB, patch)
2013-03-12 03:58 UTC, Greg Rutz
committed Details | Review

Description Greg Rutz 2013-03-11 20:54:57 UTC
Created attachment 238621 [details] [review]
Makes Basic Tutorial 7 from the GStreamer SDK work with 1.0

In the process of clearing the video frame, the current chain function performs an illegal memset.  I found this while trying to run through the GStreamer SDK Basic Tutorials using gstreamer-1.0.

Steps to reproduce:

1) Start with the code from Basic Tutorial 7 and save it as bt7.c (http://docs.gstreamer.com/display/GstSDK/Basic+tutorial+7%3A+Multithreading+and+Pad+Availability)
2) Apply the attached patch so that the code will compile/run against gstreamer-1.0
3) Run to see the crash
Comment 1 Greg Rutz 2013-03-11 21:06:43 UTC
Created attachment 238622 [details] [review]
Proposed fix
Comment 2 Tim-Philipp Müller 2013-03-12 00:32:55 UTC
Comment on attachment 238622 [details] [review]
Proposed fix

Thanks for the patch!

>       /* gst_video_frame_clear() or is output frame already cleared */
>-      memset (outframe.data, 0, scope->vinfo.size);
>+      for (i = 0; i < scope->vinfo.finfo->n_planes; i++) {
>+        memset (outframe.data[i], 0, scope->vinfo.size);
>+      }

I'm not sure if this (scope->vinfo.size) is entirely right here for more than 1 plane, though of course all visualizers in this plugin only support BGRx for now, so there's only one plane.

There doesn't appear to be a convenient way to get the plane size as far as I can see (maybe frame.map[i].size?).
Comment 3 Greg Rutz 2013-03-12 03:40:45 UTC
Created attachment 238661 [details] [review]
Updated patch based on reviewer comments

I think you are exactly right.  Each plane's data is the result of a memory map operation, so the GstMapInfo structure at the given plane index contains the appropriate size for that plane.
Comment 4 Greg Rutz 2013-03-12 03:58:45 UTC
Created attachment 238662 [details] [review]
Fix incorrect patch file from previous submission

Accidentally committed the wrong patch file in my last comment. This file contains my actual proposed fix
Comment 5 Tim-Philipp Müller 2013-03-13 01:05:34 UTC
Thanks, pushed:

commit c66fd54e78fbc777e928ad82d6cb5064676ab7ea
Author: Greg Rutz <greg@gsr-tek.com>
Date:   Mon Mar 11 21:55:28 2013 -0600

    audiovisualizer: fix improper video frame clear operation
    
    The current code is memsetting the GstVideoFrame.data address to 0s (which
    causes a segfault). This member is actually an array of data buffers (one for
    each plane).  This fix iterates over each data plane to clear them all.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695655

It actually occurred to me that it's still not 100% correct, but it will work fine for all cases we care about here (in case you wonder, it's e.g. possible that we are operating on a buffer that represents part of a larger picture, so has a large stride, and we shouldn't overwrite the 'padding' data then, because that's other parts of the image not exposed here; so really we need to clear line-by-line to accommodate that. And then memsetting 0s only works for RGB, not for YUV, say. But that can all be handled in a generic _clear_frame() function some day).

Committed same fix to copy in libvisual in -base as well:


commit c480bac5b7c9628f97d2459bbde534abd219e5c6
Author: Greg Rutz <greg@gsr-tek.com>
Date:   Mon Mar 11 21:55:28 2013 -0600

    libvisual: fix improper video frame clear operation
    
    The current code is memsetting the GstVideoFrame.data address to 0s (which
    causes a segfault). This member is actually an array of data buffers (one for
    each plane).  This fix iterates over each data plane to clear them all.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=695655