Skip to content

fixed #13673/#14076 - fixed signedCharArrayIndex detection #7740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fixed #13673/#14076 - fixed signedCharArrayIndex detection #7740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2023,7 +2023,7 @@ void CheckOther::checkCharVariable()
if (!tok->variable()->isArray() && !tok->variable()->isPointer())
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))
Copy link
Owner

@danmar danmar Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
 }

Copy link
Collaborator Author

@firewave firewave Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@firewave firewave Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@danmar danmar Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Collaborator Author

@firewave firewave Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

signedCharArrayIndexError(tok);
if (portability && astIsUnknownSignChar(index) && index->getValueGE(0x80, *mSettings))
unknownSignCharArrayIndexError(tok);
Expand Down
3 changes: 3 additions & 0 deletions lib/token.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1922,6 +1922,7 @@ const ValueFlow::Value * Token::getValueLE(const MathLib::bigint val, const Sett
{
if (!mImpl->mValues)
return nullptr;
// TODO: assert the given value is actually possible - i.e. 0x80 for a signed char is not possible
return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
return !v.isImpossible() && v.isIntValue() && v.intvalue <= val;
});
Expand All @@ -1931,6 +1932,7 @@ const ValueFlow::Value * Token::getValueGE(const MathLib::bigint val, const Sett
{
if (!mImpl->mValues)
return nullptr;
// TODO: assert the given value is actually possible - i.e. 0x80 for a signed char is not possible
return ValueFlow::findValue(*mImpl->mValues, settings, [&](const ValueFlow::Value& v) {
return !v.isImpossible() && v.isIntValue() && v.intvalue >= val;
});
Expand All @@ -1940,6 +1942,7 @@ const ValueFlow::Value * Token::getValueNE(MathLib::bigint val) const
{
if (!mImpl->mValues)
return nullptr;
// TODO: assert the given value is actually possible - i.e. 0x80 for a signed char is not possible
const auto it = std::find_if(mImpl->mValues->cbegin(), mImpl->mValues->cend(), [=](const ValueFlow::Value& value) {
return value.isIntValue() && !value.isImpossible() && value.intvalue != val;
});
Expand Down
70 changes: 66 additions & 4 deletions test/testcharvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ class TestCharVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout_str());

check("int buf[256];\n"
"void foo()\n"
"{\n"
" 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());
Copy link
Owner

@danmar danmar Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


check("int buf[256];\n"
"void foo()\n"
"{\n"
Expand All @@ -71,7 +79,7 @@ class TestCharVar : public TestFixture {
check("int buf[256];\n"
"void foo()\n"
"{\n"
" char ch = 0;\n"
" unsigned char ch = 0;\n"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());
Expand All @@ -87,10 +95,17 @@ class TestCharVar : public TestFixture {
check("int buf[256];\n"
"void foo()\n"
"{\n"
" char ch = 0x80;\n"
" char ch = 0;\n"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:5:5]: (portability) 'char' type used as array index. [unknownSignCharArrayIndex]\n", errout_str());
ASSERT_EQUALS("", errout_str());

check("int buf[256];\n"
"void foo(unsigned char ch)\n"
"{\n"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("int buf[256];\n"
"void foo(signed char ch)\n"
Expand All @@ -106,13 +121,41 @@ class TestCharVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf)\n"
"{\n"
" unsigned char ch = 0x80;"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf)\n"
"{\n"
" signed char ch = 0x80;"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:3:31]: (warning) Signed 'char' type used as array index. [signedCharArrayIndex]\n", errout_str());

check("void foo(char* buf)\n"
"{\n"
" char ch = 0x80;"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:3:24]: (portability) 'char' type used as array index. [unknownSignCharArrayIndex]\n", errout_str());

check("void foo(char* buf)\n"
"{\n"
" unsigned char ch = 0;"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf)\n"
"{\n"
" signed char ch = 0;"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf)\n"
"{\n"
" char ch = 0;"
Expand All @@ -126,6 +169,18 @@ class TestCharVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf, unsigned char ch)\n"
"{\n"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf, signed char ch)\n"
"{\n"
" buf[ch] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(char* buf, char ch)\n"
"{\n"
" buf[ch] = 0;\n"
Expand All @@ -135,7 +190,7 @@ class TestCharVar : public TestFixture {
check("int flags[256];\n"
"void foo(const char* str)\n"
"{\n"
" flags[*str] = 0;\n"
" flags[(signed char)*str] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

Expand All @@ -146,6 +201,13 @@ class TestCharVar : public TestFixture {
"}");
ASSERT_EQUALS("", errout_str());

check("int flags[256];\n"
"void foo(const char* str)\n"
"{\n"
" flags[*str] = 0;\n"
"}");
ASSERT_EQUALS("", errout_str());

check("void foo(const char str[])\n"
"{\n"
" map[str] = 0;\n"
Expand Down
Loading