500860 - Static analysis to warn on a[i] if |i| is signed

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Comment 1

[Benjamin Smedberg]

I'm not sure this is a good idea. The google style guide, for example, explicitly specifies that loop variables and other similar things should use not use unsigned variables to indicate a value which must never be negative. Are you sure you want the exact opposite?

Comment 2

[Jesse Ruderman]

I'm pretty sure we've had bugs where unsigned variables would have helped, e.g. "if (i < a.length) { a[i] }".

I believe bsmedberg is referring to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types, which argues that using unsigned variables won't save you from many bugs, and can introduce new ones.  It's fairly convincing, I must say.

Comment 3

[georgi - hopefully not receiving bugspam]

probably the most popular example of signedness comes from kernel space:

int kernel(char *buf, int len) {
#define MA 16
char targ[MA];
if(len > MA) return -1;
copy_from_user(targ,buf,len);
}

Comment 4

[georgi - hopefully not receiving bugspam]

> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types

quite disagree.

Comment 5

[georgi - hopefully not receiving bugspam]

> Don't use an unsigned type. 

hope the authors of this write a lot of code and follow their advice.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with bsmedberg and Google. When you do decreasing iteration using unsigned values, you end up having to write unnatural code where 'i' refers to the index *after* the array value you care about. I.e.

for (PRUint32 i = N; i > 0; --i) {
  ... do something with a[i - 1] ...
}

No thanks.

Comment 7

[georgi - hopefully not receiving bugspam]

the g00gle example is *optimized away* by the compiler and it is clearly wrong by inspection - what does it prove besides the dumbness of the author.

if we are measuring dumbness, the dumb will notice at first hit it never terminates and he will fix it.

about your example:
s/PRUint32/PRInt32/

what do you do if N > 2^31 ?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

N is typically already PRInt32.

Comment 9

[georgi - hopefully not receiving bugspam]

> N is typically already PRInt32.

so this means values > 2^31 (2GB chars) are unreachable even on 64 bit systems:

#include "stdio.h"
int main() {
volatile int N;
volatile int i;
N= (1<<31) + 3; /* add more here */
printf("N=%d\n",N);
for(i = N; i > 0; --i) {
/*do something with a[i - 1] ...*/
printf("now i=%d\n",i);
};
printf("end of loop\n");
return(0);
}

./a.out 
N=-2147483645
end of loop

Comment 10

[Varsha J Hegde]

I am a student from SJCE, Mysore India and would like to take up this bug.

Comment 11

[Benjamin Smedberg]

Varsha, I don't think there's agreement that this is a good bug to fix. Why don't you work on the other ones you mentioned first?

Comment 12

[(no longer active)]

I also think this is a bad idea.  Gonna call it wontfix.