Re: [PATCH 0/3] cast sizeof to int for comparison

Dan Carpenter
2018-07-03T13:00:24Z

On Sun, Jul 01, 2018 at 08:51:55PM +0200, Julia Lawall wrote:
>
>
> On Sun, 1 Jul 2018, Joe Perches wrote:
>
> > On Sun, 2018-07-01 at 19:32 +0200, Julia Lawall wrote:
> > > Comparing an int to a size, which is unsigned, causes the int to become
> > > unsigned, giving the wrong result.
> > >
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> >
> > Great, thanks.
> >
> > But what about the ones in net/smc like:
> >
> > > net/smc/smc_clc.c:
> > >
> > > len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> > > sizeof(struct smc_clc_msg_decline));
> > > if (len < sizeof(struct smc_clc_msg_decline))
> >
> > Are those detected by the semantic match and ignored?
>
> I wasn't sure how to justify that kernel_sendmsg returns a negative value.
> If it is the case, I can send the patch. I only found this in one file,
> but there were multiple occurrences.
>

In theory, Smatch is supposed to know return values but kernel_sendmsg()
is too complicated for Smatch. It's a tricky thing... That particular
check is correct and deliberate, but there is another check which is
wrong.

net/smc/smc_clc.c
369 len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
370 sizeof(struct smc_clc_msg_decline));
371 if (len < sizeof(struct smc_clc_msg_decline))
372 smc->sk.sk_err = EPROTO;
373 if (len < 0)
374 smc->sk.sk_err = -len;

If it's invalid we set an error code, if it's already an error we
preserve the error code.

375 return sock_error(&smc->sk);

[ snip ]

442 /* due to the few bytes needed for clc-handshake this cannot block */
443 len = kernel_sendmsg(smc->clcsock, &msg, vec, i, plen);
444 if (len < sizeof(pclc)) {
445 if (len >= 0) {
^^^^^^^^
This is always true.

446 reason_code = -ENETUNREACH;
447 smc->sk.sk_err = -reason_code;
448 } else {
449 smc->sk.sk_err = smc->clcsock->sk->sk_err;
450 reason_code = -smc->sk.sk_err;
451 }
452 }

The other two checks are not type promoted so they also work as
intended.

This is an interesting sort of bug I've written a Smatch script inspired
by your work here. One for the type promotion and one for the
impossible condition. I'll let you know how it goes.

regards,
dan carpenter