Add range checks #423
Add range checks #423
Conversation
|
I see there are some issues with formatting. I don't think I'm ready to finish this PR, feel free to apply any edits or close and resubmit another PR... |
| @@ -840,6 +842,8 @@ bool _isBaseCellPolarPentagon(int baseCell) { | |||
| * Valid ijk+ lookup coordinates are from (0, 0, 0) to (2, 2, 2). | |||
| */ | |||
| int _faceIjkToBaseCell(const FaceIJK* h) { | |||
| if (!h || h->face < 0 || h->face >= NUM_ICOSA_FACES) | |||
There was a problem hiding this comment.
@isaacbrodsky - Do you think we need guards on face here? Or is this value only calculated within the library? If we can't trust face, we should probably add guards for coord.i, coord.j, and coord.k as well.
There was a problem hiding this comment.
I am a little curious what case this is guarding against. While I agree with preventing undefined behavior, I am not sure offhand whether this is a needed check.
|
Thanks for submitting this! You should be able to fix the test failures with @isaacbrodsky - thoughts on bringing this into 3.x? This is potentially a security issue. |
|
I haven't been following the issues daily so sorry for being late to the conversation again... The lack of range checks on arrays throughout H3 core (including the recent |
There was a problem hiding this comment.
To @sahrk' point, another suggestion posted in the issues is to expand the base cells array and ensure we never calculate an index into that array outside of 0-127 inclusive.
I would like to ensure that H3 is resilient to dereferencing out of bounds. Beyond the security concerns, this type of crash is often very difficult to debug, especially when it happens in a library loaded into a manageed framework. I would be willing to create a new 3.x release to incorporate this kind of change.
If possible, could you post test cases demonstrating some of these? Even if not coded in olur test framework it would be very helpful for coding the tests.
| @@ -290,6 +290,9 @@ H3Index h3NeighborRotations(H3Index origin, Direction dir, int* rotations) { | |||
|
|
|||
| int newRotations = 0; | |||
| int oldBaseCell = H3_GET_BASE_CELL(out); | |||
| if (oldBaseCell < 0 || oldBaseCell >= NUM_BASE_CELLS) | |||
| return 0; | |||
| @@ -840,6 +842,8 @@ bool _isBaseCellPolarPentagon(int baseCell) { | |||
| * Valid ijk+ lookup coordinates are from (0, 0, 0) to (2, 2, 2). | |||
| */ | |||
| int _faceIjkToBaseCell(const FaceIJK* h) { | |||
| if (!h || h->face < 0 || h->face >= NUM_ICOSA_FACES) | |||
There was a problem hiding this comment.
I am a little curious what case this is guarding against. While I agree with preventing undefined behavior, I am not sure offhand whether this is a needed check.
I should point-out that the baseCells and the faces in faceIjkToBaseCell are not the only arrays that are accessed without bounds checking in the core. There are a whole bunch of arrays in the core, many with multiple dimensions. If I ever did bounds checking on any of them it was by mistake :). The core was designed to be as tight as possible, which is why so much of it is table lookups, which are as cheap as it gets in C. In the case of the baseCells we're talking about a function that's just a wrapper for an array access. However you slice it, if you add bounds checking you're at least doubling the cost of the function.
That is all reasonable of course. Earlier I suggested making safe versions of table lookup functions that are exposed to external users (like baseCells), but what if you made the API functions safe and created internal functions without bounds checking for use just in the core? |
Implement #392
Not sure if everything is correct, I did not dig into the algorithms.
But at least it fixes ClickHouse/ClickHouse#19219
There should not be significant performance degradation for two reasons: