Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check

[Sakari Ailus]

On Thu, Jun 24, 2021 at 01:32:38PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 24 Jun 2021 13:14:43 +0300
> Sakari Ailus <sakari.ailus@xxxxxx> escreveu:
> 
> > Hi Mauro,
> > 
> > On Thu, Jun 24, 2021 at 11:59:25AM +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 24 Jun 2021 12:31:53 +0300
> > > Sakari Ailus <sakari.ailus@xxxxxx> escreveu:
> > >   
> > > > Hi Mauro,
> > > > 
> > > > Could you check if your mail client could be configured not to add junk to
> > > > To: field? It often leads anything in the Cc: field being dropped.  
> > > 
> > > I have no idea why it is doing that. I'm just using git send-email
> > > here. Perhaps a git bug?
> > > 
> > > 	$ git --version
> > > 	git version 2.31.1
> > > 
> > > The setup is like this one:
> > > 
> > > 	[sendemail]
> > > 	        confirm = always
> > > 	        multiedit = true
> > > 	        chainreplyto = false
> > > 	        aliasesfile = /home/mchehab/.addressbook
> > > 	        aliasfiletype = pine
> > > 	        assume8bitencoding = UTF-8  
> > 
> > I tried sending a message to myself using git send-email with an empty To:
> > field and it came through just fine, with To: field remaining empty. I
> > wonder if it could be the list server?
> 
> It seems so.
> 
> > > So, this is not a false-positive, but, instead, a real issue.
> > > 
> > > So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> > > to explicitly check it, and return an error, e. g.:
> > > 
> > > 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > > 	{
> > >         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > >         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > 		struct led_classdev *led_cdev;
> > >         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> > >         	bool external_strobe;
> > >         	int ret = 0;
> > > 
> > > 		if (!fled_cdev)
> > > 			return -EINVAL;  
> > 
> > The approach is correct, but as noted, the check needs to be done later.
> 
> > I checked that the same pattern is used throughout much of the file. I
> > suppose if smatch isn't happy with this instance, it may not be happy with
> > the rest either. Admittedly, the pattern isn't entirely trouble-free, as it
> > requires the parts of the file to be in sync.
> >
> > Addressing this takes probably a few patches at least.
> 
> See, the main issue is not the smatch report, but the point that, on
> some cases, it will de-reference a NULL pointer.

It does not, since the controls aren't added for devices that do not have
these parts to control. For instance, if there's no flash LED, the flash
related controls aren't created. So this is primariy a static checker
issue, and secondarily perhaps an issue of the cleanness of the code. But
much of that originates from how the LED flash API connects with the V4L2
flash API.

> 
> And yeah, the same pattern is everywhere within the core.
> 
> IMO, the right fix would be to ensure that fled_cdev will always
> be not NULL, but if there are good reasons why this can't happen,
> extra checks are needed along the core (or at leds core), in order
> to prevent de-referencing NULL pointers.
> 
> > 
> > Could you drop this patch, please?
> 
> Just dropped from media_stage. It didn't reach media_tree.

Thanks.

-- 
Sakari Ailus