Created attachment 340071 [details] [review]
video-info: Catch overflows in the video frame size calculation

A moderate maximum size that we can handle is 16384x16384, which allows
for 64 bit per pixel without overflow. This is the same number as used
by ffmpeg.

This wouldn't have prevented https://scarybeastsecurity.blogspot.gr/2016/11/0day-poc-risky-design-decisions-in.html
unfortunately as gst_video_info_set_format() can't return a failure
(neither can align()) but would've given a warning at least.

We would have to check afterwards with e.g. a newly added
gst_video_info_is_valid().

This still has the possible problem of overflows caused by the stride. If we want to go this road, I would suggest that we check the actual calculations for overflows (the ones with the strides).

I'm not too happy with the fact that we can't currently make this have things fail gracefully without a crash though. Any other ideas?

In addition, we should also check *everywhere* where we calculate sizes (for malloc, etc) that the calculations don't overflow.

I have an actual image file that has width=24576, height=12288 fwiw ;)

See above ;) That would be covered if the checks are moved to the actual calculations

What I'm more interested in at this point is comments on how we can approach this properly. My best idea so far is this + a gst_video_info_validate().

We could probably just make set_format() return a gboolean, I think that should be fine with all ABIs we support, no? (There's precedent for that in Gtk IIRC.)

That sounds like a good approach, forgot about that. Thanks!

Let's go with that then, plus bubbling up of this kind of problem from other API

Created attachment 340632 [details] [review]
video-info: Sanity check the frame size to prevent overflows

Created attachment 340633 [details] [review]
video: Handle errors in gst_video_info_set_format() / gst_video_info_align()

Created attachment 340679 [details] [review]
video: Handle errors in gst_video_info_set_format() / gst_video_info_align()

Comment on attachment 340632 [details] [review]
video-info: Sanity check the frame size to prevent overflows

> /**
>  * gst_video_info_init:
>@@ -205,13 +205,15 @@ validate_colorimetry (GstVideoInfo * info)
>  * Note: This initializes @info first, no values are preserved. This function
>  * does not set the offsets correctly for interlaced vertically
>  * subsampled formats.
>+ *
>+ * Returns: %FALSE if the returned video info is invalid
>  */

Would be good to mention that it only has a return value since 1.12, and also make it more explicit why FALSE might be returned here.

>@@ -1100,8 +1115,10 @@ done:
>  *
>  * Extra padding will be added to the right side when stride alignment padding
>  * is required and @align will be updated with the new padding values.
>+ *
>+ * Returns: %FALSE if alignment could not be applie
>  */

typo - applied :)

Looks good in general. I'm sure there are still more corner cases, but this should be a good start.

Attachment 340632 [details] pushed as 17cdd36 - video-info: Sanity check the frame size to prevent overflows
Attachment 340679 [details] pushed as 681d97a - video: Handle errors in gst_video_info_set_format() / gst_video_info_align()

Were there unit tests as well somewhere? :)

In GIT now :)

Thanks ! Shall we output a compile time warning when this return is ignored ? (I believe GCC has support for that somehow).

(In reply to Nicolas Dufresne (stormer) from comment #16)
> Thanks ! Shall we output a compile time warning when this return is ignored
> ? (I believe GCC has support for that somehow).

Good idea in principle, but means that people who want to write code that still works with older gst versions will always get a warning, which seems unfortunate as well.