[libcamera-devel] Fwd: New Defects reported by Coverity Scan for libcamera

Kieran Bingham kieran.bingham at ideasonboard.com
Mon Aug 16 16:26:42 CEST 2021 

Hi Jacopo,

On 12/08/2021 18:32, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Aug 12, 2021 at 03:57:25PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Your recent series has been run through coverity and has notified me of
>> the following warnings.
>>
>> They may be false positives, could you check please?
> 
> Thanks for pointing me to these warnings
> 
>>
>> (One day, I hope this will be a pre-merge check :D)
>>
>> --
>> Kieran
>>
>>
>>
>> -------- Forwarded Message --------
>> Subject: New Defects reported by Coverity Scan for libcamera
>> Date: Thu, 12 Aug 2021 14:32:52 +0000 (UTC)
>> From: scan-admin at coverity.com
>> To: kieran.bingham at ideasonboard.com
>>
>> Hi,
>>
>> Please find the latest report on new defect(s) introduced to libcamera
>> found with Coverity Scan.
>>
>> 3 new defect(s) introduced to libcamera found with Coverity Scan.
>> 1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
>> recent build analyzed by Coverity Scan.
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 3 of 3 defect(s)
>>
>>
>> ** CID 354658:  Error handling issues  (CHECKED_RETURN)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/src/libcamera/pipeline/ipu3/ipu3.cpp:
>> 568 in libcamera::PipelineHandlerIPU3::configure(libcamera::Camera *,
>> libcamera::CameraConfiguration *)()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 354658:  Error handling issues  (CHECKED_RETURN)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/src/libcamera/pipeline/ipu3/ipu3.cpp:
>> 568 in libcamera::PipelineHandlerIPU3::configure(libcamera::Camera *,
>> libcamera::CameraConfiguration *)()
>> 562     	V4L2DeviceFormat cio2Format;
>> 563     	ret = cio2->configure(sensorSize, &cio2Format);
>> 564     	if (ret)
>> 565     		return ret;
>> 566     567     	IPACameraSensorInfo sensorInfo;
>>>>>     CID 354658:  Error handling issues  (CHECKED_RETURN)
>>>>>     Calling "sensorInfo" without checking return value (as is done elsewhere 4 out of 5 times).
> 
> How does coverty work ? This has not been introduced by recent
> patches, so I assume it scans the whole code base not the single
> patches for issues ?

It runs as a wrapper around the entire build, and submits a report to
coverity servers. They then process the report and decide if the
'issues' are new or not - and store them in the database (which is
accessible at https://scan.coverity.com/projects/libcamera)

If there is a new issue highlighted, it generates a report which gets
sent to a fixed set of addresses, - it doesn't know who to 'blame' in
the event of a new issue - so I have been manually passing them on so far.

If this was existing code, or an existing issue - it could be that
something changing in this file has now made coverity match this as a
new or different issue

> Anyway, I would consider this a false postive, we should be guaranteed
> sensorInfo() is available at this point. A paranoid check won't hurt
> but it is not required imo.

If we know it's a false positive, we can mark it as such in the
database, and it will be closed ;-)



>> 568     	cio2->sensor()->sensorInfo(&sensorInfo);
>> 569     	data->cropRegion_ = sensorInfo.analogCrop;
>> 570     571     	/*
>> 572     	 * Configure the H/V flip controls based on the combination of
>> 573     	 * the sensor and user transform.
>>
>> ** CID 354657:  Uninitialized members  (UNINIT_CTOR)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/include/libcamera/controls.h:
>> 346 in libcamera::ControlInfoMap::ControlInfoMap()()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 354657:  Uninitialized members  (UNINIT_CTOR)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/include/libcamera/controls.h:
>> 346 in libcamera::ControlInfoMap::ControlInfoMap()()
>> 340     341     	const ControlIdMap &idmap() const { return *idmap_; }
>> 342     343     private:
>> 344     	bool validate();
>> 345     >>>     CID 354657:  Uninitialized members  (UNINIT_CTOR)
>>>>>     The compiler-generated constructor for this class does not initialize "idmap_".
> 
> This might be worth a patch, to initialize idmap_ to nullptr in
> construction.

I see you've handled that too.



> 
>> 346     	const ControlIdMap *idmap_;
>> 347     };
>> 348     349     class ControlList
>> 350     {
>> 351     private:
>>
>> ** CID 354656:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/src/ipa/ipu3/ipu3.cpp:
>> 158 in libcamera::ipa::ipu3::IPAIPU3::init(const libcamera::IPASettings
>> &, const libcamera::IPACameraSensorInfo &, const
>> libcamera::ControlInfoMap &, libcamera::ControlInfoMap *)()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 354656:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>> /home/kbingham/iob/libcamera/ci/libcamera-ci/src/libcamera/src/ipa/ipu3/ipu3.cpp:
>> 158 in libcamera::ipa::ipu3::IPAIPU3::init(const libcamera::IPASettings
>> &, const libcamera::IPACameraSensorInfo &, const
>> libcamera::ControlInfoMap &, libcamera::ControlInfoMap *)()
>> 152     		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
>> 153     		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
>> 154     	};
>> 155     156     	std::array<int64_t, 3> frameDurations;
>> 157     	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
>>>>>     CID 354656:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>>>     Potentially overflowing expression "lineLength * frameHeights[i]" with type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned).
> 
> Coverty complains because an expression evaluated in 32-bits
> arithmetic is assigned to a 64 bit value, and wonders if there's an
> overflow risk (as it assumes that if the programmer uses a 64 integers
> she's dealing with big numbers).
> 
> However with 2^32-1 pixels we should be safe in expressing all frame
> sizes a sensor expose. I would ignore this warning.

In that instance, why then is frameSize a uint64_t, should it be a uint32_t?


>
> Thanks
>   j
> 
>> 158     		uint64_t frameSize = lineLength * frameHeights[i];
>> 159     		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>> 160     	}
>> 161     162     	controls[&controls::FrameDurationLimits] =
>> ControlInfo(frameDurations[0],
>> 163     							       frameDurations[1],
>>
>>
>> ________________________________________________________________________________________________________
>> To view the defects in Coverity Scan visit,
>> https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrU0jHZztxHSfchEdEv-2BUERKUFDFlzchxJzxDB2S7OB7lr-2Fth-2F9ZTRF1ITl33bQQto-3DstHr_qXXIY1fr7HINeQlTFVXABgVX8iKf8Pc2W6xJCOtsiCw64f2YdDCRdgu4-2FkWCuqEX307P8ye63Todl9jRJ2Xtn4i-2Bm36Jvmv6406w9dVz8BbjRbl2qgf-2FyJp8dE4kYO8GLlIw18o0Uw8-2BN48glp1uz4dU2r-2BcP53GZf7poYFIzddKJZ4DHrEM7WI-2BnFo1BUkVw5UVUe-2BMfMC4lxKiWvcq0KuM478M7MdYsVyN-2BGZaxUs-3D
>>

More information about the libcamera-devel mailing list