| Message ID | 20201021024744.19047-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 21/10/2020 03:47, Laurent Pinchart wrote: > When setting (or trying) a format with a multiplanar device, the > V4L2VideoDevice::trySetFormatMeta() function iterates over all planes > available in the V4L2DeviceFormat structure. The caller is responsible > for setting the plane count, and failure to do so properly may result in > memory corruption. This can lead to a crash way after the function > returns, making the problem difficult to debug. > > As the issue is caused by a bug in the caller, use an assertion to catch > it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Sounds reasonable to me, I wonder if you've hit this ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/v4l2_videodevice.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 16162e1edba3..3ba9e5ba134a 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -861,6 +861,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > pix->num_planes = format->planesCount; > pix->field = V4L2_FIELD_NONE; > > + ASSERT(pix->num_planes <= ARRAY_SIZE(pix->plane_fmt)); > + > for (unsigned int i = 0; i < pix->num_planes; ++i) { > pix->plane_fmt[i].bytesperline = format->planes[i].bpl; > pix->plane_fmt[i].sizeimage = format->planes[i].size; >
Hi Laurent, Thanks for your work. On 2020-10-21 05:47:43 +0300, Laurent Pinchart wrote: > When setting (or trying) a format with a multiplanar device, the > V4L2VideoDevice::trySetFormatMeta() function iterates over all planes > available in the V4L2DeviceFormat structure. The caller is responsible > for setting the plane count, and failure to do so properly may result in > memory corruption. This can lead to a crash way after the function > returns, making the problem difficult to debug. > > As the issue is caused by a bug in the caller, use an assertion to catch > it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/v4l2_videodevice.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 16162e1edba3..3ba9e5ba134a 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -861,6 +861,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > pix->num_planes = format->planesCount; > pix->field = V4L2_FIELD_NONE; > > + ASSERT(pix->num_planes <= ARRAY_SIZE(pix->plane_fmt)); > + > for (unsigned int i = 0; i < pix->num_planes; ++i) { > pix->plane_fmt[i].bytesperline = format->planes[i].bpl; > pix->plane_fmt[i].sizeimage = format->planes[i].size; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Wed, Oct 21, 2020 at 10:58:46AM +0100, Kieran Bingham wrote: > On 21/10/2020 03:47, Laurent Pinchart wrote: > > When setting (or trying) a format with a multiplanar device, the > > V4L2VideoDevice::trySetFormatMeta() function iterates over all planes > > available in the V4L2DeviceFormat structure. The caller is responsible > > for setting the plane count, and failure to do so properly may result in > > memory corruption. This can lead to a crash way after the function > > returns, making the problem difficult to debug. > > > > As the issue is caused by a bug in the caller, use an assertion to catch > > it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Sounds reasonable to me, I wonder if you've hit this ... How did you guess ? :-) It lead to a corrupted stack, so gdb was not helpful. I wanted to make sure the next person to hit this issue won't have a too hard time. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > src/libcamera/v4l2_videodevice.cpp | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 16162e1edba3..3ba9e5ba134a 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -861,6 +861,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > pix->num_planes = format->planesCount; > > pix->field = V4L2_FIELD_NONE; > > > > + ASSERT(pix->num_planes <= ARRAY_SIZE(pix->plane_fmt)); > > + > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > pix->plane_fmt[i].bytesperline = format->planes[i].bpl; > > pix->plane_fmt[i].sizeimage = format->planes[i].size;
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 16162e1edba3..3ba9e5ba134a 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -861,6 +861,8 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) pix->num_planes = format->planesCount; pix->field = V4L2_FIELD_NONE; + ASSERT(pix->num_planes <= ARRAY_SIZE(pix->plane_fmt)); + for (unsigned int i = 0; i < pix->num_planes; ++i) { pix->plane_fmt[i].bytesperline = format->planes[i].bpl; pix->plane_fmt[i].sizeimage = format->planes[i].size;
When setting (or trying) a format with a multiplanar device, the V4L2VideoDevice::trySetFormatMeta() function iterates over all planes available in the V4L2DeviceFormat structure. The caller is responsible for setting the plane count, and failure to do so properly may result in memory corruption. This can lead to a crash way after the function returns, making the problem difficult to debug. As the issue is caused by a bug in the caller, use an assertion to catch it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/v4l2_videodevice.cpp | 2 ++ 1 file changed, 2 insertions(+)