| Message ID | 20230404-guard-idmap-v2-2-444d135b3895@baylibre.com |
|---|---|
| State | Accepted |
| Commit | 683c6da83f078d09fc020d2b48a4abe471853b2b |
| Headers | show |
| Series |
|
| Related | show |
Hi Mattijs, On Wed, Apr 05, 2023 at 10:14:31AM +0200, Mattijs Korpershoek wrote: > ControlInfoMap can be default-constructed. In that case, some of its > members (like idmap_) can be a nullptr, and ControlInfoMap.find() will segfault. > > Add a test with a default constructed ControlInfoMap to cover this. > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > test/controls/control_info_map.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/test/controls/control_info_map.cpp b/test/controls/control_info_map.cpp > index db95945a1580..29b33515e48c 100644 > --- a/test/controls/control_info_map.cpp > +++ b/test/controls/control_info_map.cpp > @@ -75,6 +75,13 @@ protected: > return TestFail; > } > > + /* Test looking up a control on a default-constructed infoMap */ > + const ControlInfoMap emptyInfoMap; > + if (emptyInfoMap.find(12345) != emptyInfoMap.end()) { > + cerr << "find() on empty ControlInfoMap failed" << endl; > + return TestFail; > + } > + > return TestPass; > } > }; > > -- > 2.39.2 >
Hi Mattijs On 4/5/23 1:44 PM, Mattijs Korpershoek via libcamera-devel wrote: > ControlInfoMap can be default-constructed. In that case, some of its > members (like idmap_) can be a nullptr, and ControlInfoMap.find() will segfault. > > Add a test with a default constructed ControlInfoMap to cover this. > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > test/controls/control_info_map.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/test/controls/control_info_map.cpp b/test/controls/control_info_map.cpp > index db95945a1580..29b33515e48c 100644 > --- a/test/controls/control_info_map.cpp > +++ b/test/controls/control_info_map.cpp > @@ -75,6 +75,13 @@ protected: > return TestFail; > } > > + /* Test looking up a control on a default-constructed infoMap */ > + const ControlInfoMap emptyInfoMap; > + if (emptyInfoMap.find(12345) != emptyInfoMap.end()) { > + cerr << "find() on empty ControlInfoMap failed" << endl; > + return TestFail; > + } > + > return TestPass; > } > }; >
Hello Mattijs, Thank you for the patch. On Wed, Apr 05, 2023 at 10:14:31AM +0200, Mattijs Korpershoek wrote: > ControlInfoMap can be default-constructed. In that case, some of its > members (like idmap_) can be a nullptr, and ControlInfoMap.find() will segfault. Not anymore with patch 1/2 :-) It's however best to add the test first, to verify it fails, and then the fix, to verify it fixes the problem. With patch 1/2 and 2/2 swapped, I would phrase the commit message of this patch as -------- ControlInfoMap can be default-constructed. In that case, some of its members (like idmap_) can be a nullptr. Add a test to ensure that the find() function doesn't segfault. The test is expected to fail, the issue will be fixed by a subsequent commit. -------- > Add a test with a default constructed ControlInfoMap to cover this. > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > --- > test/controls/control_info_map.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/test/controls/control_info_map.cpp b/test/controls/control_info_map.cpp > index db95945a1580..29b33515e48c 100644 > --- a/test/controls/control_info_map.cpp > +++ b/test/controls/control_info_map.cpp > @@ -75,6 +75,13 @@ protected: > return TestFail; > } > > + /* Test looking up a control on a default-constructed infoMap */ s/infoMap/infoMap./ With this, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + const ControlInfoMap emptyInfoMap; > + if (emptyInfoMap.find(12345) != emptyInfoMap.end()) { > + cerr << "find() on empty ControlInfoMap failed" << endl; > + return TestFail; > + } > + > return TestPass; > } > };
Hi Laurent, Quoting Laurent Pinchart via libcamera-devel (2023-04-20 11:22:30) > Hello Mattijs, > > Thank you for the patch. > > On Wed, Apr 05, 2023 at 10:14:31AM +0200, Mattijs Korpershoek wrote: > > ControlInfoMap can be default-constructed. In that case, some of its > > members (like idmap_) can be a nullptr, and ControlInfoMap.find() will segfault. > > Not anymore with patch 1/2 :-) It's however best to add the test first, > to verify it fails, and then the fix, to verify it fixes the problem. > With patch 1/2 and 2/2 swapped, I would phrase the commit message of > this patch as > > -------- > ControlInfoMap can be default-constructed. In that case, some of its > members (like idmap_) can be a nullptr. Add a test to ensure that the > find() function doesn't segfault. > > The test is expected to fail, the issue will be fixed by a subsequent > commit. > -------- Sorry I have merged this series already - just about the time you wrote your review. -- Kieran > > > Add a test with a default constructed ControlInfoMap to cover this. > > > > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > --- > > test/controls/control_info_map.cpp | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/test/controls/control_info_map.cpp b/test/controls/control_info_map.cpp > > index db95945a1580..29b33515e48c 100644 > > --- a/test/controls/control_info_map.cpp > > +++ b/test/controls/control_info_map.cpp > > @@ -75,6 +75,13 @@ protected: > > return TestFail; > > } > > > > + /* Test looking up a control on a default-constructed infoMap */ > > s/infoMap/infoMap./ > > With this, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + const ControlInfoMap emptyInfoMap; > > + if (emptyInfoMap.find(12345) != emptyInfoMap.end()) { > > + cerr << "find() on empty ControlInfoMap failed" << endl; > > + return TestFail; > > + } > > + > > return TestPass; > > } > > }; > > -- > Regards, > > Laurent Pinchart
diff --git a/test/controls/control_info_map.cpp b/test/controls/control_info_map.cpp index db95945a1580..29b33515e48c 100644 --- a/test/controls/control_info_map.cpp +++ b/test/controls/control_info_map.cpp @@ -75,6 +75,13 @@ protected: return TestFail; } + /* Test looking up a control on a default-constructed infoMap */ + const ControlInfoMap emptyInfoMap; + if (emptyInfoMap.find(12345) != emptyInfoMap.end()) { + cerr << "find() on empty ControlInfoMap failed" << endl; + return TestFail; + } + return TestPass; } };
ControlInfoMap can be default-constructed. In that case, some of its members (like idmap_) can be a nullptr, and ControlInfoMap.find() will segfault. Add a test with a default constructed ControlInfoMap to cover this. Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> --- test/controls/control_info_map.cpp | 7 +++++++ 1 file changed, 7 insertions(+)