[libcamera-devel,1/2] gstreamer: Check if Stream configurations were generated correctly
diff mbox series

Message ID 20211001161100.1405576-1-javierm@redhat.com
State Accepted
Headers show
Series
  • [libcamera-devel,1/2] gstreamer: Check if Stream configurations were generated correctly
Related show

Commit Message

Javier Martinez Canillas Oct. 1, 2021, 4:10 p.m. UTC 
Currently, the gst_libcamera_device_new() function assumes that a call to
Camera::generateConfiguration() will always succeed, but that may not be
the case and the return value must to be checked.

Otherwise, could lead to a NULL pointer deference if the pipeline handler
failed to generate a configuration for the VideoRecording stream role.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 src/gstreamer/gstlibcameraprovider.cpp | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Nicolas Dufresne Oct. 1, 2021, 4:36 p.m. UTC | #1 
Le vendredi 01 octobre 2021 à 18:10 +0200, Javier Martinez Canillas a écrit :
> Currently, the gst_libcamera_device_new() function assumes that a call to
> Camera::generateConfiguration() will always succeed, but that may not be
> the case and the return value must to be checked.
> 
> Otherwise, could lead to a NULL pointer deference if the pipeline handler
          *this*                          dereference
> failed to generate a configuration for the VideoRecording stream role.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
> 
>  src/gstreamer/gstlibcameraprovider.cpp | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> index aee6f9a2be2..6eb0a0eb0c4 100644
> --- a/src/gstreamer/gstlibcameraprovider.cpp
> +++ b/src/gstreamer/gstlibcameraprovider.cpp
> @@ -132,6 +132,10 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
>  
>  	roles.push_back(StreamRole::VideoRecording);
>  	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> +	if (!config || config->size() != roles.size()) {
> +		GST_ERROR("Failed to generate a default configuration for %s", name);
> +		return nullptr;
> +	}
>  
>  	for (const StreamConfiguration &stream_cfg : *config) {
>  		GstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> @@ -189,8 +193,16 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
>  
>  	for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
>  		GST_INFO_OBJECT(self, "Found camera '%s'", camera->id().c_str());
> +
> +		GstDevice *dev = gst_libcamera_device_new(camera);
> +		if (!dev) {
> +			GST_ERROR_OBJECT(self, "Failed to add camera '%s'",
> +					 camera->id().c_str());
> +			return nullptr;
> +		}
> +
>  		devices = g_list_append(devices,
> -					g_object_ref_sink(gst_libcamera_device_new(camera)));
> +					g_object_ref_sink(dev));
>  	}
>  
>  	return devices;
Laurent Pinchart Oct. 12, 2021, 2:38 a.m. UTC | #2 
Hi Javier,

Thank you for the patch.

On Fri, Oct 01, 2021 at 12:36:23PM -0400, Nicolas Dufresne wrote:
> Le vendredi 01 octobre 2021 à 18:10 +0200, Javier Martinez Canillas a écrit :
> > Currently, the gst_libcamera_device_new() function assumes that a call to
> > Camera::generateConfiguration() will always succeed, but that may not be
> > the case and the return value must to be checked.
> > 
> > Otherwise, could lead to a NULL pointer deference if the pipeline handler
>           *this*                          dereference

I'll fix this when applying.

> > failed to generate a configuration for the VideoRecording stream role.
> > 
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > ---
> > 
> >  src/gstreamer/gstlibcameraprovider.cpp | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > index aee6f9a2be2..6eb0a0eb0c4 100644
> > --- a/src/gstreamer/gstlibcameraprovider.cpp
> > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > @@ -132,6 +132,10 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
> >  
> >  	roles.push_back(StreamRole::VideoRecording);
> >  	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> > +	if (!config || config->size() != roles.size()) {
> > +		GST_ERROR("Failed to generate a default configuration for %s", name);
> > +		return nullptr;
> > +	}
> >  
> >  	for (const StreamConfiguration &stream_cfg : *config) {
> >  		GstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> > @@ -189,8 +193,16 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
> >  
> >  	for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> >  		GST_INFO_OBJECT(self, "Found camera '%s'", camera->id().c_str());
> > +
> > +		GstDevice *dev = gst_libcamera_device_new(camera);
> > +		if (!dev) {
> > +			GST_ERROR_OBJECT(self, "Failed to add camera '%s'",
> > +					 camera->id().c_str());
> > +			return nullptr;
> > +		}
> > +
> >  		devices = g_list_append(devices,
> > -					g_object_ref_sink(gst_libcamera_device_new(camera)));
> > +					g_object_ref_sink(dev));
> >  	}
> >  
> >  	return devices;
Laurent Pinchart Oct. 12, 2021, 2:47 a.m. UTC | #3 
On Tue, Oct 12, 2021 at 05:39:00AM +0300, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Fri, Oct 01, 2021 at 12:36:23PM -0400, Nicolas Dufresne wrote:
> > Le vendredi 01 octobre 2021 à 18:10 +0200, Javier Martinez Canillas a écrit :
> > > Currently, the gst_libcamera_device_new() function assumes that a call to
> > > Camera::generateConfiguration() will always succeed, but that may not be
> > > the case and the return value must to be checked.
> > > 
> > > Otherwise, could lead to a NULL pointer deference if the pipeline handler
> >           *this*                          dereference
> 
> I'll fix this when applying.

And I'll of course push v2, not v1.

> > > failed to generate a configuration for the VideoRecording stream role.
> > > 
> > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > 
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > ---
> > > 
> > >  src/gstreamer/gstlibcameraprovider.cpp | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > > index aee6f9a2be2..6eb0a0eb0c4 100644
> > > --- a/src/gstreamer/gstlibcameraprovider.cpp
> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > > @@ -132,6 +132,10 @@ gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
> > >  
> > >  	roles.push_back(StreamRole::VideoRecording);
> > >  	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> > > +	if (!config || config->size() != roles.size()) {
> > > +		GST_ERROR("Failed to generate a default configuration for %s", name);
> > > +		return nullptr;
> > > +	}
> > >  
> > >  	for (const StreamConfiguration &stream_cfg : *config) {
> > >  		GstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> > > @@ -189,8 +193,16 @@ gst_libcamera_provider_probe(GstDeviceProvider *provider)
> > >  
> > >  	for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> > >  		GST_INFO_OBJECT(self, "Found camera '%s'", camera->id().c_str());
> > > +
> > > +		GstDevice *dev = gst_libcamera_device_new(camera);
> > > +		if (!dev) {
> > > +			GST_ERROR_OBJECT(self, "Failed to add camera '%s'",
> > > +					 camera->id().c_str());
> > > +			return nullptr;
> > > +		}
> > > +
> > >  		devices = g_list_append(devices,
> > > -					g_object_ref_sink(gst_libcamera_device_new(camera)));
> > > +					g_object_ref_sink(dev));
> > >  	}
> > >  
> > >  	return devices;

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
index aee6f9a2be2..6eb0a0eb0c4 100644
--- a/src/gstreamer/gstlibcameraprovider.cpp
+++ b/src/gstreamer/gstlibcameraprovider.cpp
@@ -132,6 +132,10 @@  gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
 
 	roles.push_back(StreamRole::VideoRecording);
 	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
+	if (!config || config->size() != roles.size()) {
+		GST_ERROR("Failed to generate a default configuration for %s", name);
+		return nullptr;
+	}
 
 	for (const StreamConfiguration &stream_cfg : *config) {
 		GstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
@@ -189,8 +193,16 @@  gst_libcamera_provider_probe(GstDeviceProvider *provider)
 
 	for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
 		GST_INFO_OBJECT(self, "Found camera '%s'", camera->id().c_str());
+
+		GstDevice *dev = gst_libcamera_device_new(camera);
+		if (!dev) {
+			GST_ERROR_OBJECT(self, "Failed to add camera '%s'",
+					 camera->id().c_str());
+			return nullptr;
+		}
+
 		devices = g_list_append(devices,
-					g_object_ref_sink(gst_libcamera_device_new(camera)));
+					g_object_ref_sink(dev));
 	}
 
 	return devices;