[libcamera-devel] [PATCH v4] gstreamer: Add error checking in gst_libcamera_src_task_enter()

[Nicolas Dufresne]

Le jeudi 03 juin 2021 à 14:17 +0900, paul.elder at ideasonboard.com a écrit :
> Hi Vedant,
> 
> On Wed, Jun 02, 2021 at 07:41:38PM +0530, Vedant Paranjape wrote:
> > The return value from generateConfiguration() was not checked. Only assert
> > was added as a guard which checked if the size of generated camera config was
> 
> s/size of/size of the/
> 
> > equal to size of roles passed to it.
> > 
> > If the roles variable has an invalid/unsupported role, it will return
> > a nullptr and then trying to access a member on a nullptr for size comparison
> > will result in a segmentation fault. So, if the function returns a nullptr,
> > it will simply push an error message on GstBus and gracefully exit.
> 
> s/it will simply/simply/
> 
> What you did in the patch should be stated imperatively.
> 
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> I'll wait for Nicolas to re-review this patch and then I'll push with
> those changes applied.

Was lost from V3,

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

> 
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 87246b40..ccc61590 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -375,10 +375,13 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> >  
> >  	/* Generate the stream configurations, there should be one per pad. */
> >  	state->config_ = state->cam_->generateConfiguration(roles);
> > -	/*
> > -	 * \todo Check if camera may increase or decrease the number of streams
> > -	 * regardless of the number of roles.
> > -	 */
> > +	if (state->config_ == nullptr) {
> > +		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > +				  ("Failed to generate camera configuration from roles"),
> > +				  ("Camera::generateConfiguration() returned nullptr"));
> > +		gst_task_stop(task);
> > +		return;
> > +	}
> >  	g_assert(state->config_->size() == state->srcpads_.size());
> >  
> >  	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> > -- 
> > 2.25.1
> >