Skip to content

Commit 1086fac

Browse files
committed
check signed overflow in MSBuild arithmetic intrinsics
1 parent b2d8361 commit 1086fac

2 files changed

Lines changed: 75 additions & 6 deletions

File tree

‎lib/importproject.cpp‎

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
#include <algorithm>
2929
#include <cctype>
30+
#include <climits>
3031
#include <cmath>
3132
#include <cstdlib>
3233
#include <cstring>
@@ -848,6 +849,44 @@ static std::string safeFormat(const char *fmt, Args... args) {
848849
#pragma clang diagnostic pop
849850
#endif
850851

852+
// Checked signed arithmetic for the [MSBuild]::Add/Subtract/Multiply intrinsics.
853+
// Operands come straight from the (untrusted) project file, so the result can
854+
// exceed long long and the plain a+b / a-b / a*b would be signed-overflow UB.
855+
// These return false on overflow so the caller can report it instead.
856+
static bool addChecked(long long a, long long b, long long &res) {
857+
if ((b > 0 && a > LLONG_MAX - b) || (b < 0 && a < LLONG_MIN - b))
858+
return false;
859+
res = a + b;
860+
return true;
861+
}
862+
863+
static bool subChecked(long long a, long long b, long long &res) {
864+
if ((b < 0 && a > LLONG_MAX + b) || (b > 0 && a < LLONG_MIN + b))
865+
return false;
866+
res = a - b;
867+
return true;
868+
}
869+
870+
static bool mulChecked(long long a, long long b, long long &res) {
871+
if (a > 0) {
872+
if (b > 0) {
873+
if (a > LLONG_MAX / b)
874+
return false;
875+
} else if (b < LLONG_MIN / a) {
876+
return false;
877+
}
878+
} else {
879+
if (b > 0) {
880+
if (a < LLONG_MIN / b)
881+
return false;
882+
} else if (a != 0 && b < LLONG_MAX / a) {
883+
return false;
884+
}
885+
}
886+
res = a * b;
887+
return true;
888+
}
889+
851890
// Evaluate a $([ClassName]::Method(args)) static property function.
852891
// Returns an empty string for unknown or unimplementable functions rather
853892
// than throwing, so import can continue gracefully.
@@ -893,24 +932,46 @@ std::string ImportProject::applyMSBuildStaticFunction(const std::string &classNa
893932
if (args.size() == 2) {
894933
long long a = 0, b = 0;
895934
if (toInt(args[0], a) && toInt(args[1], b)) {
896-
if (caseInsensitiveStringCompare(member, "Add") == 0)
897-
return std::to_string(a + b);
898-
if (caseInsensitiveStringCompare(member, "Subtract") == 0)
899-
return std::to_string(a - b);
900-
if (caseInsensitiveStringCompare(member, "Multiply") == 0)
901-
return std::to_string(a * b);
935+
long long r = 0;
936+
if (caseInsensitiveStringCompare(member, "Add") == 0) {
937+
if (!addChecked(a, b, r)) {
938+
addDebug("MSBuild::Add: integer overflow");
939+
return std::string();
940+
}
941+
return std::to_string(r);
942+
}
943+
if (caseInsensitiveStringCompare(member, "Subtract") == 0) {
944+
if (!subChecked(a, b, r)) {
945+
addDebug("MSBuild::Subtract: integer overflow");
946+
return std::string();
947+
}
948+
return std::to_string(r);
949+
}
950+
if (caseInsensitiveStringCompare(member, "Multiply") == 0) {
951+
if (!mulChecked(a, b, r)) {
952+
addDebug("MSBuild::Multiply: integer overflow");
953+
return std::string();
954+
}
955+
return std::to_string(r);
956+
}
902957
if (caseInsensitiveStringCompare(member, "Divide") == 0) {
903958
if (b == 0) {
904959
addDebug("MSBuild::Divide: division by zero");
905960
return std::string();
906961
}
962+
if (a == LLONG_MIN && b == -1) {
963+
addDebug("MSBuild::Divide: integer overflow");
964+
return std::string();
965+
}
907966
return std::to_string(a / b);
908967
}
909968
if (caseInsensitiveStringCompare(member, "Modulo") == 0) {
910969
if (b == 0) {
911970
addDebug("MSBuild::Modulo: division by zero");
912971
return std::string();
913972
}
973+
if (a == LLONG_MIN && b == -1)
974+
return std::string("0");
914975
return std::to_string(a % b);
915976
}
916977
}

‎test/testimportproject.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,14 @@ class TestImportProject : public TestFixture {
12111211
ASSERT_EQUALS("3", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Divide(9, 3))"));
12121212
ASSERT_EQUALS("2", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Modulo(5, 3))"));
12131213

1214+
// Operands that would overflow long long must not compute a signed-overflow
1215+
// (undefined-behavior) result; the operation yields the empty string instead.
1216+
ASSERT_EQUALS("", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Add(9223372036854775807, 1))"));
1217+
ASSERT_EQUALS("", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Subtract(-9223372036854775808, 1))"));
1218+
ASSERT_EQUALS("", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Multiply(9999999999, 9999999999))"));
1219+
ASSERT_EQUALS("", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Divide(-9223372036854775808, -1))"));
1220+
ASSERT_EQUALS("0", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::Modulo(-9223372036854775808, -1))"));
1221+
12141222
// --- $([MSBuild]::...) path helpers ---
12151223
ASSERT_EQUALS("foo/", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::EnsureTrailingSlash('foo'))"));
12161224
ASSERT_EQUALS("foo/", cppcheck::testing::expandMSBuildExpression("$([MSBuild]::EnsureTrailingSlash('foo/'))"));

0 commit comments

Comments
 (0)