Merge pull request #5744 from lhchavez/last-multiply-int64-overflow-change-i-swear

Add tests for `git__multiply_int64_overflow`

src/integer.h

@@ -166,15 +166,47 @@ GIT_INLINE(bool) git__add_int64_overflow(int64_t *out, int64_t one, int64_t two)
 #if !defined(git__multiply_int64_overflow)
 GIT_INLINE(bool) git__multiply_int64_overflow(int64_t *out, int64_t one, int64_t two)
 {
-	if ((one == -1 && two == INT64_MIN) ||
-	    (two == -1 && one == INT64_MIN))
-		return true;
+	/*
+	 * Detects whether `INT64_MAX < (one * two) || INT64_MIN > (one * two)`,
+	 * without incurring in undefined behavior. That is done by performing the
+	 * comparison with a division instead of a multiplication, which translates
+	 * to `INT64_MAX / one < two || INT64_MIN / one > two`. Some caveats:
+	 *
+	 * - The comparison sign is inverted when both sides of the inequality are
+	 *   multiplied/divided by a negative number, so if `one < 0` the comparison
+	 *   needs to be flipped.
+	 * - `INT64_MAX / -1` itself overflows (or traps), so that case should be
+	 *   avoided.
+	 * - Since the overflow flag is defined as the discrepance between the result
+	 *   of performing the multiplication in a signed integer at twice the width
+	 *   of the operands, and the truncated+sign-extended version of that same
+	 *   result, there are four cases where the result is the opposite of what
+	 *   would be expected:
+	 *   * `INT64_MIN * -1` / `-1 * INT64_MIN`
+	 *   * `INT64_MIN * 1 / `1 * INT64_MIN`
+	 */
 	if (one && two) {
-		if (one > 0 == two > 0) {
+		if (one > 0 && two > 0) {
 			if (INT64_MAX / one < two)
 				return true;
+		} else if (one < 0 && two < 0) {
+			if ((one == -1 && two == INT64_MIN) ||
+				  (two == -1 && one == INT64_MIN)) {
+				*out = INT64_MIN;
+				return false;
+			}
+			if (INT64_MAX / one > two)
+				return true;
+		} else if (one > 0 && two < 0) {
+			if ((one == 1 && two == INT64_MIN) ||
+			    (INT64_MIN / one > two))
+				return true;
+		} else if (one == -1) {
+			if (INT64_MIN / two > one)
+				return true;
 		} else {
-			if (INT64_MIN / one < two)
+			if ((one == INT64_MIN && two == 1) ||
+			    (INT64_MIN / one < two))
 				return true;
 		}
 	}

tests/core/strtol.c

@@ -30,6 +30,7 @@ void test_core_strtol__int32(void)
 {
 	assert_l32_parses("123", 123, 10);
 	assert_l32_parses("  +123 ", 123, 10);
+	assert_l32_parses("  -123 ", -123, 10);
 	assert_l32_parses("  +2147483647 ", 2147483647, 10);
 	assert_l32_parses("  -2147483648 ", -2147483648LL, 10);
 	assert_l32_parses("A", 10, 16);
@@ -46,6 +47,7 @@ void test_core_strtol__int64(void)
 {
 	assert_l64_parses("123", 123, 10);
 	assert_l64_parses("  +123 ", 123, 10);
+	assert_l64_parses("  -123 ", -123, 10);
 	assert_l64_parses("  +2147483647 ", 2147483647, 10);
 	assert_l64_parses("  -2147483648 ", -2147483648LL, 10);
 	assert_l64_parses("  2147483657 ", 2147483657LL, 10);