Skip to content

fix(ffi): regenerate hiroz_ffi.h to match the Rust FFI surface - #343

Closed
richardw347 wants to merge 1 commit into
ZettaScaleLabs:mainfrom
richardw347:fix/ffi-header-drift
Closed

richardw347 wants to merge 1 commit into
ZettaScaleLabs:mainfrom
richardw347:fix/ffi-header-drift

Conversation

@richardw347

Copy link
Copy Markdown
Contributor

Summary

The committed C header crates/hiroz-go/hiroz/hiroz_ffi.h has drifted from the
#[repr(C)] types in crates/hiroz/src/ffi. The header wasn't regenerated after
several FFI changes landed, so consumers compile against a stale ABI.

The consequential drift is in hiroz_context_config_t. The Rust
CContextConfig ends with a namespace: *const c_char field, read in
hiroz_context_create_with_config:

// crates/hiroz/src/ffi/context.rs
pub struct CContextConfig {
    ...
    pub enable_logging: bool,
    pub namespace: *const c_char,   // 12th field
}
...
if let Some(Ok(namespace)) = (!cfg.namespace.is_null()).then(|| cstr_to_str(cfg.namespace)) {
    builder = builder.with_namespace(namespace);
}

…but the committed header stops at enable_logging (11 fields):

typedef struct hiroz_context_config_t {
  ...
  bool enable_logging;
} hiroz_context_config_t;   // no namespace_

A caller that allocates this struct from the header gets one 8 bytes smaller
than the Rust side reads. hiroz_context_create_with_config then dereferences
cfg.namespace — memory just past the caller's allocation, i.e. an
uninitialised/garbage pointer — and passes it to cstr_to_str(). That is UB;
in practice it presents either as a segfault during context creation or as a
spurious -16 "failed to build entity: context" when the garbage happens to be
a non-null but invalid pointer.

The header had also fallen behind on constants that exist in the FFI surface
(KEEP_ALL_CACHE_DEPTH, the parameter-type enum values, DEPTH_RECURSIVE) and
on a couple of doc comments.

Fix

Regenerate the header from the crate's own build.rs cbindgen path
(cargo build --features ffi, cbindgen 0.29.4). No hand edits — the diff is
purely generator output, and every added item corresponds to a real exported
Rust symbol (verified: KEEP_ALL_CACHE_DEPTH in qos, DEPTH_RECURSIVE in
parameter/wire_types, namespace_ in CContextConfig). Nothing is removed.

How we hit this

We consume libhiroz.a from Go via cgo. Against a busy rmw_zenoh_cpp router,
hiroz_context_create_with_config intermittently returned -16 / segfaulted.
The 8-byte struct mismatch was one of the contributing ABI hazards we traced
(and it is latent for any header-based consumer, not just ours).

Suggested follow-up (optional, not in this PR)

To stop the header re-drifting, consider a CI check that regenerates the header
and fails if it differs from the committed copy (cbindgen ... --output - | diff),
or make build.rs hard-error when the ffi feature is set but cbindgen is
absent (it currently prints a warning and silently ships the stale header).

Verification

  • cargo build --release --features ffi,jazzy --no-default-features -p hiroz
    regenerates the header cleanly; struct tail now ends const char *namespace_;.
  • Diff is +57/-4, additions only, all backed by real FFI symbols.

The committed C header had drifted from the #[repr(C)] types in
crates/hiroz/src/ffi. Most importantly hiroz_context_config_t was
missing its trailing `namespace` field: the Rust CContextConfig has it
(read in hiroz_context_create_with_config), but the header stopped at
`enable_logging`. A caller compiling against this header allocates a
struct 8 bytes smaller than the Rust side expects, so the FFI reads
`cfg.namespace` past the caller's allocation — an uninitialised pointer
that is then passed to cstr_to_str(), i.e. UB / a likely segfault or a
spurious context-build failure.

The header had also fallen behind on several constants that exist in
the FFI surface (KEEP_ALL_CACHE_DEPTH, the parameter-type enum values,
DEPTH_RECURSIVE) plus updated doc comments.

Regenerated with cbindgen 0.29.4 via the crate's own build.rs
(cargo build --features ffi), so it now matches the compiled staticlib.
No hand edits; purely the generator output.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Sep 7, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@YuanYuYuan

Copy link
Copy Markdown
Collaborator

Reviewed this against the current FFI surface. Looks clean — purely additive cbindgen output, and every new symbol traces back to a real Rust export with matching values:

  • namespace_ on hiroz_context_config_t matches CContextConfig::namespace in context.rs.
  • KEEP_ALL_CACHE_DEPTH (42) matches qos::KEEP_ALL_CACHE_DEPTH.
  • DEPTH_RECURSIVE (0) and the parameter-type enum values (BOOL=1 .. STRING_ARRAY=9) match parameter::wire_types.
  • CDR_HEADER_LE ({0, 1, 0, 0}) matches msg::CDR_HEADER_LE.

The namespace_ field is the one that actually matters here, not just a cosmetic diff: without it, hiroz_context_config_t was 8 bytes smaller than what hiroz_context_create_with_config reads, so a caller allocating the struct from the old header hit UB — a garbage namespace pointer, surfacing as either a segfault or a spurious -16 error. This fixes a real ABI hazard, matching what the PR description describes.

One pre-existing oddity, not introduced here: the regenerated doc comment on hiroz_service_client_wait_for_service now says "client is null", but the parameter is still named client_handle. That's inherited from the Rust doc comment (the regen just reflects it faithfully), so nothing to fix in this PR — worth a follow-up on the Rust side if it's ever touched again.

No concerns — this is a straightforward, correct regen.

@YuanYuYuan

Copy link
Copy Markdown
Collaborator

Following up on the review above: it turns out there was already an internal PR (#277) fixing this exact namespace_ drift, opened before this one and sitting unmerged — sorry for the overlap, that's on us for not landing it sooner.

#277 additionally adds a CI check that regenerates the header at build time and fails if it drifts from what's committed, so it covers this defect and prevents it from recurring. It'll land instead of this PR, with credit to you in its description for independently catching the same bug through a real Go/cgo consumer — thanks for the report and the clean fix.

Closing this in favor of #277.

@YuanYuYuan YuanYuYuan closed this Sep 10, 2026
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.

3 participants