| Message ID | 20210602141138.582472-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1e5cee701781276fb03f18e704a5a6ea8f24eff8 |
| Headers | show |
| Series |
|
| Related | show |
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@gmail.com> Reviewed-by: Paul Elder <paul.elder@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 >
Le jeudi 03 juin 2021 à 14:17 +0900, paul.elder@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@gmail.com> > > Reviewed-by: Paul Elder <paul.elder@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@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 > >
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++) {
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 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. Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com> --- src/gstreamer/gstlibcamerasrc.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)