RE: [PATCH 1/3] treewide: Lift switch variables out of switches
On Wednesday, January 23, 2019 6:04 AM, Kees Cook wrote
>
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
>
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed
> [-Wswitch-unreachable]
>siginfo_t si;
> ^~
>
> Signed-off-by: Kees Cook
Reviewed by: Edwin Zimmerman
> ---
> arch/x86/xen/enlighten_pv.c | 7 ---
> drivers/char/pcmcia/cm4000_cs.c | 2 +-
> drivers/char/ppdev.c | 20 ---
> drivers/gpu/drm/drm_edid.c| 4 ++--
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> drivers/net/ethernet/intel/e1000/e1000_main.c | 3 ++-
> drivers/tty/n_tty.c | 3 +--
> drivers/usb/gadget/udc/net2280.c | 5 ++---
> fs/fcntl.c| 3 ++-
> mm/shmem.c| 5 +++--
> net/core/skbuff.c | 4 ++--
> net/ipv6/ip6_gre.c| 4 ++--
> net/ipv6/ip6_tunnel.c | 4 ++--
> net/openvswitch/flow_netlink.c| 7 +++
> security/tomoyo/common.c | 3 ++-
> security/tomoyo/condition.c | 7 ---
> security/tomoyo/util.c| 4 ++--
> 18 files changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c54a493e139a..a79d4b548a08 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
> static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
> {
> int ret;
> +#ifdef CONFIG_X86_64
> + unsigned which;
> + u64 base;
> +#endif
>
> ret = 0;
>
> switch (msr) {
> #ifdef CONFIG_X86_64
> - unsigned which;
> - u64 base;
> -
> case MSR_FS_BASE: which = SEGBASE_FS; goto set;
> case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
> case MSR_GS_BASE: which = SEGBASE_GS_KERNEL; goto set;
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index 7a4eb86aedac..7211dc0e6f4f 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
> {
> struct cm4000_dev *dev = from_timer(dev, t, timer);
> unsigned int iobase = dev->p_dev->resource[0]->start;
> + unsigned char flags0;
> unsigned short s;
> struct ptsreq ptsreq;
> int i, atrc;
> @@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
> }
>
> switch (dev->mstate) {
> - unsigned char flags0;
> case M_CARDOFF:
> DEBUGP(4, dev, "M_CARDOFF\n");
> flags0 = inb(REG_FLAGS0(iobase));
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 1ae77b41050a..d77c97e4f996 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
> struct pp_struct *pp = file->private_data;
> struct parport *port;
> void __user *argp = (void __user *)arg;
> + struct ieee1284_info *info;
> + unsigned char reg;
> + unsigned char mask;
> + int mode;
> + s32 time32[2];
> + s64 time64[2];
> + struct timespec64 ts;
> + int ret;
>
> /* First handle the cases that don't take arguments. */
> switch (cmd) {
> case PPCLAIM:
> {
> - struct ieee1284_info *info;
> - int ret;
> -
> if (pp->flags & PP_CLAIMED) {
> dev_dbg(>pdev->dev, "you've already got it!\n");
> return -EINVAL;
> @@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int
> cmd, unsigned long arg)
>
> port = pp->pdev->port;
> switch (cmd) {
> - struct ieee1284_info *info;
> - unsigned char reg;
> - unsigned char mask;
> - int mode;
> - s32 time32[2];
> - s64 time64[2];
> - struct timespec64 ts;
> - int ret;
> -
> case PPRSTATUS:
> reg = parport_read_status(port);
> if (copy_to_user(argp, , sizeof(reg)))
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index b506e3622b08..8f93956c1628 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3942,12 +3942,12 @@ static void
Re: [PATCH 1/3] treewide: Lift switch variables out of switches
> On Jan 23, 2019, at 5:09 AM, Jann Horn wrote: > > AFAICS this only applies to switch statements (because they jump to a > case and don't execute stuff at the start of the block), not blocks > after if/while/... . It bothers me that we are going out of our way to deprecate valid C constructs in favor of placing the declarations elsewhere. As current compiler warnings would catch any reference before initialization usage anyway, it seems like we are letting a compiler warning rather than the language standard dictate syntax. Certainly if we want to make it a best practice coding style issue we can, and then an appropriate note explaining why should be added to Documentation/process/coding-style.rst. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] treewide: Lift switch variables out of switches
On Wed, 23 Jan 2019 at 13:09, Jann Horn wrote:
>
> On Wed, Jan 23, 2019 at 1:04 PM Greg KH wrote:
> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > > Variables declared in a switch statement before any case statements
> > > cannot be initialized, so move all instances out of the switches.
> > > After this, future always-initialized stack variables will work
> > > and not throw warnings like this:
> > >
> > > fs/fcntl.c: In function ‘send_sigio_to_task’:
> > > fs/fcntl.c:738:13: warning: statement will never be executed
> > > [-Wswitch-unreachable]
> > >siginfo_t si;
> > > ^~
> >
> > That's a pain, so this means we can't have any new variables in { }
> > scope except for at the top of a function?
>
> AFAICS this only applies to switch statements (because they jump to a
> case and don't execute stuff at the start of the block), not blocks
> after if/while/... .
>
I guess that means it may apply to other cases where you do a 'goto'
into the middle of a for() loop, for instance (at the first
iteration), which is also a valid pattern.
Is there any way to tag these assignments so the diagnostic disregards them?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] treewide: Lift switch variables out of switches
On Wed, Jan 23, 2019 at 1:04 PM Greg KH wrote:
> On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > Variables declared in a switch statement before any case statements
> > cannot be initialized, so move all instances out of the switches.
> > After this, future always-initialized stack variables will work
> > and not throw warnings like this:
> >
> > fs/fcntl.c: In function ‘send_sigio_to_task’:
> > fs/fcntl.c:738:13: warning: statement will never be executed
> > [-Wswitch-unreachable]
> >siginfo_t si;
> > ^~
>
> That's a pain, so this means we can't have any new variables in { }
> scope except for at the top of a function?
AFAICS this only applies to switch statements (because they jump to a
case and don't execute stuff at the start of the block), not blocks
after if/while/... .
> That's going to be a hard thing to keep from happening over time, as
> this is valid C :(
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] treewide: Lift switch variables out of switches
On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
>
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed
> [-Wswitch-unreachable]
>siginfo_t si;
> ^~
That's a pain, so this means we can't have any new variables in { }
scope except for at the top of a function?
That's going to be a hard thing to keep from happening over time, as
this is valid C :(
greg k-h
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
