linux-kernel - Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCt5OZ8dAvZ7G_fic6eCaCsGvmnprjmG9p_9kLbv9cX76A@mail.gmail.com>
Date: Tue, 1 Jul 2025 21:25:59 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Desnes Nunes <desnesn@...hat.com>, hansg@...nel.org, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

On Tue, 1 Jul 2025 at 20:42, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Tue, Jul 01, 2025 at 02:20:53PM -0300, Desnes Nunes wrote:
> > On Tue, Jul 1, 2025 at 1:48 PM Ricardo Ribalda <ribalda@...omium.org> wrote:
> > > On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@...hat.com> wrote:
> > > >
> > > > This avoids a variable loop shadowing occurring between the local loop
> > > > iterating through the uvc_entity's controls and the global one going
> > > > through the pending async controls of the file handle
> > > >
> > > > Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
> > > If you add a fixes you need to add
> > > Cc: stable@...nel.org
> >
> > Thanks for letting me know
> >
> > > Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > Signed-off-by: Desnes Nunes <desnesn@...hat.com>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 44b6513c5264..91cc874da798 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> > > >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > >  {
> > > >         struct uvc_entity *entity;
> > > > -       int i;
> > > >
> > > >         guard(mutex)(&handle->chain->ctrl_mutex);
> > > >
> > > > @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > >         if (!WARN_ON(handle->pending_async_ctrls))
> > > >                 return;
> > > >
> > > > -       for (i = 0; i < handle->pending_async_ctrls; i++)
> > >
> > > nitpick: I would have called the variable i, not j.  For me j usually
> > > means nested loop. But up to you
> >
> > Noted - I used a different variable name because I wanted to
> > differentiate the loops.
>
> Variable declaration in the loop statement is relatively new in the
> kernel, so there's no consensus yet (to my knowledge) on clear coding
> styles, but I would have simply used the same variable name in both
> loops, with two separate declarations:
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 303b7509ec47..6b9486749c3f 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -3299,7 +3299,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  {
>         struct uvc_entity *entity;
> -       int i;
>
>         guard(mutex)(&handle->chain->ctrl_mutex);
>
> @@ -3317,7 +3316,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>         if (!WARN_ON(handle->pending_async_ctrls))
>                 return;
>
> -       for (i = 0; i < handle->pending_async_ctrls; i++)
> +       for (unsigned int i = 0; i < handle->pending_async_ctrls; i++)
>                 uvc_pm_put(handle->stream->dev);
>  }
>
> Is there a downside to this ?

The toolchain that they are using does not seem to like it, and there
is no benefit for having two initialization vs one.

(also makes the lines shorter... but that is just a matter of taste).

I don't really have a preference.

>
> > > I am also not against your first version with a different commit message.
> >
> > Third time's a charm then!
> >
> > Will send a v2 with the first version having this commit message.
> >
> > Thanks for the review Ricardo,
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda