Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix undefined behavior in polygonToCells test #636

Merged
merged 4 commits into from
Aug 16, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ The public API of this library consists of the functions declared in file
[h3api.h.in](./src/h3lib/include/h3api.h.in).

## [Unreleased]
### Fixed
- `polygonToCells` returns an error if Infinity is passed in. (#636)

## [4.0.0-rc4] - 2022-07-25
### Breaking changes
Expand Down
2 changes: 1 addition & 1 deletion src/apps/testapps/testPolygon.c
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ SUITE(polygon) {
// This is a carefully crafted shape + point to hit an otherwise
// missed branch in coverage
LatLng verts[] = {{0, 0}, {1, 0.5}, {0, 1}};
GeoLoop geoloop = {.numVerts = 4, .verts = verts};
GeoLoop geoloop = {.numVerts = 3, .verts = verts};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏


BBox bbox;
bboxFromGeoLoop(&geoloop, &bbox);
Expand Down
44 changes: 44 additions & 0 deletions src/apps/testapps/testPolygonToCells.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
* limitations under the License.
*/

#include <assert.h>
#include <math.h>
#include <stdlib.h>

#include "algos.h"
Expand Down Expand Up @@ -43,6 +45,10 @@ static LatLng emptyVerts[] = {{0.659966917655, -2.1364398519394},
static GeoLoop emptyGeoLoop = {.numVerts = 3, .verts = emptyVerts};
static GeoPolygon emptyGeoPolygon;

static LatLng invalidVerts[] = {{INFINITY, INFINITY}, {-INFINITY, -INFINITY}};
static GeoLoop invalidGeoLoop = {.numVerts = 2, .verts = invalidVerts};
static GeoPolygon invalidGeoPolygon;

/**
* Return true if the cell crosses the meridian.
*/
Expand Down Expand Up @@ -128,6 +134,9 @@ SUITE(polygonToCells) {
emptyGeoPolygon.geoloop = emptyGeoLoop;
emptyGeoPolygon.numHoles = 0;

invalidGeoPolygon.geoloop = invalidGeoLoop;
invalidGeoPolygon.numHoles = 0;

TEST(maxPolygonToCellsSize) {
int64_t numHexagons;
t_assertSuccess(H3_EXPORT(maxPolygonToCellsSize)(&sfGeoPolygon, 9, 0,
Expand Down Expand Up @@ -420,4 +429,39 @@ SUITE(polygonToCells) {
iterateAllIndexesAtRes(1, fillIndex_assertions);
iterateAllIndexesAtRes(2, fillIndex_assertions);
}

TEST(getEdgeHexagonsInvalid) {
int64_t numHexagons = 100;
H3Index *search = calloc(numHexagons, sizeof(H3Index));
assert(search != NULL);
H3Index *found = calloc(numHexagons, sizeof(H3Index));
assert(found != NULL);

int res = 0;
int64_t numSearchHexes = 0;
H3Error err = _getEdgeHexagons(&invalidGeoLoop, numHexagons, res,
&numSearchHexes, search, found);
t_assert(err != E_SUCCESS,
"_getEdgeHexagons returns error for invalid geoloop");

free(found);
free(search);
}

TEST(polygonToCellsInvalid) {
int64_t numHexagons;
t_assert(H3_EXPORT(maxPolygonToCellsSize)(&invalidGeoPolygon, 9, 0,
&numHexagons) == E_FAILED,
"Cannot determine cell size to invalid geo polygon");

// Chosen arbitrarily, polygonToCells should error out before this is an
// issue.
numHexagons = 0;

H3Index *hexagons = calloc(numHexagons, sizeof(H3Index));
t_assert(H3_EXPORT(polygonToCells)(&invalidGeoPolygon, 9, 0,
hexagons) == E_FAILED,
"Flags other than 0 are invalid for polygonToCells");
free(hexagons);
}
}
6 changes: 3 additions & 3 deletions src/h3lib/include/bbox.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ bool bboxIsTransmeridian(const BBox *bbox);
void bboxCenter(const BBox *bbox, LatLng *center);
bool bboxContains(const BBox *bbox, const LatLng *point);
bool bboxEquals(const BBox *b1, const BBox *b2);
int64_t bboxHexEstimate(const BBox *bbox, int res);
int64_t lineHexEstimate(const LatLng *origin, const LatLng *destination,
int res);
H3Error bboxHexEstimate(const BBox *bbox, int res, int64_t *out);
H3Error lineHexEstimate(const LatLng *origin, const LatLng *destination,
int res, int64_t *out);

#endif
18 changes: 13 additions & 5 deletions src/h3lib/lib/algos.c
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,8 @@ H3Error h3NeighborRotations(H3Index origin, Direction dir, int *rotations,

int newRotations = 0;
int oldBaseCell = H3_GET_BASE_CELL(current);
if (oldBaseCell < 0 ||
oldBaseCell >= NUM_BASE_CELLS) { // LCOV_EXCL_BR_LINE
if (oldBaseCell < 0 || // LCOV_EXCL_BR_LINE
oldBaseCell >= NUM_BASE_CELLS) {
// Base cells less than zero can not be represented in an index
return E_CELL_INVALID;
}
Expand Down Expand Up @@ -757,7 +757,11 @@ H3Error H3_EXPORT(maxPolygonToCellsSize)(const GeoPolygon *geoPolygon, int res,
BBox bbox;
const GeoLoop geoloop = geoPolygon->geoloop;
bboxFromGeoLoop(&geoloop, &bbox);
int64_t numHexagons = bboxHexEstimate(&bbox, res);
int64_t numHexagons;
H3Error estimateErr = bboxHexEstimate(&bbox, res, &numHexagons);
if (estimateErr) {
return estimateErr;
}
// This algorithm assumes that the number of vertices is usually less than
// the number of hexagons, but when it's wrong, this will keep it from
// failing
Expand Down Expand Up @@ -800,8 +804,12 @@ H3Error _getEdgeHexagons(const GeoLoop *geoloop, int64_t numHexagons, int res,
LatLng origin = geoloop->verts[i];
LatLng destination = i == geoloop->numVerts - 1 ? geoloop->verts[0]
: geoloop->verts[i + 1];
const int64_t numHexesEstimate =
lineHexEstimate(&origin, &destination, res);
int64_t numHexesEstimate;
H3Error estimateErr =
lineHexEstimate(&origin, &destination, res, &numHexesEstimate);
if (estimateErr) {
return estimateErr;
}
for (int64_t j = 0; j < numHexesEstimate; j++) {
LatLng interpolate;
interpolate.lat =
Expand Down
37 changes: 25 additions & 12 deletions src/h3lib/lib/bbox.c
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ double _hexRadiusKm(H3Index h3Index) {
*
* @param bbox the bounding box to estimate the hexagon fill level
* @param res the resolution of the H3 hexagons to fill the bounding box
* @return the estimated number of hexagons to fill the bounding box
* @param out the estimated number of hexagons to fill the bounding box
* @return E_SUCCESS (0) on success, or another value otherwise.
*/
int64_t bboxHexEstimate(const BBox *bbox, int res) {
H3Error bboxHexEstimate(const BBox *bbox, int res, int64_t *out) {
// Get the area of the pentagon as the maximally-distorted area possible
H3Index pentagons[12] = {0};
// TODO: Return error here
isaacbrodsky marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -123,30 +124,42 @@ int64_t bboxHexEstimate(const BBox *bbox, int res) {
double a = d * d / fmin(3.0, fabs((p1.lng - p2.lng) / (p1.lat - p2.lat)));

// Divide the two to get an estimate of the number of hexagons needed
int64_t estimate = (int64_t)ceil(a / pentagonAreaKm2);
double estimateDouble = ceil(a / pentagonAreaKm2);
if (!isfinite(estimateDouble)) {
return E_FAILED;
}
int64_t estimate = (int64_t)estimateDouble;
if (estimate == 0) estimate = 1;
return estimate;
*out = estimate;
return E_SUCCESS;
}

/**
* lineHexEstimate returns an estimated number of hexagons that trace
* the cartesian-projected line
*
* @param origin the origin coordinates
* @param destination the destination coordinates
* @param res the resolution of the H3 hexagons to trace the line
* @return the estimated number of hexagons required to trace the line
* @param origin the origin coordinates
* @param destination the destination coordinates
* @param res the resolution of the H3 hexagons to trace the line
* @param out Out parameter for the estimated number of hexagons required to
* trace the line
* @return E_SUCCESS (0) on success or another value otherwise.
*/
int64_t lineHexEstimate(const LatLng *origin, const LatLng *destination,
int res) {
H3Error lineHexEstimate(const LatLng *origin, const LatLng *destination,
int res, int64_t *out) {
// Get the area of the pentagon as the maximally-distorted area possible
H3Index pentagons[12] = {0};
// TODO: Return error here
isaacbrodsky marked this conversation as resolved.
Show resolved Hide resolved
H3_EXPORT(getPentagons)(res, pentagons);
double pentagonRadiusKm = _hexRadiusKm(pentagons[0]);

double dist = H3_EXPORT(greatCircleDistanceKm)(origin, destination);
int64_t estimate = (int64_t)ceil(dist / (2 * pentagonRadiusKm));
double distCeil = ceil(dist / (2 * pentagonRadiusKm));
if (!isfinite(distCeil)) {
return E_FAILED;
}
int64_t estimate = (int64_t)distCeil;
if (estimate == 0) estimate = 1;
return estimate;
*out = estimate;
return E_SUCCESS;
}