GNOME Bugzilla – Bug 754429
basetextoverlay: crash in basetextoverlay for padding greater than video size
Last modified: 2015-09-03 14:26:20 UTC
gst-launch-1.0 videotestsrc num-buffers=20 ! clockoverlay ypad=300 ! videoconvert ! autovideosink The above pipeline crash because basetextoverlay try to allocate very large amount of memory. The amount of memory to be allocted is calculated from the text image width and height. If the xpad or ypad value is greater than the video size, text image height/width value became -ve. Because of the -ve value used in the memory size, it try to allocate very large block of memory and crash. basetextoverly should avoid the drawing if padding is larger than the video size. basetextoverly is already handling this logic but it couldn't proceed because of the crash while allocating memory for textimage.
Created attachment 310458 [details] [review] basetextoverlay: FIX crash if padding greater than video size Making width/height of textimage to 1px to avoid crash because of -ve width/height of image.
Comment on attachment 310458 [details] [review] basetextoverlay: FIX crash if padding greater than video size Thanks! :) This seems more like a workaround than a fix. If the overlay is completely outside the video frame, it should not be rendered at all and have width=height=0 instead of having a 1x1 pixel overlay somewhere. That should be possible by just skipping some code, or doing an early return, if width or height is <= 0. Can you update the patch?
Created attachment 310464 [details] [review] basetextoverlay: FIX crash if padding greater than video size updated patch. Skipping rendering of text image if overlay is completely outside video frame. please review it.
Comment on attachment 310464 [details] [review] basetextoverlay: FIX crash if padding greater than video size This looks better (and simpler) :) I'm just not sure if the checks should actually be moved a bit further down, are at multiple places, or unscaled_* should also be checked. I didn't fully follow the code, but I saw that width and height are modified further before the cairo_matrix_init_scale() and could possibly become at least 0 (not sure if negative) there. I think we need to make sure that at no point we're calculating with negative numbers (and if they happen we can just ignore rendering), and if width or height are 0 before the cairo_matrix_init_scale() we can also stop rendering anything.
Review of attachment 310458 [details] [review]: Why don't we skip the rendering when out of frame ?
Ah, never mind ;-P
Prashant, do you want to update your patch?
Created attachment 310561 [details] [review] basetextoverlay: FIX crash if padding greater than video size Thanks for the review. Updated patch to skip rendering after checking for width, height and unscaled width, height for values <=0.
Comment on attachment 310561 [details] [review] basetextoverlay: FIX crash if padding greater than video size Nicolas, can you double check if this is correct, or if further checks are needed above this to ensure that none of the calculations go out of range? You worked on this code last, so probably remember what it is doing :)
Review of attachment 310561 [details] [review]: Checking both width/height and unscaled_widht/height is redundant, but not faulty (it's extra safe). Otherwise the patch is correct. Obviously a unit test in tests/check/element/textoverlay.c could be nice (so we have a test that crash before, and works now).
Comment on attachment 310561 [details] [review] basetextoverlay: FIX crash if padding greater than video size And I'm fine in commiting this now btw.
Attachment 310561 [details] pushed as 7447736 - basetextoverlay: FIX crash if padding greater than video size
Thanks.