Skip to content

Commit

Permalink
Change per review: add an overall bounds check around the overflow ch…
Browse files Browse the repository at this point in the history 
…ecks
  • Loading branch information
@isaacbrodsky
isaacbrodsky committed Dec 22, 2022
1 parent 2f3952e commit 5e1e2cd
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 75 deletions.
6 changes: 6 additions & 0 deletions src/apps/testapps/testCoordIjk.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ SUITE(coordIjk) {
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) {
Expand All @@ -85,5 +88,8 @@ SUITE(coordIjk) {
// 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");
}
}
66 changes: 33 additions & 33 deletions src/apps/testapps/testMathExtensions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
t_assert(!SUB_INT32S_OVERFLOWS((a), (b)), #a " - " #b \
" Does Not " \
"Overflow")
#define ASSERT_SUM_OVERFLOW(a, b) \
t_assert(SUM_INT32S_OVERFLOWS((a), (b)), #a " + " #b " Overflows")
#define ASSERT_SUM_DOES_NOT_OVERFLOW(a, b) \
t_assert(!SUM_INT32S_OVERFLOWS((a), (b)), #a " + " #b \
#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")

Expand Down Expand Up @@ -76,34 +76,34 @@ SUITE(mathExtensions) {
ASSERT_SUB_OVERFLOW(0, INT32_MIN);
}

TEST(sumOverflows) {
ASSERT_SUM_DOES_NOT_OVERFLOW(0, 0);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN, 0);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN, 1);
ASSERT_SUM_OVERFLOW(INT32_MIN, -1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN + 1, 0);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN + 1, 1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN + 1, -1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN + 1, 2);
ASSERT_SUM_OVERFLOW(INT32_MIN + 1, -2);
ASSERT_SUM_DOES_NOT_OVERFLOW(100, 10);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX, 0);
ASSERT_SUM_OVERFLOW(INT32_MAX, 1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX, -1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX - 1, 1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX - 1, -1);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX - 1, -2);
ASSERT_SUM_OVERFLOW(INT32_MAX - 1, 2);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MIN, INT32_MAX);
ASSERT_SUM_DOES_NOT_OVERFLOW(INT32_MAX, INT32_MIN);
ASSERT_SUM_OVERFLOW(INT32_MAX, INT32_MAX);
ASSERT_SUM_OVERFLOW(INT32_MIN, INT32_MIN);
ASSERT_SUM_DOES_NOT_OVERFLOW(-1, 0);
ASSERT_SUM_DOES_NOT_OVERFLOW(-1, 10);
ASSERT_SUM_DOES_NOT_OVERFLOW(-1, -10);
ASSERT_SUM_DOES_NOT_OVERFLOW(-1, INT32_MAX);
ASSERT_SUM_DOES_NOT_OVERFLOW(-2, INT32_MAX);
ASSERT_SUM_OVERFLOW(-1, INT32_MIN);
ASSERT_SUM_DOES_NOT_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);
}
}
2 changes: 1 addition & 1 deletion src/h3lib/include/mathExtensions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#define MAX(a, b) (((a) > (b)) ? (a) : (b))

/** Evaluates to true if a + b would overflow for int32 */
#define SUM_INT32S_OVERFLOWS(a, b) \
#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 */
Expand Down
92 changes: 51 additions & 41 deletions src/h3lib/lib/coordijk.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
#include "latLng.h"
#include "mathExtensions.h"

#define INT32_MAX_3 (INT32_MAX / 3)

/**
* Sets an IJK coordinate to the specified component values.
*
Expand Down Expand Up @@ -227,7 +229,7 @@ bool _ijkNormalizeCouldOverflow(const CoordIJK *ijk) {
// than max. If min is positive, then max is also positive, and a
// positive signed integer minus another positive signed integer will
// not overflow.
if (SUM_INT32S_OVERFLOWS(max, min)) {
if (ADD_INT32S_OVERFLOWS(max, min)) {
// max + min would overflow
return true;
}
Expand Down Expand Up @@ -315,33 +317,37 @@ Direction _unitIjkToDigit(const CoordIJK *ijk) {
* Assumes ijk is IJK+ coordinates (no negative numbers).
*/
H3Error _upAp7Checked(CoordIJK *ijk) {
// Doesn't need to be checked because i, j, and k must all be positive
// Doesn't need to be checked because i, j, and k must all be non-negative
int i = ijk->i - ijk->k;
int j = ijk->j - ijk->k;

if (SUM_INT32S_OVERFLOWS(i, i)) {
return E_FAILED;
}
int i2 = i + i;
if (SUM_INT32S_OVERFLOWS(i2, i)) {
return E_FAILED;
}
int i3 = i2 + i;
if (SUM_INT32S_OVERFLOWS(j, j)) {
return E_FAILED;
}
int j2 = j + j;
// <0 is checked because the input must all be non-negative, but some
// negative inputs are used in unit tests to exercise the below.
if (i >= INT32_MAX_3 || j >= INT32_MAX_3 || i < 0 || j < 0) {
if (ADD_INT32S_OVERFLOWS(i, i)) {
return E_FAILED;
}
int i2 = i + i;
if (ADD_INT32S_OVERFLOWS(i2, i)) {
return E_FAILED;
}
int i3 = i2 + i;
if (ADD_INT32S_OVERFLOWS(j, j)) {
return E_FAILED;
}
int j2 = j + j;

if (SUB_INT32S_OVERFLOWS(i3, j)) {
return E_FAILED;
}
if (SUM_INT32S_OVERFLOWS(i, j2)) {
return E_FAILED;
if (SUB_INT32S_OVERFLOWS(i3, j)) {
return E_FAILED;
}
if (ADD_INT32S_OVERFLOWS(i, j2)) {
return E_FAILED;
}
}

// TODO: Do the int math parts here in long double?
ijk->i = (int)lroundl((i3 - j) / 7.0L);
ijk->j = (int)lroundl((i + j2) / 7.0L);
ijk->i = (int)lroundl(((i * 3) - j) / 7.0L);
ijk->j = (int)lroundl((i + (j * 2)) / 7.0L);
ijk->k = 0;

// Expected not to be reachable, because max + min or max - min would need
Expand All @@ -360,33 +366,37 @@ H3Error _upAp7Checked(CoordIJK *ijk) {
* Assumes ijk is IJK+ coordinates (no negative numbers).
*/
H3Error _upAp7rChecked(CoordIJK *ijk) {
// Doesn't need to be checked because i, j, and k must all be positive
// Doesn't need to be checked because i, j, and k must all be non-negative
int i = ijk->i - ijk->k;
int j = ijk->j - ijk->k;

if (SUM_INT32S_OVERFLOWS(i, i)) {
return E_FAILED;
}
int i2 = i + i;
if (SUM_INT32S_OVERFLOWS(j, j)) {
return E_FAILED;
}
int j2 = j + j;
if (SUM_INT32S_OVERFLOWS(j2, j)) {
return E_FAILED;
}
int j3 = j2 + j;
// <0 is checked because the input must all be non-negative, but some
// negative inputs are used in unit tests to exercise the below.
if (i >= INT32_MAX_3 || j >= INT32_MAX_3 || i < 0 || j < 0) {
if (ADD_INT32S_OVERFLOWS(i, i)) {
return E_FAILED;
}
int i2 = i + i;
if (ADD_INT32S_OVERFLOWS(j, j)) {
return E_FAILED;
}
int j2 = j + j;
if (ADD_INT32S_OVERFLOWS(j2, j)) {
return E_FAILED;
}
int j3 = j2 + j;

if (SUM_INT32S_OVERFLOWS(i2, j)) {
return E_FAILED;
}
if (SUB_INT32S_OVERFLOWS(j3, i)) {
return E_FAILED;
if (ADD_INT32S_OVERFLOWS(i2, j)) {
return E_FAILED;
}
if (SUB_INT32S_OVERFLOWS(j3, i)) {
return E_FAILED;
}
}

// TODO: Do the int math parts here in long double?
ijk->i = (int)lroundl((i2 + j) / 7.0L);
ijk->j = (int)lroundl((j3 - i) / 7.0L);
ijk->i = (int)lroundl(((i * 2) + j) / 7.0L);
ijk->j = (int)lroundl(((j * 3) - i) / 7.0L);
ijk->k = 0;

// Expected not to be reachable, because max + min or max - min would need
Expand Down

0 comments on commit 5e1e2cd

Please sign in to comment.