On Mon, May 03, 2021 at 09:54:39AM +0300, Dan Carpenter wrote:
> On Fri, Apr 30, 2021 at 05:21:24PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 30, 2021 at 04:33:29PM +0300, Dan Carpenter wrote:
> > > On Fri, Apr 30, 2021 at 03:53:44PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Apr 30, 2021 at 02:41:06PM +0300, Dan Carpenter wrote:
> > > > > On Fri, Apr 30, 2021 at 11:46:07AM +0300, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > > Why would I make it unsigned? As a static analysis developer,
> > > > > pointlessly unsigned variables are one of the leading causes for the
> > > > > bugs I see.
> > > > >
> > > > > There are times where a iterator counter needs to be unsigned long, or
> > > > > u64 but I have never seen a case where changing an iterator from
> > > > > "int i;" to "unsigned int i;" solves a real life kernel bug. It only
> > > > > introduces bugs.
> > > >
> > > > See my followup to that, I meant
> > > >
> > > > unsigned int count;
> > > >
> > > > do {
> > > > ...
> > > > } while (--count);
> > > >
> > > > It doesn't solve bug, but prevents the code be read incorrectly like what you
> > > > are fixing can be avoided with do {} while (); along with unsigned type.
> > >
> > > Why would you use an unsigned int for this???
> >
> > Why it should be signed? You clearly show the amount of iterations. Check for
> > null I guess even compact in the assembly in comparison to -1.
> >
> > I do not see any point why it should be signed. For what purpose?
> >
> > It's a *down* counter.
>
> Yeah. And people regularly test down counters for >= 0.
I don't know that. What I see the test is as simple as
while (--count) which is basically > 0.
> Signed ints
> are safer.
Any research article about it? What about wrong integral promotions which
I consider is a root cause of many bugs? People should learn that, or the
C (standard) should be fixed to make it easier to get.
> Unsigned ints are a *leading* cause of bugs in the kernel.
Again, where this statistics comes from? Maybe it's a simple answer to the
question that review in kernel is not good enough?
> I don't know if they're in the top five but they're definitely in the
> top ten.
Again, I don't see any proofs.
> Also if you need a larger type you should switch to a 64 bit type. The
> 2-4 million range is very narrow.
>
> I have never seen a single kernel bug where the for loop counter was
> "int i;" and making it "unsigned int i;" fixed a real life kernel bug.
Your very patch suggests one of the solution to switch to unsigned to fix a
kernel bug (though with lowest severity).
> Of course, there are times when unsigned int is appropriate, like for
> sizes or because it's in the spec.
>
> It's frustrating to me because GCC encourages people to make loop
> counters unsigned and it introduces bugs.
Any code which was written in unthought manner is a buggy code. So, how
unsigned vs. signed loop counter any different here to any other buggy code?
> I'm looking at the git log right now and I see that someone changed:
>
> void dt_to_asm(FILE *f, struct dt_info *dti, int version)
> {
> struct version_info *vi = NULL;
> - int i;
> + unsigned int i;
> struct data strbuf = empty_data;
> struct reserve_info *re;
> const char *symprefix = "dt";
>
> There are two loops in that function:
>
> for (i = 0; i < ARRAY_SIZE(version_table); i++) {
>
> This the one that generates the warning. GCC knows at compile time that
> ARRAY_SIZE() is 5. ARGH!!! GCC is so lazy and horrible. If I did this
> in Smatch people would never accept it. Even if ARRAY_SIZE() were
> higher than INT_MAX the loop would behave the same regardless of whether
> it was signed or not because of type promotion.
>
> The other loop is:
>
> for (i = 0; i < reservenum; i++) {
>
> In this case "reservenum" comes from the command line. In the original
> code if it were negative that would be a harmless no-op but now because
> i is unsigned it's a crashing bug. Why did GCC not generate a warning
> for this? The code was obviously bad before, that's true, but now in a
> very measurable way it has become worse.
See above. I think the root cause that people do not understand C and how to
program in C in bug-less manner.
> This example is not really important. I only brought it up because it
> is most recent example of people changing "int i;" to "unsigned int i;".
> But there have been other cases like this which have had a security
> impact.
--
With Best Regards,
Andy Shevchenko
