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

[Dan Carpenter]

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