Re: [PATCH 4/4] net/tls: implement ->read_sock()

Dan Carpenter
2023-06-19T08:17:01Z

On Sat, Jun 17, 2023 at 04:08:08PM +0200, Simon Horman wrote:
> + Dan Carpenter
>
> On Wed, Jun 14, 2023 at 08:22:12AM +0200, Hannes Reinecke wrote:
>
> ...
>
> > +
> > +read_sock_end:
> > + return copied ? : err;
>
> Hi Hannes,
>
> I'm of two minds about raising this or not, but in any case here I am
> doing so.
>
> Both gcc-12 [-Wmaybe-uninitialized] and Smatch warn that err may be
> used uninitialised on the line above.
>
> My own analysis is that it cannot occur: I think it is always the case
> that either copied is non-zero or err is initialised.

Hm... Yeah. This is a bug in how Smatch handles:

return copied ? : err;

Smatch wants every return to have a simple literal or variable. So if
the return is complicated it gets changed into:

fake_variable = copied ? : err;
return fake_variable;

Smatch translates this to:

if (!(fake_variable = copied))
fake_variable = err;

[ Here fake_variable doesn't have side effects but under other
circumstances this transformation could cause double evaluate side
effects bugs. So that's another bug in Smatch. ]

Then Smatch parses the fake_variable = copied condition as:

fake_variable = copied;
if (fake_variable) {

The problem is that the type of fake_variable is based on the type of
tls_sw_read_sock() so it is a 32bit while copied is a 64bit (of unknown
value). So Smatch says, "Just because copied is non-zero doesn't mean
fake_variable is non-zero because the value might get truncated when
we cast away the high 32 bits."

This not a serious proposal but a just to demonstrate that this is
what happens there are two ways to silence this warning. Changing the
type of tls_sw_read_sock() to long. Or change the code to:

if (copied)
return copied;
return err;

Probably the right thing is to create a second fake_copied variable
based on typeof(copied).

fake_copied = copied;
if (fake_copied)
fake_return_variable = fake_copied;
else
fake_return_variable = err;

It's a doable thing. Plus now there are no double evaluate side effects
bugs. I have written this code and it silences the warning, but I'll
test it out tonight to see what breaks.

regards,
dan carpenter