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

[paul.elder at ideasonboard.com]

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.

> ---
>  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
>