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

[Nicolas Dufresne]

Hi Vedant,

thanks for the patch.

Le mercredi 02 juin 2021 à 00:54 +0530, Vedant Paranjape a écrit :
> 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.
> 
> If the roles argument has a invalid member, 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 put
                                                                            push
> a error on GstBus and gracefully exit.
  an error message
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 8b6057df..910c0f83 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -375,9 +375,9 @@ 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);
> -	if (state->config_ == nullptr) {
> +	if (state->config_ == nullptr && state->config_->size() == state->srcpads_.size()) {

If state->config_ is null, then the right part will be executed (true &&
right_part) and will crash due to nullptr deference.

I'd drop this patch, and resend v1 with the assert kept but placed after the
nullptr handling branch.

>  		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> -				  ("Failed to generate camera configuration from roles"),
> +				  ("Failed to generate camera configuration from"),

I agree the initial message was vague, but how the removing "roles" word helps ?

>  				  ("Camera::generateConfiguration() returned nullptr"));
>  		gst_task_stop(task);
>  		return;