Repository navigation
Conversation
|
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
left a comment
There was a problem hiding this comment.
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, or0for Modulo, with no UB. - The remaining UB reports come only from the existing
System.Mathcode, which the PR does not touch.
Automated review of 1086fac by Claude. The pull request was built and tested locally.
|
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/ 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. |
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.