Skip to content

check signed overflow in MSBuild arithmetic intrinsics - #8938

Open
Nussu06 wants to merge 1 commit into
cppcheck-opensource:mainfrom
Nussu06:msbuild-arith-overflow
Open

Nussu06 wants to merge 1 commit 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 compute a+b, a-b and a*b on operands read straight from a Visual Studio project/props file, and Divide/Modulo don't guard the LLONG_MIN/-1 case, so a crafted property such as $([MSBuild]::Multiply(9999999999,9999999999)) or $([MSBuild]::Divide(-9223372036854775808,-1)) overflows long long, which is signed-overflow UB (reproduced under -fsanitize=signed-integer-overflow). Added checked arithmetic that detects the overflow and returns the empty string via addDebug, matching the existing division-by-zero handling in the same block. Regression tests for each operation were 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.

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