Skip to content

Fix stboxFromHexWKB reading past the input buffer (#170)#185

Open
estebanzimanyi wants to merge 2 commits into
MobilityDB:fix/bump-meos-pinfrom
estebanzimanyi:fix/stbox-from-hexwkb-nul-terminate
Open

Fix stboxFromHexWKB reading past the input buffer (#170)#185
estebanzimanyi wants to merge 2 commits into
MobilityDB:fix/bump-meos-pinfrom
estebanzimanyi:fix/stbox-from-hexwkb-nul-terminate

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

Problem

stboxFromHexWKB intermittently failed on osx_arm64 (~66% of runs) with a
spurious Invalid hex string, length (N) has to be a multiple of two!, while
all other architectures passed. PR #170 worked around this by excluding
osx_arm64 from the matrix; this PR fixes the actual bug so the exclusion is
unnecessary.

Root cause

StboxFunctions::Stbox_from_hexwkb passed the DuckDB string_t::GetData()
pointer straight to the MEOS stbox_from_hexwkb():

char *hexwkb = (char*)input_hexwkb.GetData();
STBox *stbox = stbox_from_hexwkb(hexwkb);

stbox_from_hexwkb() derives its length with strlen() (meos/.../stbox.c),
but a string_t payload is not NUL-terminated. strlen() therefore read
past the end of the buffer:

  • On glibc the byte after the allocation is usually 0, so the overrun stopped
    immediately and the bug stayed hidden (linux/osx_amd64 pass).
  • On the macOS arm64 allocator the trailing byte is often non-zero, so strlen
    walked into adjacent heap and returned an odd, oversized length built from the
    valid hex plus garbage — hence the rejection.

This was confirmed under AddressSanitizer: stbox_as_hexwkb returns
strlen=68 / size_out=69, and feeding a non-NUL-terminated 68-byte copy to
stbox_from_hexwkb trips a heap-buffer-overflow READ of size 69, 0 bytes to the right of the 68-byte region at stbox.c:269.

Stbox_from_hexwkb was the only one of the 16 *FromHexWKB consumers reading
the raw GetData() pointer; every sibling already copies into a NUL-terminated
std::string first.

Fix

Copy the input into a NUL-terminated std::string before the call, matching the
sibling consumers. One-line behavioural change; no API or test change required.

@estebanzimanyi
Copy link
Copy Markdown
Member Author

Verified on osx_arm64

The fix was confirmed on the architecture where the bug actually manifested. On the integration base (which compiles against the pinned MEOS) the failure reproduced ~66% of runs before the fix; with the fix the osx_arm64 build + full test suite passes 4/4:

The root cause was also reproduced locally and deterministically under AddressSanitizer: feeding a non-NUL-terminated copy of stbox_as_hexwkb output to stbox_from_hexwkb trips heap-buffer-overflow READ of size 69, 0 bytes to the right of the 68-byte region at stbox.c:269 — the strlen overrun this PR removes.

Note on this PR's own CI

The checks on this branch are red for an unrelated, pre-existing reason: main does not currently build against its pinned MEOS (tydef.hpp aliases meosType/MeosType the wrong way for vcpkg_ports/meos REF f11b7443…), so the matrix fails at compile on every arch (e.g. linux_arm64) before reaching the hex test. That pin/alias mismatch is tracked separately and affects all topical branches off main; it is not introduced or affected by this one-line change. The fix is also folded into the integration branch, where the verification above ran.

Adds the SIZEOF_LONG_LONG emission to the rendered pg_config.h so the
DuckDB-Wasm (wasm32-emscripten / ILP32) build of MEOS no longer fails the
pg_bitutils integer-width check.
Stbox_from_hexwkb passed the DuckDB string_t::GetData() pointer straight to
the MEOS stbox_from_hexwkb(), which derives the length itself with strlen().
A string_t payload is not NUL-terminated, so strlen() ran past the buffer:
on allocators that leave the trailing byte non-zero (macOS arm64) it walked
into adjacent heap and reported a spurious odd-length "Invalid hex string,
length (N)" intermittently, while glibc's zeroed tail hid it elsewhere.

Copy the input into a NUL-terminated std::string before the call, matching
every other *FromHexWKB consumer. This removes the intermittent osx_arm64
failure that MobilityDB#170 worked around by excluding the architecture.
@estebanzimanyi estebanzimanyi changed the base branch from main to fix/bump-meos-pin May 30, 2026 08:00
@estebanzimanyi estebanzimanyi force-pushed the fix/stbox-from-hexwkb-nul-terminate branch from 06dd6ea to b4bf00f Compare May 30, 2026 08:00
@estebanzimanyi estebanzimanyi force-pushed the fix/bump-meos-pin branch 4 times, most recently from fa2de57 to 17588c0 Compare June 2, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant