Skip to content
This repository was archived by the owner on Jun 16, 2026. It is now read-only.

Document FFI safety contracts for bridge entry points - #3

Open
Sanjays2402 wants to merge 2 commits into
b-nnett:mainfrom
Sanjays2402:docs/bridge-ffi-safety
Open

Document FFI safety contracts for bridge entry points#3
Sanjays2402 wants to merge 2 commits into
b-nnett:mainfrom
Sanjays2402:docs/bridge-ffi-safety

Conversation

@Sanjays2402

Copy link
Copy Markdown

What

The two C ABI entry points exposed to the iOS shell — goose_bridge_handle_json and goose_bridge_free_string in Rust/core/src/bridge.rs — are declared pub unsafe extern "C" but ship without a documented safety contract. Clippy flags both with missing_safety_doc, and more importantly the bridge is the cross-language interface, so its preconditions shouldn't have to be reconstructed from the implementation.

This PR adds rustdoc # Safety sections covering:

  • pointer validity, null handling, and UTF-8 expectations for request_json
  • aliasing rules for the duration of the call
  • ownership transfer of the returned C string and the requirement to release it through goose_bridge_free_string (not free(3) — the Rust allocator backing CString is not guaranteed to match the host's, and Swift's swift_demangle-style helpers can hide this)
  • double-free and foreign-allocator pitfalls on the free side

Treating this header as a stable contract is also the spirit of #2 — a documented safety contract is a prerequisite for the C ABI being something a future Android/JNI or Flutter/Dart FFI port can rely on without reading Rust source.

Why this matters in practice

The Swift bridge code (GooseRustBridge.swift) already has to obey these rules implicitly. Documenting them:

  1. Locks the contract in the same place as the implementation, so it stays correct when the Rust side evolves.
  2. Surfaces concrete pitfalls (mismatched allocator, double-free, lifetime of request_json) to any future contributor wiring a new platform shell.
  3. Makes the next clippy run quieter on the file that matters most for soundness.

Change

Rust/core/src/bridge.rs only — two doc-comment blocks added, no code changes.

Verification

  • cargo build --lib → clean (1m 23s, no new warnings, no new errors).
  • cargo clippy --lib --no-deps → the two missing_safety_doc warnings on bridge.rs:2505 and bridge.rs:2529 are gone (0 remaining for that lint on this file).
  • No behaviour change. Pure documentation.

Test plan

  • Builds clean on macOS host (cargo build --lib)
  • Clippy missing_safety_doc warnings on bridge entry points resolved
  • CI build on iOS targets stays green (no code change, expected to pass)

Notes

  • Kept the diff minimal — no rustfmt reflow, no clippy --fix sweep, nothing outside the two functions. If you'd like me to follow up with a broader safety/clippy pass (there are ~120 other warnings, mostly collapsible_if and too_many_arguments style nits), happy to send that as a separate PR.
  • I'm running on a Mac without full Xcode, so I haven't built the iOS app — only the Rust core. Flagging in case anything iOS-side surprises you.

The two C ABI entry points exposed to the iOS shell — goose_bridge_handle_json
and goose_bridge_free_string — were marked unsafe extern "C" without a
documented safety contract. Clippy flagged this (missing_safety_doc); more
importantly, the bridge IS the cross-language interface, so its preconditions
should not have to be reconstructed from the implementation.

Add rustdoc # Safety sections covering:
- pointer validity, null handling, and UTF-8 expectations for the request
  buffer
- aliasing rules during the call
- ownership transfer of the returned C string and the requirement to free it
  through goose_bridge_free_string (not free(3) — allocator mismatch is UB)
- double-free and foreign-allocator pitfalls on the free side

No behaviour change. cargo build --lib stays clean; the two
clippy::missing_safety_doc warnings on bridge.rs:2505 and bridge.rs:2529
are now resolved (0 remaining).
goamorim added a commit to goamorim/goose that referenced this pull request Jun 3, 2026
tigercraft4 referenced this pull request in tigercraft4/goose Jun 3, 2026
tigercraft4 referenced this pull request in tigercraft4/goose Jun 3, 2026
…osure

- main.py: remove user-supplied body.device from log entry (log injection)
- main.py: return type(exc).__name__ instead of str(exc) (stack trace exposure)
- daily.py: remove device_id from warning log entry (log injection)

Fixes CodeQL alerts #1, #2, #3
Comment thread Rust/core/src/bridge.rs
/// - `request_json` is either null **or** a valid pointer to a
/// null-terminated UTF-8 C string that remains valid (and unmodified by
/// other threads) for the duration of this call.
/// - The buffer referenced by `request_json` is not aliased by any mutable

@tigercraft4 tigercraft4 Jun 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing thread-safety contract.

The # Safety section correctly covers pointer validity, UTF-8, aliasing for the duration of a single call, and ownership transfer. What is not stated: whether goose_bridge_handle_json is safe to call concurrently from multiple threads.

If the bridge dispatches into stateful Rust (SQLite via rusqlite, any global), concurrent calls could race. If it is safe (because each call opens its own connection and the Rust side is stateless), that guarantee is as important to document as the pointer invariants — it determines whether a Swift caller can dispatch bridge calls from concurrent DispatchQueues without a lock.

Suggest adding: /// This function may be called concurrently from multiple threads — [safe/unsafe because ...]

Comment thread Rust/core/src/bridge.rs
/// `goose_core_*` function.
///
/// # Safety
///

@tigercraft4 tigercraft4 Jun 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doc-drift risk — no CI hook keeps the safety text in sync with behavior.

The goose_bridge_free_string safety section correctly documents ownership transfer and the double-free pitfall. However, if the implementation changes (e.g. the allocation strategy behind CString changes, or a wrapper is added), these docs will silently become wrong.

Minimum mitigation: reference the exact implementation so a future maintainer editing the code knows to update the doc:

/// Releases the string by calling [`CString::from_raw`] on `value`,
/// which reclaims the memory via the Rust allocator that produced it
/// (see the `string_to_c_string` helper). Do not pass a pointer
/// allocated by any other means.

tigercraft4 referenced this pull request in tigercraft4/goose Jun 5, 2026
tigercraft4 referenced this pull request in tigercraft4/goose Jun 5, 2026
…osure

- main.py: remove user-supplied body.device from log entry (log injection)
- main.py: return type(exc).__name__ instead of str(exc) (stack trace exposure)
- daily.py: remove device_id from warning log entry (log injection)

Fixes CodeQL alerts #1, #2, #3
mitchellfgibson added a commit to mitchellfgibson/whoop that referenced this pull request Jun 15, 2026
b-nnett#3 (v2.8.6): v26 PpgHr record-rate notch (#194) — de-artifact the per-record
comb before autocorrelation so a sub-60-bpm sleeper can't snap to a false
60 bpm; gated on a record-boundary discontinuity so a true 60 is preserved.

b-nnett#4 (v4.0.0): port FitnessAgeEngine + VitalityEngine (HUNT-study VO2max /
fitness-age regression + a hazard-weighted Body Age) verbatim, and surface a
self-contained Vitality / Body Age hero on the Health Monitor screen that
computes live from the last 14 days of repo.days + profile — no precompute
pipeline needed. Honest: a fitness comparison, not a biological/medical age.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants