Fix upAp7 signed integer overflow #735
Conversation
isaacbrodsky
force-pushed
the
fix-upap7-signed-integer-overflow
branch
from
975a7ff
to
d5e49e5
Compare
src/h3lib/include/mathExtensions.h
Outdated
| @@ -27,6 +27,14 @@ | |||
| */ | |||
| #define MAX(a, b) (((a) > (b)) ? (a) : (b)) | |||
|
|
|||
| // TODO: Verify | |||
There was a problem hiding this comment.
What needs to be verified here?
There was a problem hiding this comment.
This comment seems to be on an outdated version of the file. That comment was removed when I added tests for these macros.
| // This function needs a guard check to be safe and that guard | ||
| // check assumes k = 0. | ||
| CoordIJK ijkCopy3 = args->ijk; | ||
| if (ijkCopy3.k == 0 && !_ijkNormalizeCouldOverflow(&ijkCopy3)) { |
There was a problem hiding this comment.
Would it make more sense to actually set ijkCopy3.k = 0, rather than discarding all random input where this was not the case?
|
Is there a strong reason to keep the unchecked versions of these functions? Is there a significant perf difference? |
I didn't evaluate the performance difference. I don't believe there is a strong reason to keep the unchecked versions of the functions, although in use in cell indexing it should not be possible to overflow them, so they would have a bunch of |
| return true; | ||
| } | ||
| if (SUB_INT32S_OVERFLOWS(0, min)) { | ||
| // 0 - INT32_MIN would overflow |
There was a problem hiding this comment.
This comment should be:
| // 0 - INT32_MIN would overflow | |
| // 0 - min would overflow |
Right?
There was a problem hiding this comment.
In this case, min would be INT32_MIN, which is I believe the only case that would hit this condition.
src/h3lib/include/mathExtensions.h
Outdated
| @@ -27,6 +27,14 @@ | |||
| */ | |||
| #define MAX(a, b) (((a) > (b)) ? (a) : (b)) | |||
|
|
|||
| /** Evaluates to true if a + b would overflow for int32 */ | |||
| #define SUM_INT32S_OVERFLOWS(a, b) \ | |||
There was a problem hiding this comment.
Nit: SUM and SUB look nearly identical so I didn't even notice they were different macros in the conditionals below at first. Maybe this one can be:
| #define SUM_INT32S_OVERFLOWS(a, b) \ | |
| #define ADD_INT32S_OVERFLOWS(a, b) \ |
There was a problem hiding this comment.
That's reasonable. Renamed to ADD_...
src/h3lib/lib/coordijk.c
Outdated
| 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; |
There was a problem hiding this comment.
Maybe it would make sense to make a MUL_INT32S_OVERFLOWS(i, 3) check and skip this intermediate addition and check?
There was a problem hiding this comment.
Is there a reference for a MUL_INT32S_OVERFLOWS we can use? I looked at the implementation you pointed to in a conversation last week but that involved floating point math, and I was skeptical there would be a significant difference with the intermediate addition+check approach. If another macro is being used for performance reasons, I'd prefer there be some form of benchmarking.
src/h3lib/lib/coordijk.c
Outdated
| 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; |
There was a problem hiding this comment.
A MUL_INT32S_OVERFLOWS check would work here, too.
Adds versions of _upAp7/_upAp7r that check for signed integer overflow on integer operations, for use in the LocalIJ functions. Replaces #733.