ffi-code-review

What this skill does

# FFI Code Review

## Review Workflow

1. **Check Cargo.toml** -- Note Rust edition (2024 has breaking changes to extern blocks and unsafe attributes), `build-dependencies` (bindgen, cc, pkg-config), `crate-type` (`cdylib`, `staticlib`), and `links` key
2. **Check build.rs** -- Verify link directives (`cargo:rustc-link-lib`, `cargo:rustc-link-search`), bindgen configuration, and C source compilation
3. **Check extern blocks** -- Verify calling conventions, symbol declarations, and safety annotations
4. **Check type layout** -- Every type crossing FFI must be `#[repr(C)]` or a primitive FFI type
5. **Check string and pointer handling** -- CStr/CString usage, null checks, ownership transfers
6. **Check callbacks** -- `extern "C" fn` pointers, panic safety across FFI boundary
7. **Gates** -- Complete **Gates** below before reporting; do not skip ahead on “internal verification”

## Gates

Complete in order. **Do not emit findings** until **Gate 4** passes for each issue.

**Gate 1 — Crate context (on disk)**  
**PASS when:** You opened the reviewed crate’s `Cargo.toml` (workspace member path if applicable) and recorded `edition =`, plus any of `links`, `crate-type`, or `build-dependencies` that matter for this FFI.  
**Blocks rationalization:** Edition-specific findings (`unsafe extern "C" {}`, `#[unsafe(no_mangle)]`, etc.) require this — if `edition` is not 2024, do not flag 2024-only requirements.

**Gate 2 — Linkage and binding sources**  
**PASS when:** If the crate links native code or uses bindgen/pkg-config, you opened `build.rs` (or the checked-in bindings entry point). If there is no `build.rs`, you stated that bindings are hand-written and reviewed those `extern` / `include!` sites.  
**Artifact:** At least one path you opened (e.g. `build.rs`, `src/ffi.rs`, or `OUT_DIR` bindings via `include!`).

**Gate 3 — Code evidence**  
**PASS when:** Every planned finding has a target `[FILE:LINE]` from a full function/block you read, not only diff hunks or partial snippets.

**Gate 4 — Pre-report protocol**  
**PASS when:** You loaded and applied the [review-verification-protocol](../review-verification-protocol/SKILL.md) skill, including **FFI-Specific Verification** for `repr(C)`, safety comments, ownership/callbacks, or bindgen-heavy code.

## Output Format

Report findings as:

```text
[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.
```

## Quick Reference

| Issue Type | Reference |
|------------|-----------|
| C-to-Rust type mapping, repr(C) layout, enums, opaque types | [references/type-mapping.md](references/type-mapping.md) |
| Safe wrappers, ownership transfer, callbacks, build.rs, testing | [references/safety-patterns.md](references/safety-patterns.md) |

## Review Checklist

### extern Blocks and Calling Conventions
- [ ] Foreign function declarations use `extern "C"` (explicit, not bare `extern`)
- [ ] **Edition 2024**: `extern "C" {}` blocks written as `unsafe extern "C" {}`
- [ ] Functions exposed to C use `extern "C" fn` (not default Rust calling convention)
- [ ] Calling convention matches the foreign library (`"C"`, `"system"` for Win32 API)
- [ ] `#[link(name = "...")]` specifies the correct library name
- [ ] `#[link(name = "...", kind = "static")]` used when statically linking

### Symbol Management
- [ ] Exported functions use `#[no_mangle]` to preserve symbol names
- [ ] **Edition 2024**: `#[no_mangle]` written as `#[unsafe(no_mangle)]`
- [ ] **Edition 2024**: `#[export_name = "..."]` written as `#[unsafe(export_name = "...")]`
- [ ] `#[link_name = "..."]` used when Rust name differs from C symbol
- [ ] Exported items are `pub` (only public `#[no_mangle]` symbols appear in library output)

### Type Layout
- [ ] Every struct/union crossing FFI has `#[repr(C)]` -- Rust's default layout is undefined
- [ ] Primitive types use `std::ffi` / `std::os::raw` equivalents (`c_int`, `c_char`, `c_void`)
- [ ] No bare `i32` where C uses `int` -- use `c_int` (width varies by platform)
- [ ] Quirky C types like `__be32` use byte arrays (`[u8; 4]`), not Rust integers
- [ ] Enums crossing FFI use `#[repr(C)]` or `#[repr(u8)]`/`#[repr(u32)]` with explicit discriminants
- [ ] C-style bitflag enums use a newtype around an integer (or `bitflags` crate), not a Rust enum
- [ ] `#[non_exhaustive]` on enums representing C enumerations that may gain new values

### String Handling
- [ ] C strings use `CStr` (borrowed) or `CString` (owned), never `&str` or `String`
- [ ] `CString::new()` result is checked for interior null bytes (returns `Err` on `\0`)
- [ ] `CString` outlives any `*const c_char` pointer derived from it via `.as_ptr()`
- [ ] Incoming `*const c_char` validated with `CStr::from_ptr()` inside `unsafe`
- [ ] No assumption that C strings are valid UTF-8 -- use `to_str()` which returns `Result`
- [ ] OS paths use `OsStr`/`OsString` and `CStr`, not `&str`

### Ownership and Allocation
- [ ] Clear ownership contract: who allocates, who frees
- [ ] Rust-allocated memory freed by Rust (`Box::from_raw`), C-allocated freed by C
- [ ] `Box::into_raw` / `Box::from_raw` paired correctly for heap transfers
- [ ] `Vec::into_raw_parts` used when passing arrays to C (pointer + length + capacity)
- [ ] Destructor functions exposed for every opaque Rust type given to C
- [ ] No `Drop` running on C-allocated memory (and vice versa)

### Callbacks
- [ ] Callback types are `extern "C" fn(...)`, not closures or `fn(...)`
- [ ] Callbacks use `std::panic::catch_unwind` to prevent panics from unwinding across FFI
- [ ] Callback context passed as `*mut c_void` with safe reconstruction at call site
- [ ] `Option<extern "C" fn(...)>` used for nullable function pointers (niche optimization)

### Bindgen and Build Scripts
- [ ] Bindgen output reviewed for correctness (auto-generated types may need adjustment)
- [ ] `-sys` crate pattern used for raw bindings, separate crate for safe wrappers
- [ ] `build.rs` uses `cargo:rustc-link-lib` and `cargo:rustc-link-search` correctly
- [ ] `links` key in `Cargo.toml` prevents duplicate linking of the same native library
- [ ] Platform-specific bindings generated per-build (not checked in for a single platform)

### Safety Documentation
- [ ] Every `unsafe` block has a `// SAFETY:` comment explaining invariants
- [ ] Every public FFI wrapper function documents safety requirements
- [ ] **Edition 2024**: `unsafe fn` bodies use explicit `unsafe {}` blocks around unsafe ops

## Severity Calibration

### Critical (Block Merge)
- Missing `#[repr(C)]` on types crossing FFI boundary (undefined memory layout)
- Wrong string handling: `&str`/`String` where `CStr`/`CString` required
- Ownership confusion: freeing C-allocated memory with Rust's allocator (or vice versa)
- Panic unwinding across FFI boundary without `catch_unwind`
- Using Rust enum for C bitflags (invalid discriminant = undefined behavior)
- Passing closure where `extern "C" fn` pointer required

### Major (Should Fix)
- Missing safety documentation on `unsafe` blocks or public FFI functions
- No null pointer check on incoming `*const T` / `*mut T` before dereferencing
- `CString` dropped before its pointer is used by C (dangling pointer)
- Missing `#[link(name = "...")]` causing link failures on some platforms
- **Edition 2024**: `extern` block not marked `unsafe extern`
- **Edition 2024**: `#[no_mangle]` not wrapped in `#[unsafe(...)]`

### Minor (Consider Fixing)
- Using `i32` instead of `c_int` for C `int` (correct on most platforms but not portable)
- Missing `#[non_exhaustive]` on enums mapping to extensible C enumerations
- Verbose manual bindings where bindgen would be more maintainable
- Checked-in bindings without platform guards

### Informational
- Suggestions to split raw bindings into a `-sys` crate
- Suggestions to add opaque wrapper types for distinct `*mut c_void` pointers
- Suggestions to use `Option<NonNull<T>>` for nullable pointers

## Valid Patterns (Do NOT Flag)

- **`unsafe extern