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

[Jacopo Mondi]

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 ?

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.


> 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.

> 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.

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
>