Hello Rasmus, Konrad, *,
On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:
> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
>
> Why cc stable?
>
> > Signed-off-by: Konrad Gräfe <k.graefe@xxxxxxxxxxx>
> > ---
> > Added in v3
> >
> > lib/vsprintf.c | 18 +++++++++++++++---
> > 1 file changed, 15 insertions(+), 3 deletions(-)
>
> The diffstat here, or for some other patch in the same series,
> definitely ought to mention lib/test_printf.c.
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index be71a03c936a..8aee1caabd9e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> > {
> > char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> > char *p = mac_addr;
> > - int i;
> > + int i, pos;
> > char separator;
> > bool reversed = false;
> > + bool uppercase = false;
> >
> > if (check_pointer(&buf, end, addr, spec))
> > return buf;
> > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> > separator = '-';
> > break;
> >
> > + case 'U':
> > + uppercase = true;
> > + break;
> > +
> > case 'R':
> > reversed = true;
> > fallthrough;
>
> This seems broken, and I'm surprised the compiler doesn't warn about
> separator possibly being uninitialized further down. I'm also surprised
> your testing hasn't caught this. For reference, the full switch
> statement is currently
Compiler (gcc) does not warn because of Makefile:
1038 # Enabled with W=2, disabled by default as noisy
1039 ifdef CONFIG_CC_IS_GCC
1040 KBUILD_CFLAGS += -Wno-maybe-uninitialized
1041 endif
With this commented:
lib/vsprintf.c: In function ‘mac_address_string’:
lib/vsprintf.c:1310:30: warning: ‘separator’ may be used uninitialized [-Wmaybe-uninitialized]
1310 | *p++ = separator;
| ~~~~~^~~~~~~~~~~
lib/vsprintf.c:1273:14: note: ‘separator’ was declared here
1273 | char separator;
| ^~~~~~~~~
Regards,
Peter
>
> switch (fmt[1]) {
> case 'F':
> separator = '-';
> break;
>
> case 'R':
> reversed = true;
> fallthrough;
>
> default:
> separator = ':';
> break;
> }
>
> > @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >
> > for (i = 0; i < 6; i++) {
> > if (reversed)
> > - p = hex_byte_pack(p, addr[5 - i]);
> > + pos = 5 - i;
> > + else
> > + pos = i;
> > +
> > + if (uppercase)
> > + p = hex_byte_pack_upper(p, addr[pos]);
> > else
> > - p = hex_byte_pack(p, addr[i]);
> > + p = hex_byte_pack(p, addr[pos]);
>
> I think this becomes quite hard to follow. We have string_upper() in
> linux/string_helpers.h, so I'd rather just leave this loop alone and do
>
> if (uppercase)
> string_upper(mac_addr, mac_addr);
>
> after the nul-termination.
>
> Rasmus
>
