lkml.org 
[lkml]   [2021]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check
Em Thu, 24 Jun 2021 12:31:53 +0300
Sakari Ailus <sakari.ailus@iki.fi> 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


>
> On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > As pointed by smatch:
> > drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> >
> > It is too late to check if fled_cdev is NULL there. If such check is
> > needed, it should be, instead, inside v4l2_flash_init().
> >
> > On other words, if v4l2_flash->fled_cdev() is NULL at
> > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > would try to de-reference a NULL pointer, as the logic won't prevent
> > it.
> >
> > So, remove the useless check.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > index 10ddcc48aa17..a1653c635d82 100644
> > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > @@ -194,7 +194,7 @@ 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 = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > + struct led_classdev *led_cdev = &fled_cdev->led_cdev;
>
> fled_cdev may be NULL here. The reason is that some controls are for flash
> LEDs only but the same sub-device may also control an indicator. This is
> covered when the controls are created, so that the NULL pointer isn't
> dereferenced.

I double-checked the code: if a a NULL pointer is passed, the calls
to the leds framework will try to de-reference it or will return an
error.

For instance, those will return an error:

static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
bool state)
{
if (!fled_cdev)
return -EINVAL;
return fled_cdev->ops->strobe_set(fled_cdev, state);
}

#define call_flash_op(fled_cdev, op, args...) \
((has_flash_op(fled_cdev, op)) ? \
(fled_cdev->ops->op(fled_cdev, args)) : \
-EINVAL)

No big issue here (except that the function will do nothing but
return an error).

However, there are places that it will cause it to de-reference
a NULL pointer:

int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
{
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
return -EBUSY;

led_cdev->brightness = min(value, led_cdev->max_brightness);

if (led_cdev->flags & LED_SUSPENDED)
return 0;

return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
}

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;

led_cdev = &fled_cdev->led_cdev;

...

>
> If you wish the false positive to be addressed while also improving the
> implementation, that could be done by e.g. splitting the switch into two,
> the part that needs fled_cdev and another that doesn't.
>
> I can send a patch for that.
>
> Please also cc me to V4L2 flash class patches. I noticed this one by
> accident only.

Better to add you as a reviewer at the MAINTAINERS file, to
ensure that you'll always be c/c on such code.

Thanks,
Mauro

\
 
 \ /
  Last update: 2021-06-24 12:00    [W:0.467 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site