Skip to content

Guard antimeridian boundary vertex appends#205

Open
Komzpa wants to merge 1 commit into
mainfrom
codex/propose-fix-for-cellboundary-overflow-vulnerability
Open

Guard antimeridian boundary vertex appends#205
Komzpa wants to merge 1 commit into
mainfrom
codex/propose-fix-for-cellboundary-overflow-vulnerability

Conversation

@Komzpa

@Komzpa Komzpa commented Jun 11, 2026

Copy link
Copy Markdown
Member

Motivation

  • Prevent stack-buffer-overflow when splitting polar/antimeridian-crossing H3 cell boundaries by ensuring we never append past the fixed CellBoundary.verts capacity.

Description

  • Add a capacity-checked helper boundary_add_vert(CellBoundary *, const LatLng *) that asserts the CellBoundary has room before appending a vertex in h3_postgis/src/wkb_indexing.c.
  • Replace all direct part->verts[part->numVerts++] and res->verts[res->numVerts++] writes in boundary_split_180 and boundary_split_180_polar with calls to boundary_add_vert to enforce the bound.
  • Preserve existing behavior by raising the existing split assertion (SPLIT_ASSERT) when a split would exceed the vertex array instead of allowing silent overflow.

Testing

  • Performed a syntax-only compile check using cc -fsyntax-only with temporary PostgreSQL/H3/WKB stubs, which succeeded.
  • Ran git diff --check to validate no whitespace/git-diff issues, which passed.
  • Attempted full CMake configuration with cmake -S . -B build -DBUILD_TESTING=OFF, which failed due to an external FetchContent download of H3 v4.5.0 returning HTTP 403 from the environment proxy, so full integration build/tests could not complete in this environment.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78277aeed5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const int maxVerts = sizeof(boundary->verts) / sizeof(boundary->verts[0]);

SPLIT_ASSERT(
boundary->numVerts < maxVerts,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use a larger split buffer for seam vertices

When a valid H3 boundary already uses more than six of CellBoundary's fixed vertex slots, the polar splitter appends the original vertices plus four synthetic antimeridian/pole vertices; this new guard turns those cases into Cell boundary split exceeds CellBoundary vertex capacity errors for h3_cell_to_boundary_wkb/geometry instead of producing the boundary. The bounds check needs to be paired with a split representation sized for the extra seam vertices, otherwise high-resolution polar or distorted antimeridian cells remain unusable.

Useful? React with 👍 / 👎.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 331e0a81-50e5-47e1-86f4-55e681edf6d1

📥 Commits

Reviewing files that changed from the base of the PR and between 9d70e9c and 78277ae.

📒 Files selected for processing (1)
  • h3_postgis/src/wkb_indexing.c

📝 Walkthrough

Walkthrough

This PR adds a new boundary_add_vert helper function to centralize bounds checking when appending vertices to a CellBoundary. All direct vertex buffer writes in boundary_split_180 and boundary_split_180_polar are replaced with calls to this helper, which asserts the fixed vertex capacity constraint.

Changes

Boundary Vertex Capacity Enforcement

Layer / File(s) Summary
Boundary vertex helper definition
h3_postgis/src/wkb_indexing.c
Introduces boundary_add_vert, a static helper that appends a LatLng vertex to a CellBoundary while asserting the vertex buffer capacity via SPLIT_ASSERT.
Standard antimeridian splitting integration
h3_postgis/src/wkb_indexing.c
boundary_split_180 routes current vertex and split/intersection vertex additions through boundary_add_vert, replacing direct boundary->verts[...++] writes while preserving vertex construction.
Polar antimeridian splitting integration
h3_postgis/src/wkb_indexing.c
boundary_split_180_polar updates current vertex and final intersection point insertions to use boundary_add_vert, centralizing capacity enforcement in polar boundary expansion.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A boundary grows, one vertex blessed,
Each point now checked before it rests,
Split and polar paths align—
One helper guards the sacred line! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Guard antimeridian boundary vertex appends' directly and clearly summarizes the main change: adding guard/bounds-checking to vertex append operations in antimeridian boundary processing.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the motivation for preventing buffer overflows, the specific helper function added, and the affected functions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/propose-fix-for-cellboundary-overflow-vulnerability

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant