fixed #13673/#14076 - fixed signedCharArrayIndex detection#7740
fixed #13673/#14076 - fixed signedCharArrayIndex detection#7740firewave wants to merge 1 commit intodanmar:mainfrom
signedCharArrayIndex detection#7740Conversation
firewave
force-pushed
the
sign-index
branch
2 times, most recently
from
d63e081 to
1899482
Compare
firewave
changed the title
signedCharArrayIndex detectionsignedCharArrayIndex detection
|
| " signed char ch = 0x80;\n" | ||
| " buf[ch] = 0;\n" | ||
| "}"); | ||
| ASSERT_EQUALS("[test.cpp:5:5]: (warning) Signed 'char' type used as array index. [signedCharArrayIndex]\n", errout_str()); |
There was a problem hiding this comment.
it's not required to change in this PR but I think the warning message should say that the index is negative.
In this case it's a known value so imho a "error" would be fine.
| continue; | ||
| const Token *index = tok->next()->astOperand2(); | ||
| if (warning && tok->variable()->isArray() && astIsSignedChar(index) && index->getValueGE(0x80, *mSettings)) | ||
| if (warning && astIsSignedChar(index) && index->getValueLE(-1, *mSettings)) |
There was a problem hiding this comment.
I fear there will be false positives when there is not an array. I.e. this code is safe:
void foo() {
int buf[1000];
int *p = buf + 512;
signed char c = 0x80;
p[c] = 123;
}
There was a problem hiding this comment.
With the array check there are false negatives. It also matches the code for the unknown signedness below. So that FP might apply to that, too. Will check later and if that is the case will file a ticket and would treat it as out-of-scope for now.
There was a problem hiding this comment.
I fear there will be false positives when there is not an array. I.e. this code is safe:
void foo() { int buf[1000]; int *p = buf + 512; signed char c = 0x80; p[c] = 123; }
Good catch. This is indeed reported. The index will be negative and the memory access still valid but I would not call it "safe".
But we do not care if the access is actually valid because if you reduce the offset, it would not be reported.
If you remove signed you will get a -Wchar-subscripts Clang warning though.
There was a problem hiding this comment.
I would not call it "safe".
ok sure. it can very well be unintentional access. But it's not undefined behavior at least. And if it's not undefined behavior I don't feel that error/warning severity should be used. So I am not 100% against such warning but make it non-warning/error.. and maybe review the message doesn't it explicitly say "array index".
There was a problem hiding this comment.
Yes, this needs some more looking into as this is a rather confusing check. And there's also negativeIndex lurking around which might have overlap.
2 participants
No description provided.