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

[Mattijs Korpershoek]

Hi Jacopo,

Thank you for your review.

On mer., avril 05, 2023 at 10:01, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:

> Hello Mattijs
>
> On Tue, Apr 04, 2023 at 06:11:16PM +0200, Mattijs Korpershoek via libcamera-devel wrote:
>> 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
>
> This won't stay here forever I presume, I'm not sure it should be in
> the git history. A minor detail, can be removed when applying

Since I'm spinning up a v2 with an added unit test, I will move this
codepad link to the cover letter instead.

>
>> 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>
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
>    j
>
>> ---
>> 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>
>>