Skip to content

fix signed overflow when multiplying array dimensions - #9027

Open
Nussu06 wants to merge 1 commit into
cppcheck-opensource:mainfrom
Nussu06:array-dimensions-overflow
Open

Nussu06 wants to merge 1 commit into
cppcheck-opensource:mainfrom
Nussu06:array-dimensions-overflow

Conversation

@Nussu06

@Nussu06 Nussu06 commented Nov 14, 2026

Copy link
Copy Markdown
Contributor

CheckBufferOverrunImpl::getBufferSize, accumulateStructMembers (used by ValueType::getSizeOf) and setValues in clangimport multiply the array dimensions of a variable in MathLib::bigint without an overflow check, so a declaration like char a[0x100000000][0x100000000]; in the checked code makes cppcheck run into signed integer overflow (UBSan at checkbufferoverrun.cpp:635 and symboldatabase.cpp:8702). When the product wraps to a small value the results are wrong as well: for char a[0x4000000000000001][4]; memset(a, 0, 10); cppcheck reports bufferAccessOutOfBounds, and sizeof of a struct holding that array is folded to 4. Added Variable::dimensionsProduct, which reports the overflow, and all three sites now treat an overflowing product as an unknown size; sizes that fit are unchanged. TestBufferOverrun::arraysize_overflow and a new case in TestValueFlow::valueFlowSizeof fail without the change, the full testrunner passes, and TestBufferOverrun, TestValueFlow and TestClangImport also pass in a build with -fsanitize=signed-integer-overflow. I don't have Trac access, could one of you create a ticket for this so I can reference it in the title?

@dmcppcheck

Copy link
Copy Markdown
Contributor

There should probably be a trac ticket for this PR. Please create it, or ask us to create it. Then the PR title should be updated.

@Nussu06

Nussu06 commented Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

hey can anyone create trac ticket for this pr, thanks.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

The PR removes the signed overflow when the dimensions are multiplied together. It does not fix the wrong results it describes, because the element count is then multiplied by the element size without a check. When the count fits but the byte size does not, it still wraps, and the false positives remain. Example: int a[0x4000000000000001]; memset(a, 0, 10); still reports bufferAccessOutOfBounds. sizeof of struct U { int m[0x4000000000000001]; } is still folded to 4. The PR's own example uses char a[0x4000000000000001][4], which the new check catches only because the element size is 1. Use int a[0x4000000000000001][1] and the bug comes back. The byte-size multiplications are in getBufferSize ((dim - index) * typeSize), n *= dim in the accHelper of getSizeOf, and mul * getSizeOf(...) in clangimport. These are size_t operations, so they are not UB, but the sizes they give are wrong. The new overflow check also duplicates the existing mulOverflows() in checkbufferoverrun.cpp. Apart from this I found no problems; the new overflow check itself looks correct.

Testing

I built cppcheck and testrunner in pr/ and cppcheck in base/; both built without errors. ./testrunner TestBufferOverrun TestValueFlow TestClangImport TestSymbolDatabase: 825 tests, 0 failed.

Test file a.c:

void f1() { int a[0x4000000000000001]; memset(a, 0, 10); }
void f2() { char a[0x2000000000000001][2]; memset(a, 0, 10); }
void f3() { int a[0x4000000000000001][1]; memset(a, 0, 10); }
struct S { int m[0x4000000000000001]; };
int f4() { return sizeof(struct S); }
struct T { char m[0x4000000000000001][4]; };
int f5() { char b[2]; return b[sizeof(struct T)]; }
struct U { int m[0x4000000000000001]; };
int f6() { char b[2]; return b[sizeof(struct U)]; }

Command: cppcheck -q --enable=all a.c

case base PR
f5 (the case from the PR) arrayIndexOutOfBounds, index 4 fixed
f1 a.c:1:47: error: Buffer is accessed out of bounds: a [bufferAccessOutOfBounds] still reported
f3 a.c:3:50: error: Buffer is accessed out of bounds: a [bufferAccessOutOfBounds] still reported
f6 a.c:9:31: error: Array 'b[2]' accessed at index 4, which is out of bounds. [arrayIndexOutOfBounds] still reported

In f1, f3 and f6 the element count fits in a bigint, but the byte size wraps when it is multiplied by sizeof(int). I did not run a sanitizer build.


Automated review of e208afc by Claude. The pull request was built and tested locally.

return i1 * dim.num;
});
MathLib::bigint dim = 0;
if (!var->dimensionsProduct(dim))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

This catches overflow only in the element count. Below, (dim - index) * typeSize can still wrap, because it is size_t arithmetic. void f() { int a[0x4000000000000001]; memset(a, 0, 10); } still gives a false positive bufferAccessOutOfBounds, and so does int a[0x4000000000000001][1]. The byte size needs the same check. The existing mulOverflows() a few lines below in this file could be used for both, instead of adding a second implementation in Variable::dimensionsProduct.

Comment thread lib/symboldatabase.cpp
return i1 * dim.num;
});
MathLib::bigint dim = 0;
if (!var.dimensionsProduct(dim))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

accHelper later does n *= dim with the element size, in size_t, and that can still wrap. With struct U { int m[0x4000000000000001]; };, sizeof(struct U) is still folded to 4, so char b[2]; b[sizeof(struct U)] reports arrayIndexOutOfBounds at index 4. The test only covers char elements, where the element size is 1.

Comment thread lib/clangimport.cpp
typeSize = 0;
break;
}
if (var.valueType())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

The same applies here: mul * getSizeOf(...) and the += into typeSize are not checked, so the struct size can still wrap when the element size is greater than 1.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit e208afc (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9027/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-9027/2026-11-14_06-15-12-e208afcd4929_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants