Message-ID: <7975041.c8dSIL3S0B@silver>
Date:   Tue, 25 Apr 2023 14:19:09 +0200
From:   Christian Schoenebeck <linux_oss@...debyte.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Eric Van Hensbergen <ericvh@...nel.org>,
        Latchesar Ionkov <lucho@...kov.net>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        v9fs@...ts.linux.dev
Subject: Re: [PATCH] fs/9p: Fix a datatype used with V9FS_DIRECT_IO

On Tuesday, April 25, 2023 12:40:22 PM CEST Dominique Martinet wrote:
> Christian Schoenebeck wrote on Tue, Apr 25, 2023 at 11:18:37AM +0200:
> > > I'm surprised W=1 doesn't catch this... and now I'm checking higher
> > > (noisy) W=, or even clang doesn't seem to print anything about e.g.
> > > 'v9ses->flags & V9FS_DIRECT_IO is never true' or other warnings I'd have
> > > expected to come up -- out of curiosity how did you find this?
> > 
> > Both gcc and clang only trigger an implicit conversion warning if the value of
> > the expression can be evaluated at compile time (i.e. all operands are
> > constant), then compiler realizes that the compile-time evaluated constant
> > value is too big for the assignment destination and triggers the warning.
> 
> Right, `v9ses->flags = V9FS_DIRECT_IO` would have triggered it but not
> with `|=` -- but in this case I was also expecting the check
> `v9ses->flags & V9fs_DIRECT_IO` to flag something odd...

No, because before that binary expression is "reduced" by the compiler,
`v9ses->flags` is already auto-promoted from `char` to `int`. So it is
effectively:

  INT_RVALUE BITWISE_AND INT_LITERAL

And not:

  CHAR_LVALUE BITWISE_AND INT_LITERAL

And up to this parser state that's absolutely valid.

The compiler would only able to detect this issue if it would carry the
information (min. used bits at runtime) along over multiple reductions,
up to the parser state where it eventually reduces the assignment, which is
apparently not implemented ATM.

I am however more suprised that neither clang's sanitizer, nor static analyzer
detect this issue either.

> But nothing seems to care; testing with this snippet:
> ---
> int foo(char x) {
> 	if (x & 0x200)
> 		return 1;
> 	return 0;
> }
> int foo2(unsigned char x) {
> 	if (x < 0)
> 		return 1;
> 	return 0;
> }
> ---
> gcc warns that the x < 0 is always false (clang actually doesn't, even
> with scan-build, I must be missing a flag?), but I didn't find anything
> complaining about the &.

-Wtype-limits (or specifically -Wtautological-unsigned-zero-compare) does it.

> I'd expect something like coverity to perform a bit better here but it's
> a pain to use the "free for open source" version (... I just requested
> access to https://scan.coverity.com/projects/128 but I have no idea if
> they build next or not)
> 
> Oh, well; glad Christophe noticed anyway.
> 
> > > Would probably be interesting to run some form of the same in our
> > > automation.
> > 
> > If there is any ATM? I als tried this issue with clang's undefined behaviour
> > sanitizer and with the clang static analyzer. Both did not detect it.
> 
> There's at least the intel bot building with W=1 and warning if any new
> such warning pops up (and I'd like to say I check myself, but I probably
> forget about half the time; I looked at making W=1 default for our part
> of the tree but it didn't look trivial? I'll try to have another look);
> but I'm not aware of anyone testing with scan-build or something else
> that'd contact us on new defects.
> 
> 

Powered by blists - more mailing lists