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. |
|
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. |
|
I have created this ticket: https://trac.cppcheck.net/ticket/15117 |
|
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. |
|
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. |
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.