Skip to content

Fix #15117 (MSBuild arithmetic intrinsics: signed overflow UB in Add/Subtract/Multiply) - #8938

Open
Nussu06 wants to merge 2 commits into
cppcheck-opensource:mainfrom
Nussu06:msbuild-arith-overflow
Open

Nussu06 wants to merge 2 commits into
cppcheck-opensource:mainfrom
Nussu06:msbuild-arith-overflow

Conversation

@Nussu06

@Nussu06 Nussu06 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

The $([MSBuild]::Add/Subtract/Multiply) intrinsics in applyMSBuildStaticFunction computed a+b, a-b and a*b directly on long long operands from the project file, so values like $([MSBuild]::Add(9223372036854775807, 1)) were signed-overflow UB (reported by -fsanitize=signed-integer-overflow).

MSBuild evaluates these in .NET's unchecked context, so on overflow the result wraps. The arithmetic is now done on unsigned long long and cast back, which gives the same two's complement result without UB. Divide/Modulo of LLONG_MIN by -1 throw OverflowException in .NET, so those return the empty string with a debug message, the same as the existing division-by-zero handling.

Regression tests added to testMSBuildStaticFunctions.

@dmcppcheck

Copy link
Copy Markdown

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.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AI review

I found no new problems in the changed code. I checked addChecked, subChecked and mulChecked against exact 128-bit results for every pair of boundary values and they gave no wrong answers. An end-to-end run of a UBSan build on a crafted .vcxproj gives no signed-overflow reports for [MSBuild]::Add/Subtract/Multiply/Divide/Modulo.

One related issue sits outside the diff, so it is not a line comment: the same file still has undefined behaviour from untrusted project values in the System.Math intrinsics. fmtDouble (lib/importproject.cpp:1445) and Round (lib/importproject.cpp:1466, and the + 1 at line 1468) cast a double to long long without a range check. For example, $([System.Math]::Pow(10,30)) and $([System.Math]::Round(1e30)) trigger UBSan float-cast-overflow reports. This code was there before the PR. The maintainers may want to fix it here, since the PR's stated goal is to remove UB in the MSBuild arithmetic intrinsics, or handle it in a follow-up.

Testing

Build: make -j8 cppcheck testrunner in pr/ succeeded.

Unit tests: ./testrunner TestImportProject passed: 48 tests, 0 failed, 1 todo.

UBSan build: I rebuilt only lib/importproject.cpp with -fsanitize=undefined,float-cast-overflow and linked it into testrunner_ubsan and cppcheck_ubsan. testrunner_ubsan TestImportProject passed with no sanitizer reports.

Helper check: I copied the three helpers into scratch/chk.cpp and compared them with __int128 arithmetic. The value set was {LLONG_MIN, LLONG_MIN+1, ±3037000499/500, -2, -1, 0, 1, 2, LLONG_MAX-1, LLONG_MAX}, run as all pairs. Result: bad=0, so no wrong overflow decisions.

End-to-end project (scratch/proj/p.vcxproj): It defines these properties:

<A>$([MSBuild]::Multiply(9999999999,9999999999))</A>
<B>$([MSBuild]::Divide(-9223372036854775808,-1))</B>
<C>$([MSBuild]::Add(9223372036854775807,1))</C>
<D>$([MSBuild]::Modulo(-9223372036854775808,-1))</D>
<E>$([MSBuild]::Multiply(-1,-9223372036854775808))</E>
<F>$([System.Math]::Pow(10,30))</F>
<G>$([System.Math]::Round(1e30))</G>

and uses them in PreprocessorDefinitions. I ran:

cppcheck_ubsan --project=p.vcxproj

lib/importproject.cpp:1445:51: runtime error: 1e+30 is outside the range of representable values of type 'long long int'
lib/importproject.cpp:1466:58: runtime error: 1e+30 is outside the range of representable values of type 'long long int'
Checking a.c: ...;A=;B=;C=;D=0;E=;F=1000000000000000019884624838656.000000;G=-9223372036854775808;...
  • The [MSBuild] cases (A–E) give the empty string, or 0 for Modulo, with no UB.
  • The remaining UB reports come only from the existing System.Math code, which the PR does not touch.

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

@dmcppcheck

Copy link
Copy Markdown

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

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8938/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.9% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8938/2026-10-13_10-24-21-1086fac0cd08_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.

@Nussu06

Nussu06 commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

I don't have access to the Trac instance, so could you create the ticket? Once there's a number I'll update the title to reference it (Fix #NNNN).

On the review's System.Math note: that float-cast-overflow in fmtDouble/Round is pre-existing and outside this diff, so I'd rather keep this PR scoped to the arithmetic intrinsics. Happy to send a follow-up for it once this lands if that's preferred.

@danmar

danmar commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

I have created this ticket: https://trac.cppcheck.net/ticket/15117

@IOBYTE

IOBYTE commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

MSBuild compatibility is what's important. See https://trac.cppcheck.net/ticket/15117

The AI review needs to know that MSBuild compatibility is the goal.

@Nussu06 Nussu06 changed the title check signed overflow in MSBuild arithmetic intrinsics Fix #15117 (MSBuild arithmetic intrinsics: signed overflow UB in Add/Subtract/Multiply) Oct 20, 2026
@Nussu06

Nussu06 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Makes sense, I've reworked it to match MSBuild instead of rejecting the value. Add/Subtract/Multiply now wrap the same way .NET does (done in unsigned arithmetic so there's no UB), e.g. Add(9223372036854775807, 1) gives -9223372036854775808 as in the ticket. LLONG_MIN / -1 and LLONG_MIN % -1 throw OverflowException in .NET, so those both return empty with a debug message like division by zero does; I changed Modulo from "0" to empty for that reason. Title now references #15117.

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.

4 participants