GNOME Bugzilla – Bug 769765
v4l2: Sanitise buffer sizes provided by v4l2
Last modified: 2016-11-04 17:39:55 UTC
Created attachment 333134 [details] [review] patch If the error flag is set we can't trust the rest of the fields in the `v4l2_buffer` struct. In particular I've seen problems where the `bytesused` field of `v4l2_buffer` would be a silly number causing the later call to: gst_memory_resize (group->mem[i], 0, group->planes[i].bytesused); to result in this error to be printed: (pulsevideo:11): GStreamer-CRITICAL **: gst_memory_resize: assertion 'size + mem->offset + offset <= mem->maxsize' failed besides causing who-knows what other problems. We reenqueue these buffers and return `GST_V4L2_FLOW_CORRUPTED_BUFFER` so this will not be a fatal error and dequeue will be attempted once again by the loop in `gst_v4l2_buffer_pool_dqbuf`. The invalid `v4l2_buffer` I saw from my capture device was: buffer = { index = 0, type = 1, bytesused = 534748928, // <- Invalid flags = 8260, // V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC | V4L2_BUF_FLAG_ERROR | V4L2_BUF_FLAG_DONE field = 01330, // <- Invalid timestamp = { tv_sec = 0, tv_usec = 0 }, timecode = { type = 0, flags = 0, frames = 0 '\000', seconds = 0 '\000', minutes = 0 '\000', hours = 0 '\000', userbits = "\000\000\000" }, sequence = 0, memory = 2, m = { offset = 3537219584, userptr = 140706665836544, // Could be nonsense, not sure planes = 0x7ff8d2d5b000, fd = -757747712 }, length = 2764800, reserved2 = 0, reserved = 0 } This is from gdb with my own annotations added. Tested with gst-plugins-good 1.8.1, a Magewell XI100DUSB-HDMI video capture device and kernel 3.13 using a dodgy HDMI cable which is great at breaking HDMI capture devices. I'm using io-mode=userptr and have built gst-plugins-good without libv4l.
Review of attachment 333134 [details] [review]: The V4L2 doc don't agree, since the error flag means corruption in the produced data. I think we should instead validate the size, if it's smaller then expected, then I'd use the code path where it drops. In decoder scenario, you'll have the error flag on each buffer the follow a drop, deciding to drop of not those is implementation dependant.
Will you update your patch ?
Ping.
Will do, just a little busy at the moment. I'll get back to you :).
Created attachment 337242 [details] [review] patch Ok, I've reworked this patch. I won't have a chance to test it until after the conference when I can get back to my hardware.
Review of attachment 337242 [details] [review]: Looks good to me, let me know when you are done with the test. I'm marking it as commit after freeze, should give you enough time. I think it's a good candidate for backports (1.8/1.10).
commit 56b1d088a9a6c69f8c1f3067bc261e88632fae4a Author: William Manley <will@williammanley.net> Date: Sat Oct 8 18:11:17 2016 +0200 v4l2: Warn, don't assert if v4l gives us a buffer with a too large size I've seen problems where the `bytesused` field of `v4l2_buffer` would be a silly number causing the later call to: gst_memory_resize (group->mem[i], 0, group->planes[i].bytesused); to result in this error to be printed: (pulsevideo:11): GStreamer-CRITICAL **: gst_memory_resize: assertion 'size + mem->offset + offset <= mem->maxsize' failed besides causing who-knows what other problems.