[libcamera-devel] [PATCH] libcamera: ipa_module: prevent uninitialised access

[Kieran Bingham]

On 18/07/2019 15:04, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Jul 18, 2019 at 06:06:17AM +0100, Kieran Bingham wrote:
>> The IPAModule::loadIPAModuleInfo() function includes a *data pointer
>> which is used as a null-pointer comparison in the error path with a
>> conditional statement of "if (ret || !data)".
>>
>> The data variable is not initialised, and a single error path evaluates
>> this as "if (true || uninitialised)".
>>
>> Whilst this error path does not incorrectly utilise the uninitialised
>> data, as the ret evaluates to true already, it does leave a statement
>> which includes an uninitialised variable.
>>
>> Help the static anlaysers by initialising the data variable when it is

s/anlaysers/analysers/

>> defined.
> 
> Have you found this with any static initialiser ? Does valgrind report
> this issue ?

These issues were found with clang-analyser. That was going to be
detailed in the cover letter that I didn't write :-D

There's one more fault in the options parsing code shared between cam
and qcam, but I need to look deeper into the cause / reasoning of that one.

I don't think valgrind would report this issue, as I don't think it will
be an issue at runtime. The expression simply evaluates as true
regardless of the uninitialised data, because the only case it can occur
is when ret is already non-zero.


>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/ipa_module.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
>> index 003611625214..2ddb02c1562e 100644
>> --- a/src/libcamera/ipa_module.cpp
>> +++ b/src/libcamera/ipa_module.cpp
>> @@ -291,7 +291,7 @@ int IPAModule::loadIPAModuleInfo()
>>  		return ret;
>>  	}
>>  
>> -	void *data;
>> +	void *data = NULL;
> 
> This should be nullptr.

Indeed it should!


> 
>>  	size_t dataSize;
>>  	void *map;
>>  	size_t soSize;
> 

-- 
Regards
--
Kieran