[libcamera-devel] [PATCH] libcamera: controls: guard ControlInfoMap against nullptr idmap_

[Mattijs Korpershoek]

It's possible to construct a Camera with an unsafe controlInfo_.
This is the case in the Simple pipeline, where the camera controls are
not populated.

With Simple, if we attempt to set a Control, we end up with a segfault
because the default constructor for ControlInfoMap doesn't
intialized idmap_ which is initialized at class declaration time as

  const ControlIdMap *idmap_ = nullptr;

Add some safeguards in ControlInfoMap to handle this case.

Link: http://codepad.org/CiLLcPNW
Link: https://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037439.html
Suggested-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
---
I have build-tested this against master and functionally tested on a
v0.0.4 android integration branch.

I've tested that i'm able to do a camera preview even when the
ScalerCrop control is unavailble.

Note that Jacopo already discussed alternative implementation by making
idmap_ an instance instead of a pointer, but that seemed not a good idea
either:

https://lists.libcamera.org/pipermail/libcamera-devel/2023-April/037439.html
---
 src/libcamera/controls.cpp | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 6dbf9b348709..b808116c01e5 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -677,6 +677,9 @@ ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)
 
 bool ControlInfoMap::validate()
 {
+	if (!idmap_)
+		return false;
+
 	for (const auto &ctrl : *this) {
 		const ControlId *id = ctrl.first;
 		auto it = idmap_->find(id->id());
@@ -719,6 +722,8 @@ bool ControlInfoMap::validate()
  */
 ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
 {
+	ASSERT(idmap_);
+
 	return at(idmap_->at(id));
 }
 
@@ -729,6 +734,8 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
  */
 const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
 {
+	ASSERT(idmap_);
+
 	return at(idmap_->at(id));
 }
 
@@ -739,6 +746,9 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
  */
 ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
 {
+	if (!idmap_)
+		return 0;
+
 	/*
 	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
 	 * entries, we can thus just count the matching entries in idmap to
@@ -755,6 +765,9 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
  */
 ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
 {
+	if (!idmap_)
+		return end();
+
 	auto iter = idmap_->find(id);
 	if (iter == idmap_->end())
 		return end();
@@ -770,6 +783,9 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
  */
 ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
 {
+	if (!idmap_)
+		return end();
+
 	auto iter = idmap_->find(id);
 	if (iter == idmap_->end())
 		return end();

---
base-commit: ac7511dc4c594f567ddff27ccc02c30bf6c00bfd
change-id: 20230404-guard-idmap-1d95f2100aca

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek at baylibre.com>