Fix upAp7 signed integer overflow

CHANGELOG.md

@@ -11,7 +11,7 @@ The public API of this library consists of the functions declared in file
 
 ### Fixed
 - Fixed possible signed integer overflow in `h3NeighborRotations` (#707)
-- Fixed possible signed integer overflow in `localIjToCell` (#706)
+- Fixed possible signed integer overflow in `localIjToCell` (#706, #735)
 
 ### Changed
 - `assert` on defensive code blocks that are not already covered. (#720)

CMakeLists.txt

@@ -233,6 +233,7 @@ set(OTHER_SOURCE_FILES
     src/apps/testapps/testCoordIjk.c
     src/apps/testapps/testH3Memory.c
     src/apps/testapps/testH3Iterators.c
+    src/apps/testapps/testMathExtensions.c
     src/apps/miscapps/cellToBoundaryHier.c
     src/apps/miscapps/cellToLatLngHier.c
     src/apps/miscapps/generateBaseCellNeighbors.c
@@ -258,6 +259,7 @@ set(OTHER_SOURCE_FILES
     src/apps/fuzzers/fuzzerPolygonToCellsNoHoles.c
     src/apps/fuzzers/fuzzerCellToChildPos.c
     src/apps/fuzzers/fuzzerInternalAlgos.c
+    src/apps/fuzzers/fuzzerInternalCoordIjk.c
     src/apps/benchmarks/benchmarkPolygonToCells.c
     src/apps/benchmarks/benchmarkPolygon.c
     src/apps/benchmarks/benchmarkCellsToLinkedMultiPolygon.c
@@ -512,6 +514,7 @@ if(BUILD_FUZZERS)
     add_h3_fuzzer(fuzzerCellToChildPos src/apps/fuzzers/fuzzerCellToChildPos.c)
     if(ENABLE_REQUIRES_ALL_SYMBOLS)
         add_h3_fuzzer(fuzzerInternalAlgos src/apps/fuzzers/fuzzerInternalAlgos.c)
+        add_h3_fuzzer(fuzzerInternalCoordIjk src/apps/fuzzers/fuzzerInternalCoordIjk.c)
     endif()
 endif()
 

CMakeTests.cmake

@@ -202,6 +202,7 @@ add_h3_test(testCoordIjk src/apps/testapps/testCoordIjk.c)
 add_h3_test(testBaseCells src/apps/testapps/testBaseCells.c)
 add_h3_test(testPentagonIndexes src/apps/testapps/testPentagonIndexes.c)
 add_h3_test(testH3Iterators src/apps/testapps/testH3Iterators.c)
+add_h3_test(testMathExtensions src/apps/testapps/testMathExtensions.c)
 
 add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 0)
 add_h3_test_with_arg(testH3NeighborRotations src/apps/testapps/testH3NeighborRotations.c 1)

src/apps/fuzzers/README.md

@@ -74,6 +74,10 @@ In addition to the public API, the following internal functions of H3 are covere
 | directionForNeighbor | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
 | h3SetToVertexGraph | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
 | _vertexGraphToLinkedGeo | [fuzzerInternalAlgos](./fuzzerInternalAlgos.c)
+| _upAp7Checked | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
+| _upAp7rChecked | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
+| _ijkNormalizeCouldOverflow | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
+| _ijkNormalize | [fuzzerInternalCoordIjk](./fuzzerInternalCoordIjk.c)
 
 # libFuzzer Usage
 

src/apps/fuzzers/fuzzerInternalCoordIjk.c

@@ -0,0 +1,53 @@
+/*
+ * Copyright 2022 Uber Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/** @file
+ * @brief Fuzzer program for internal functions in coordijk.c
+ */
+
+#include "aflHarness.h"
+#include "algos.h"
+#include "h3api.h"
+#include "utility.h"
+
+typedef struct {
+    CoordIJK ijk;
+} inputArgs;
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
+    if (size < sizeof(inputArgs)) {
+        return 0;
+    }
+    const inputArgs *args = (const inputArgs *)data;
+
+    // These functions require that input be non-negative
+    if (args->ijk.i >= 0 && args->ijk.j >= 0 && args->ijk.k >= 0) {
+        CoordIJK ijkCopy1 = args->ijk;
+        _upAp7Checked(&ijkCopy1);
+        CoordIJK ijkCopy2 = args->ijk;
+        _upAp7rChecked(&ijkCopy2);
+    }
+    // This function needs a guard check to be safe and that guard
+    // check assumes k = 0.
+    CoordIJK ijkCopy3 = args->ijk;
+    ijkCopy3.k = 0;
+    if (!_ijkNormalizeCouldOverflow(&ijkCopy3)) {
+        _ijkNormalize(&ijkCopy3);
+    }
+
+    return 0;
+}
+
+AFL_HARNESS_MAIN(sizeof(inputArgs));

src/apps/testapps/testCellToLocalIj.c

@@ -307,4 +307,36 @@ SUITE(h3ToLocalIj) {
         t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
                  "High magnitude J and I components fail");
     }
+
+    TEST(localIjToCell_overflow_particularCases) {
+        H3Index origin;
+        setH3Index(&origin, 2, 2, CENTER_DIGIT);
+        H3Index originRes3;
+        setH3Index(&originRes3, 2, 2, CENTER_DIGIT);
+
+        CoordIJ ij = {.i = 553648127, .j = -2145378272};
+        H3Index out;
+        t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
+                 "Particular high magnitude J and I components fail (1)");
+
+        ij.i = INT32_MAX - 10;
+        ij.j = -11;
+        t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
+                 "Particular high magnitude J and I components fail (2)");
+
+        ij.i = 553648127;
+        ij.j = -2145378272;
+        t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
+                 "Particular high magnitude J and I components fail (3)");
+
+        ij.i = INT32_MAX - 10;
+        ij.j = -10;
+        t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
+                 "Particular high magnitude J and I components fail (4)");
+
+        ij.i = INT32_MAX - 10;
+        ij.j = -9;
+        t_assert(H3_EXPORT(localIjToCell)(origin, &ij, 0, &out) == E_FAILED,
+                 "Particular high magnitude J and I components fail (5)");
+    }
 }

src/apps/testapps/testCoordIjk.c

@@ -50,4 +50,46 @@ SUITE(coordIjk) {
         _neighbor(&ijk, INVALID_DIGIT);
         t_assert(_ijkMatches(&ijk, &i), "Invalid neighbor is self");
     }
+
+    TEST(_upAp7Checked) {
+        CoordIJK ijk;
+
+        _setIJK(&ijk, 0, 0, 0);
+        t_assertSuccess(_upAp7Checked(&ijk));
+        _setIJK(&ijk, INT32_MAX, 0, 0);
+        t_assert(_upAp7Checked(&ijk) == E_FAILED, "i + i overflows");
+        _setIJK(&ijk, INT32_MAX / 2, 0, 0);
+        t_assert(_upAp7Checked(&ijk) == E_FAILED, "i * 3 overflows");
+        _setIJK(&ijk, 0, INT32_MAX, 0);
+        t_assert(_upAp7Checked(&ijk) == E_FAILED, "j + j overflows");
+        // This input should be invalid because j < 0
+        _setIJK(&ijk, INT32_MAX / 3, -2, 0);
+        t_assert(_upAp7Checked(&ijk) == E_FAILED, "(i * 3) - j overflows");
+        _setIJK(&ijk, INT32_MAX / 3, INT32_MAX / 2, 0);
+        t_assert(_upAp7Checked(&ijk) == E_FAILED, "i + (j * 2) overflows");
+        // This input should be invalid because j < 0
+        _setIJK(&ijk, -1, 0, 0);
+        t_assert(_upAp7Checked(&ijk) == E_SUCCESS, "i < 0 succeeds");
+    }
+
+    TEST(_upAp7rChecked) {
+        CoordIJK ijk;
+
+        _setIJK(&ijk, 0, 0, 0);
+        t_assertSuccess(_upAp7rChecked(&ijk));
+        _setIJK(&ijk, INT32_MAX, 0, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_FAILED, "i + i overflows");
+        _setIJK(&ijk, 0, INT32_MAX, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_FAILED, "j + j overflows");
+        _setIJK(&ijk, 0, INT32_MAX / 2, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_FAILED, "3 * j overflows");
+        _setIJK(&ijk, INT32_MAX / 2, INT32_MAX / 3, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_FAILED, "(i * 2) + j overflows");
+        // This input should be invalid because i < 0
+        _setIJK(&ijk, -2, INT32_MAX / 3, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_FAILED, "(j * 3) - 1 overflows");
+        // This input should be invalid because j < 0
+        _setIJK(&ijk, -1, 0, 0);
+        t_assert(_upAp7rChecked(&ijk) == E_SUCCESS, "i < 0 succeeds");
+    }
 }

src/apps/testapps/testMathExtensions.c

@@ -0,0 +1,109 @@
+/*
+ * Copyright 2022 Uber Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *         http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+/** @file
+ * @brief tests functions and macros in mathExtensions.h
+ *
+ *  usage: `testMathExtensions`
+ */
+
+#include "mathExtensions.h"
+#include "test.h"
+#include "utility.h"
+
+#define ASSERT_SUB_OVERFLOW(a, b) \
+    t_assert(SUB_INT32S_OVERFLOWS((a), (b)), #a " - " #b " Overflows")
+#define ASSERT_SUB_DOES_NOT_OVERFLOW(a, b)                    \
+    t_assert(!SUB_INT32S_OVERFLOWS((a), (b)), #a " - " #b     \
+                                                 " Does Not " \
+                                                 "Overflow")
+#define ASSERT_ADD_OVERFLOW(a, b) \
+    t_assert(ADD_INT32S_OVERFLOWS((a), (b)), #a " + " #b " Overflows")
+#define ASSERT_ADD_DOES_NOT_OVERFLOW(a, b)                    \
+    t_assert(!ADD_INT32S_OVERFLOWS((a), (b)), #a " + " #b     \
+                                                 " Does Not " \
+                                                 "Overflow")
+
+SUITE(mathExtensions) {
+    TEST(_ipow) {
+        t_assert(_ipow(7, 0) == 1, "7 ** 0 == 1");
+        t_assert(_ipow(7, 1) == 7, "7 ** 1 == 7");
+        t_assert(_ipow(7, 2) == 49, "7 ** 2 == 49");
+        t_assert(_ipow(1, 20) == 1, "1 ** 20 == 1");
+        t_assert(_ipow(2, 5) == 32, "2 ** 5 == 32");
+    }
+
+    TEST(subOverflows) {
+        ASSERT_SUB_DOES_NOT_OVERFLOW(0, 0);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, 0);
+        ASSERT_SUB_OVERFLOW(INT32_MIN, 1);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, -1);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, 0);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, 1);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, -1);
+        ASSERT_SUB_OVERFLOW(INT32_MIN + 1, 2);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN + 1, -2);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(100, 10);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, 0);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, 1);
+        ASSERT_SUB_OVERFLOW(INT32_MAX, -1);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX - 1, 1);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX - 1, -1);
+        ASSERT_SUB_OVERFLOW(INT32_MAX - 1, -2);
+        ASSERT_SUB_OVERFLOW(INT32_MAX - 1, -2);
+        ASSERT_SUB_OVERFLOW(INT32_MIN, INT32_MAX);
+        ASSERT_SUB_OVERFLOW(INT32_MAX, INT32_MIN);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MIN, INT32_MIN);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(INT32_MAX, INT32_MAX);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(-1, 0);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(-1, 10);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(-1, -10);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(-1, INT32_MAX);
+        ASSERT_SUB_OVERFLOW(-2, INT32_MAX);
+        ASSERT_SUB_DOES_NOT_OVERFLOW(-1, INT32_MIN);
+        ASSERT_SUB_OVERFLOW(0, INT32_MIN);
+    }
+
+    TEST(addOverflows) {
+        ASSERT_ADD_DOES_NOT_OVERFLOW(0, 0);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, 0);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, 1);
+        ASSERT_ADD_OVERFLOW(INT32_MIN, -1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 0);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, -1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN + 1, 2);
+        ASSERT_ADD_OVERFLOW(INT32_MIN + 1, -2);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(100, 10);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, 0);
+        ASSERT_ADD_OVERFLOW(INT32_MAX, 1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, -1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, 1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, -1);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX - 1, -2);
+        ASSERT_ADD_OVERFLOW(INT32_MAX - 1, 2);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MIN, INT32_MAX);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(INT32_MAX, INT32_MIN);
+        ASSERT_ADD_OVERFLOW(INT32_MAX, INT32_MAX);
+        ASSERT_ADD_OVERFLOW(INT32_MIN, INT32_MIN);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(-1, 0);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(-1, 10);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(-1, -10);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(-1, INT32_MAX);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(-2, INT32_MAX);
+        ASSERT_ADD_OVERFLOW(-1, INT32_MIN);
+        ASSERT_ADD_DOES_NOT_OVERFLOW(0, INT32_MIN);
+    }
+}

src/h3lib/include/coordijk.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2018, 2020-2021 Uber Technologies, Inc.
+ * Copyright 2016-2018, 2020-2022 Uber Technologies, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -93,8 +93,11 @@ int _ijkMatches(const CoordIJK *c1, const CoordIJK *c2);
 void _ijkAdd(const CoordIJK *h1, const CoordIJK *h2, CoordIJK *sum);
 void _ijkSub(const CoordIJK *h1, const CoordIJK *h2, CoordIJK *diff);
 void _ijkScale(CoordIJK *c, int factor);
+bool _ijkNormalizeCouldOverflow(const CoordIJK *ijk);
 void _ijkNormalize(CoordIJK *c);
 Direction _unitIjkToDigit(const CoordIJK *ijk);
+H3Error _upAp7Checked(CoordIJK *ijk);
+H3Error _upAp7rChecked(CoordIJK *ijk);
 void _upAp7(CoordIJK *ijk);
 void _upAp7r(CoordIJK *ijk);
 void _downAp7(CoordIJK *ijk);

src/h3lib/include/mathExtensions.h

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017-2018 Uber Technologies, Inc.
+ * Copyright 2017-2018, 2022 Uber Technologies, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,14 @@
  */
 #define MAX(a, b) (((a) > (b)) ? (a) : (b))
 
+/** Evaluates to true if a + b would overflow for int32 */
+#define ADD_INT32S_OVERFLOWS(a, b) \
+    ((a) > 0 ? (INT32_MAX - (a) < (b)) : (INT32_MIN - (a) > (b)))
+
+/** Evaluates to true if a - b would overflow for int32 */
+#define SUB_INT32S_OVERFLOWS(a, b) \
+    ((a) >= 0 ? (INT32_MIN + (a) >= (b)) : (INT32_MAX + (a) + 1 < (b)))
+
 // Internal functions
 int64_t _ipow(int64_t base, int64_t exp);