Skip to content

Commit

Permalink
Revert "Protect H3 library against integer overflow #92829" (#113773) (…
Browse files Browse the repository at this point in the history 
…#113782)

This library has well defined inputs and outputs so protecting against overflow is not necessary and it introduces a 
significant overhead.
  • Loading branch information
@iverase
iverase committed Sep 30, 2024
1 parent c5a4792 commit bd5f989
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 69 deletions.
73 changes: 36 additions & 37 deletions libs/h3/src/main/java/org/elasticsearch/h3/CoordIJK.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,17 +109,17 @@ void reset(int i, int j, int k) {
* Find the center point in 2D cartesian coordinates of a hex.
*/
public Vec2d ijkToHex2d() {
final int i = Math.subtractExact(this.i, this.k);
final int j = Math.subtractExact(this.j, this.k);
final int i = this.i - this.k;
final int j = this.j - this.k;
return new Vec2d(i - 0.5 * j, j * Constants.M_SQRT3_2);
}

/**
* Find the center point in spherical coordinates of a hex on a particular icosahedral face.
*/
public LatLng ijkToGeo(int face, int res, boolean substrate) {
final int i = Math.subtractExact(this.i, this.k);
final int j = Math.subtractExact(this.j, this.k);
final int i = this.i - this.k;
final int j = this.j - this.k;
return Vec2d.hex2dToGeo(i - 0.5 * j, j * Constants.M_SQRT3_2, face, res, substrate);
}

Expand All @@ -132,9 +132,9 @@ public LatLng ijkToGeo(int face, int res, boolean substrate) {
*/

public void ijkAdd(int i, int j, int k) {
this.i = Math.addExact(this.i, i);
this.j = Math.addExact(this.j, j);
this.k = Math.addExact(this.k, k);
this.i += i;
this.j += j;
this.k += k;
}

/**
Expand All @@ -145,9 +145,9 @@ public void ijkAdd(int i, int j, int k) {
* @param k the k coordinate
*/
public void ijkSub(int i, int j, int k) {
this.i = Math.subtractExact(this.i, i);
this.j = Math.subtractExact(this.j, j);
this.k = Math.subtractExact(this.k, k);
this.i -= i;
this.j -= j;
this.k -= k;
}

/**
Expand All @@ -168,9 +168,9 @@ public void downAp7() {
// iVec (3, 0, 1)
// jVec (1, 3, 0)
// kVec (0, 1, 3)
final int i = Math.addExact(Math.multiplyExact(this.i, 3), this.j);
final int j = Math.addExact(Math.multiplyExact(this.j, 3), this.k);
final int k = Math.addExact(Math.multiplyExact(this.k, 3), this.i);
final int i = this.i * 3 + this.j;
final int j = this.j * 3 + this.k;
final int k = this.k * 3 + this.i;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -185,9 +185,9 @@ public void downAp7r() {
// iVec (3, 1, 0)
// jVec (0, 3, 1)
// kVec (1, 0, 3)
final int i = Math.addExact(Math.multiplyExact(this.i, 3), this.k);
final int j = Math.addExact(Math.multiplyExact(this.j, 3), this.i);
final int k = Math.addExact(Math.multiplyExact(this.k, 3), this.j);
final int i = this.i * 3 + this.k;
final int j = this.j * 3 + this.i;
final int k = this.k * 3 + this.j;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -203,9 +203,9 @@ public void downAp3() {
// iVec (2, 0, 1)
// jVec (1, 2, 0)
// kVec (0, 1, 2)
final int i = Math.addExact(Math.multiplyExact(this.i, 2), this.j);
final int j = Math.addExact(Math.multiplyExact(this.j, 2), this.k);
final int k = Math.addExact(Math.multiplyExact(this.k, 2), this.i);
final int i = this.i * 2 + this.j;
final int j = this.j * 2 + this.k;
final int k = this.k * 2 + this.i;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -221,9 +221,9 @@ public void downAp3r() {
// iVec (2, 1, 0)
// jVec (0, 2, 1)
// kVec (1, 0, 2)
final int i = Math.addExact(Math.multiplyExact(this.i, 2), this.k);
final int j = Math.addExact(Math.multiplyExact(this.j, 2), this.i);
final int k = Math.addExact(Math.multiplyExact(this.k, 2), this.j);
final int i = this.i * 2 + this.k;
final int j = this.j * 2 + this.i;
final int k = this.k * 2 + this.j;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -239,9 +239,9 @@ public void ijkRotate60cw() {
// iVec (1, 0, 1)
// jVec (1, 1, 0)
// kVec (0, 1, 1)
final int i = Math.addExact(this.i, this.j);
final int j = Math.addExact(this.j, this.k);
final int k = Math.addExact(this.i, this.k);
final int i = this.i + this.j;
final int j = this.j + this.k;
final int k = this.i + this.k;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -256,9 +256,9 @@ public void ijkRotate60ccw() {
// iVec (1, 1, 0)
// jVec (0, 1, 1)
// kVec (1, 0, 1)
final int i = Math.addExact(this.i, this.k);
final int j = Math.addExact(this.i, this.j);
final int k = Math.addExact(this.j, this.k);
final int i = this.i + this.k;
final int j = this.i + this.j;
final int k = this.j + this.k;
this.i = i;
this.j = j;
this.k = k;
Expand All @@ -282,10 +282,10 @@ public void neighbor(int digit) {
* clockwise aperture 7 grid.
*/
public void upAp7r() {
final int i = Math.subtractExact(this.i, this.k);
final int j = Math.subtractExact(this.j, this.k);
this.i = (int) Math.round((Math.addExact(Math.multiplyExact(2, i), j)) * M_ONESEVENTH);
this.j = (int) Math.round((Math.subtractExact(Math.multiplyExact(3, j), i)) * M_ONESEVENTH);
final int i = this.i - this.k;
final int j = this.j - this.k;
this.i = (int) Math.round((2 * i + j) * M_ONESEVENTH);
this.j = (int) Math.round((3 * j - i) * M_ONESEVENTH);
this.k = 0;
ijkNormalize();
}
Expand All @@ -296,10 +296,10 @@ public void upAp7r() {
*
*/
public void upAp7() {
final int i = Math.subtractExact(this.i, this.k);
final int j = Math.subtractExact(this.j, this.k);
this.i = (int) Math.round((Math.subtractExact(Math.multiplyExact(3, i), j)) * M_ONESEVENTH);
this.j = (int) Math.round((Math.addExact(Math.multiplyExact(2, j), i)) * M_ONESEVENTH);
final int i = this.i - this.k;
final int j = this.j - this.k;
this.i = (int) Math.round((3 * i - j) * M_ONESEVENTH);
this.j = (int) Math.round((2 * j + i) * M_ONESEVENTH);
this.k = 0;
ijkNormalize();
}
Expand Down Expand Up @@ -363,5 +363,4 @@ public static int rotate60ccw(int digit) {
default -> digit;
};
}

}
20 changes: 4 additions & 16 deletions libs/h3/src/main/java/org/elasticsearch/h3/FaceIJK.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,11 +474,7 @@ public CellBoundary faceIjkPentToCellBoundary(int res, int start, int length) {
}

final int unitScale = unitScaleByCIIres[adjRes] * 3;
lastCoord.ijkAdd(
Math.multiplyExact(fijkOrient.translateI, unitScale),
Math.multiplyExact(fijkOrient.translateJ, unitScale),
Math.multiplyExact(fijkOrient.translateK, unitScale)
);
lastCoord.ijkAdd(fijkOrient.translateI * unitScale, fijkOrient.translateJ * unitScale, fijkOrient.translateK * unitScale);
lastCoord.ijkNormalize();

final Vec2d orig2d1 = lastCoord.ijkToHex2d();
Expand Down Expand Up @@ -584,18 +580,10 @@ public CellBoundary faceIjkToCellBoundary(final int res, final int start, final
// to each vertex to translate the vertices to that cell.
final int[] vertexLast = verts[lastV];
final int[] vertexV = verts[v];
scratch2.reset(
Math.addExact(vertexLast[0], this.coord.i),
Math.addExact(vertexLast[1], this.coord.j),
Math.addExact(vertexLast[2], this.coord.k)
);
scratch2.reset(vertexLast[0] + this.coord.i, vertexLast[1] + this.coord.j, vertexLast[2] + this.coord.k);
scratch2.ijkNormalize();
final Vec2d orig2d0 = scratch2.ijkToHex2d();
scratch2.reset(
Math.addExact(vertexV[0], this.coord.i),
Math.addExact(vertexV[1], this.coord.j),
Math.addExact(vertexV[2], this.coord.k)
);
scratch2.reset(vertexV[0] + this.coord.i, vertexV[1] + this.coord.j, vertexV[2] + this.coord.k);
scratch2.ijkNormalize();
final Vec2d orig2d1 = scratch2.ijkToHex2d();

Expand Down Expand Up @@ -692,7 +680,7 @@ static long faceIjkToH3(int res, int face, CoordIJK coord) {
scratch.reset(coord.i, coord.j, coord.k);
scratch.downAp7r();
}
scratch.reset(Math.subtractExact(lastI, scratch.i), Math.subtractExact(lastJ, scratch.j), Math.subtractExact(lastK, scratch.k));
scratch.reset(lastI - scratch.i, lastJ - scratch.j, lastK - scratch.k);
scratch.ijkNormalize();
h = H3Index.H3_set_index_digit(h, r, scratch.unitIjkToDigit());
}
Expand Down
33 changes: 17 additions & 16 deletions libs/h3/src/main/java/org/elasticsearch/h3/Vec2d.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ static LatLng hex2dToGeo(double x, double y, int face, int res, boolean substrat

// scale accordingly if this is a substrate grid
if (substrate) {
r /= 3.0;
r *= M_ONETHIRD;
if (H3Index.isResolutionClassIII(res)) {
r *= Constants.M_RSQRT7;
}
Expand Down Expand Up @@ -197,17 +197,17 @@ static CoordIJK hex2dToCoordIJK(double x, double y) {
j = m2;
} else {
i = m1;
j = Math.incrementExact(m2);
j = m2 + 1;
}
} else {
if (r2 < (1.0 - r1)) {
j = m2;
} else {
j = Math.incrementExact(m2);
j = m2 + 1;
}

if ((1.0 - r1) <= r2 && r2 < (2.0 * r1)) {
i = Math.incrementExact(m1);
i = m1 + 1;
} else {
i = m1;
}
Expand All @@ -217,21 +217,21 @@ static CoordIJK hex2dToCoordIJK(double x, double y) {
if (r2 < (1.0 - r1)) {
j = m2;
} else {
j = Math.addExact(m2, 1);
j = m2 + 1;
}

if ((2.0 * r1 - 1.0) < r2 && r2 < (1.0 - r1)) {
i = m1;
} else {
i = Math.incrementExact(m1);
i = m1 + 1;
}
} else {
if (r2 < (r1 * 0.5)) {
i = Math.incrementExact(m1);
i = m1 + 1;
j = m2;
} else {
i = Math.incrementExact(m1);
j = Math.incrementExact(m2);
i = m1 + 1;
j = m2 + 1;
}
}
}
Expand All @@ -242,18 +242,19 @@ static CoordIJK hex2dToCoordIJK(double x, double y) {
if ((j % 2) == 0) // even
{
final int axisi = j / 2;
final int diff = Math.subtractExact(i, axisi);
i = Math.subtractExact(i, Math.multiplyExact(2, diff));
final int diff = i - axisi;
i = i - (2 * diff);
} else {
final int axisi = Math.addExact(j, 1) / 2;
final int diff = Math.subtractExact(i, axisi);
i = Math.subtractExact(i, Math.addExact(Math.multiplyExact(2, diff), 1));
final int axisi = (j + 1) / 2;
final int diff = i - axisi;
i = i - ((2 * diff) + 1);
}
}

if (y < 0.0) {
i = Math.subtractExact(i, Math.addExact(Math.multiplyExact(2, j), 1) / 2);
j = Math.multiplyExact(-1, j);

i = i - ((2 * j + 1) / 2);
j *= -1;
}
final CoordIJK coordIJK = new CoordIJK(i, j, k);
coordIJK.ijkNormalize();
Expand Down

0 comments on commit bd5f989

Please sign in to comment.