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
>
> 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@xxxxxxxxxx>
> > ---
> > 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
