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

[Laurent Pinchart]

Hi Vedant,

On Wed, Jun 02, 2021 at 12:57:26AM +0530, Vedant Paranjape wrote:
> Hey,
> I think I messed up with the commit history in this patch, do you want me
> to rebase the commit in v1 and v2 together and send a new patch ?

Please do. As a general rule, patches should be based on the master
branch of libcamera. In this particular case, we won't apply v1 (given
that it didn't pass the review step), so v2 has to be self-contained.
There are multiple reasons for that, two major ones are to avoid
bisection breakages (if v1 introduces a problem that gets noticed during
review, applying v1 and a fix on top of it would cause breakages when
testing the version with v1 without the fix), and to show the whole
context during review (if I were to review this v2, I would need to
first apply your v1, then v2, and review the result).

> On Wed, Jun 2, 2021 at 12:54 AM 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.
> >
> > 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
> > a error on GstBus and gracefully exit.
> >
> > 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()) {
> >                 GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > -                                 ("Failed to generate camera configuration from roles"),
> > +                                 ("Failed to generate camera configuration from"),
> >                                   ("Camera::generateConfiguration() returned nullptr"));
> >                 gst_task_stop(task);
> >                 return;

-- 
Regards,

Laurent Pinchart