Re: drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar]

[Vincent MAILHOL]

On Wed. 2 Mar 2022 at 19:32, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 02.03.2022 17:47:08, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   575115360652e9920cc56a028a286ebe9bf82694
> > commit: c664e2137a27680922d8aeb64fb10313416b254f can: etas_es58x: add support for the ETAS ES58X_FD CAN USB interfaces
> > date:   11 months ago
> > compiler: powerpc64-linux-gcc (GCC) 11.2.0
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <yujie.liu@xxxxxxxxx>

Do we still need the tag for false positives (c.f. below)?
I am fine to add it, but maybe this will mess with your statistics?

> >
> > cppcheck possible warnings: (new ones prefixed by >>, may not be real problems)

Indeed, not a real problem. My wild guess is that cppcheck does
not recognize __stringify() as a pre-procesor macro. That would
not be the first time a static analyzer would fail on
it. checkpatch had the same issue which I fixed in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b844345fc2a9c46f8bb8cdb7408c766dfcdd83d

Or maybe cppcheck does not try to expand the macro and directly
yield a warning?

Overall, I think that GCC/Clang already does a good job at
finding usage of uninitialized variable. If cppcheck is less
mature in this aspect, I suggest to deactivate this particular
cppcheck rule and leave this job to GCC/Clang's -Wuninitialized.

> >    In file included from drivers/net/can/usb/etas_es58x/es58x_fd.c:
> > >> drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar]
> >     ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> >           ^
> >
> > vim +174 drivers/net/can/usb/etas_es58x/es58x_fd.c
> >
> > c664e2137a2768 Vincent Mailhol 2021-04-10  165
> > c664e2137a2768 Vincent Mailhol 2021-04-10  166  static int es58x_fd_rx_event_msg(struct net_device *netdev,
> > c664e2137a2768 Vincent Mailhol 2021-04-10  167                                 const struct es58x_fd_urb_cmd *es58x_fd_urb_cmd)
> > c664e2137a2768 Vincent Mailhol 2021-04-10  168  {
> > c664e2137a2768 Vincent Mailhol 2021-04-10  169        struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev;
> > c664e2137a2768 Vincent Mailhol 2021-04-10  170        u16 msg_len = get_unaligned_le16(&es58x_fd_urb_cmd->msg_len);
> > c664e2137a2768 Vincent Mailhol 2021-04-10 @171        const struct es58x_fd_rx_event_msg *rx_event_msg;
> > c664e2137a2768 Vincent Mailhol 2021-04-10  172        int ret;
> > c664e2137a2768 Vincent Mailhol 2021-04-10  173
> > c664e2137a2768 Vincent Mailhol 2021-04-10 @174        ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len);
> > c664e2137a2768 Vincent Mailhol 2021-04-10  175        if (ret)
> > c664e2137a2768 Vincent Mailhol 2021-04-10  176                return ret;
> > c664e2137a2768 Vincent Mailhol 2021-04-10  177
> > c664e2137a2768 Vincent Mailhol 2021-04-10  178        rx_event_msg = &es58x_fd_urb_cmd->rx_event_msg;
> > c664e2137a2768 Vincent Mailhol 2021-04-10  179
> > c664e2137a2768 Vincent Mailhol 2021-04-10  180        return es58x_rx_err_msg(netdev, rx_event_msg->error_code,
> > c664e2137a2768 Vincent Mailhol 2021-04-10  181                                rx_event_msg->event_code,
> > c664e2137a2768 Vincent Mailhol 2021-04-10  182                                get_unaligned_le64(&rx_event_msg->timestamp));
> > c664e2137a2768 Vincent Mailhol 2021-04-10  183  }
> > c664e2137a2768 Vincent Mailhol 2021-04-10  184
>
> Thanks for the report.
>
> This looks like a false positive to me, as es58x_check_msg_len() is not
> a function, but a macro:
>
> | #define es58x_check_msg_len(dev, msg, actual_len)                     \
> |       __es58x_check_msg_len(dev, __stringify(msg),                    \
> |                             actual_len, sizeof(msg))
>
> __es58x_check_msg_len() don't use "rx_event_msg" directly, but only a
> string representation of it and a "sizeof()".

Ack.

> I think it's possible to assign rx_event_msg before the
> es58x_check_msg_len().

Yes, I will do so. Even if this is a false positive, this pattern
can be misleading. e.g. during a code review, this does indeed
look incorrect at first glance.

Also, doing such change would be consistent with was is done in
other functions:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L210

This not being a bug fix, is it fine to send it to net-next?
Or do you see a need to backport this?

> I think (hope?) the compiler will not optimize
> anything away. :)

With a function call and a return statement, the compiler would
need to be severely defective to try to optimize this away :)


> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |