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

[paul.elder at ideasonboard.com]

Hi Vedant,

On Wed, Jun 02, 2021 at 02:26:40AM +0530, Vedant Paranjape wrote:
> Error checking the output from generateConfiguration() was missing in
> the code. Only assert was added as a guard which checked if the size of
> generated camera config was equal to size of roles passed to it.

Maybe "The return value from generateConfiguration() was not checked" ?

> 
> If the roles argument has a invalid member, it will return a nullptr and then

It's not really about roles having an invalid member, it's about the
camera not supporting a requested role.

> 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.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.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());

I thought the whole point of adding the error checking is to remove this
assert?

If the return value is not nullptr, then the number of
StreamConfigurations in the CameraConfiguration is guaranteed to be the
same as the number of roles that were requested. If any role is not
supported, then nullptr will be returned. So this assertion, if reached,
will always be true.


Paul

>  
>  	for (gsize i = 0; i < state->srcpads_.size(); i++) {
> -- 
> 2.25.1
>