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

Jacopo Mondi jacopo at jmondi.org
Mon Aug 16 16:46:31 CEST 2021 

Hi Kieran,

On Mon, Aug 16, 2021 at 03:26:42PM +0100, Kieran Bingham wrote:
> 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 ;-)

I think we can, yes..

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

I guess because it's then used to compute frameDurations[i] which, by
the definition of the control, it's an int64_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