Prevent signed integer overflow in maxGridDiskSize #686
Conversation
isaacbrodsky
force-pushed
the
prevent-signed-integer-overflow-maxgriddisksize
branch
from
231a8bd to
e3bb418
Compare
src/h3lib/lib/algos.c
Outdated
| @@ -162,6 +162,17 @@ H3Error H3_EXPORT(maxGridDiskSize)(int k, int64_t *out) { | |||
| if (k < 0) { | |||
| return E_DOMAIN; | |||
| } | |||
| if (k >= 13780510) { | |||
There was a problem hiding this comment.
Nit: Should we assign this to a const?
There was a problem hiding this comment.
And/or a collection of constants for each resolution? I assume a much lower k at res 0 will run into similar problems?
There was a problem hiding this comment.
As this function does not accept the resolution we have no way of knowing if the k value will be larger than the grid. This would require an API change.
k that generates >122 cells at resolution 0 is also not the immediate issue. The issue is k values like INT32_MAX which would result in signed integer overflow, which is undefined behavior.
There was a problem hiding this comment.
We can do that without breaking backwards compatibility by creating a new function that does take the resolution, and recommending it over the old one? That would help prevent memory over-allocation for the smaller resolutions, and we could mark the current one as deprecated (but still with this fix).
I do agree with @nrabinowitz in any case that this would be better as a self-descriptive constant variable name.
There was a problem hiding this comment.
Added a comment for the constant. I'd defer a new function which limits the size to a future PR. (Such a function would also need to have a few other considerations: gridDisk would need to use the new function, the new functions' return value can never be higher than maxGridDiskSize's for a given k)
| for (int idx = 1; idx < arrSize; idx++) { | ||
| t_assert(compressed[idx] == 0, "expected only 1 cell"); | ||
| } |
There was a problem hiding this comment.
just curious why checking the second element isn't sufficient? i'd expect the first 0 to "terminate" the array, no matter how big it is (like a C string)
There was a problem hiding this comment.
I'm not sure that is a valid assumption for compactCells. We should document that clearly if it is. For comparison, I don't believe that assumption holds for gridDisk specifically due to the use of a hash function.
There was a problem hiding this comment.
good point! this has ramifications for bindings (at least for h3-go) where I "post-process" the internally allocated C buffer to fit the language's paradigms, e.g. I resize the returned slice of indexes to the first zero'd index for compaction...
| if (loopCount > // LCOV_EXCL_BR_LINE | ||
| numRemainingHexes) { |
isaacbrodsky
deleted the
prevent-signed-integer-overflow-maxgriddisksize
branch
Signed integer overflow is undefined behavior in C, so we cannot add, multiply, etc. arbitrary integer inputs. This adds a hard cap on the maxGridDiskSize function based on knowledge of the grid system itself.