linux-kernel - Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'

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]

Date:   Wed, 31 Aug 2022 09:38:10 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Dmitry Rokosov <ddrokosov@...rdevices.ru>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        kernel test robot <lkp@...el.com>, <llvm@...ts.linux.dev>,
        <kbuild-all@...ts.01.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>
Subject: Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24:
 warning: format specifies type 'unsigned char' but the argument has type
 'unsigned int'

On Wed, 31 Aug 2022 03:24:05 +0300
Dmitry Rokosov <ddrokosov@...rdevices.ru> wrote:

> Hello Jonathan and Andy,
> 
> Sorry for such a late response, a couple of days ago my daughter was born.
> So I couldn't reach my laptop :)

Congratulations and good luck! :)

> 
> Please find my thoughts below.
> 
> > > > >> drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int' [-Wformat]  
> > > >                                               "msa311-%hhx", partid);
> > > >                                                       ~~~~   ^~~~~~
> > > >                                                       %x
> > > >    1 warning generated.  
> >   
> > > >    992                msa311->chip_name = devm_kasprintf(dev, GFP_KERNEL,  
> > > >  > 993                                                   "msa311-%hhx", partid);  
> >   
> > > I'm thinking intent here was to limit range of what was printed. Maybe better to use
> > > local u8 variable or cast?
> > >
> > > I can fix it up if that's fine with you - or even better send me a patch that fixes
> > > it however you prefer!  
> > 
> > Looking back at what Linus said about those specifiers, I would rather
> > go with simple %x or %02x.
> > 
> > P.S. Surprisingly many C developers don't know the difference between
> > %hhx and %02x, which is easy to check by
> > 
> >   char a = -1;
> >   printf("%hhx <==> %02x\n", a, a);
> >   a = 217;
> >   printf("%hhx <==> %02x\n", a, a);  
> 
> Thank you for pointing to Linus answer. I have explored it at the link:
> 
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
> 
> Actually, Linus described one exception to this rule, which I have
> in my patch. I have an integer which I want to print as a char.
> I see that Linus mentions it's a bad idea. I agree with that. But
> currently %hhx => %02x replacement breaks the requested behavior, %02x
> will not shrink integer value to char. I want to say, maybe it's better
> just cast the value to u8 type and print as %x. What do you think? I can
> prepare such a patch.
> 
> P.S. Andy's example to show the difference between %hhx and %02x makes
> more clear why such a replacement is not acceptable here.
> 
> Output:
> ff <==> ffffffff
> d9 <==> ffffffd9
> 
In this case the storage is an unsigned int, not an unsigned char.
Hence the value will be small and positive.  So I'm fairly sure you
won't hit the above because it's

0x000000ff --> ff
0x000000d9 --> d9

The range is limited to 8 bits because that's all the underlying register
holds.

Jonathan