| Message ID | 20210601192405.143591-1-vedantparanjape160201@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 ? Regards, *Vedant Paranjape* On Wed, Jun 2, 2021 at 12:54 AM Vedant Paranjape < vedantparanjape160201@gmail.com> 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@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; > -- > 2.25.1 > >
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@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;
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@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;
Hi, Got it. I'll send a completely new patch by merging the two commits on my local repo using git rebase. Also, let me know if the commit message looks good to you. I did changes according to your comments on the v1 patch Regards, Vedant Paranjape On Wed, 2 Jun, 2021, 01:41 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > 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@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 >
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;
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@gmail.com> --- src/gstreamer/gstlibcamerasrc.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)