Sprimo/skills/unsafe-checker/checklists/review-unsafe.md

Checklist: Reviewing Unsafe Code

Use this checklist when reviewing code containing unsafe.

1. Surface-Level Checks

• Does every unsafe block have a // SAFETY: comment?
• Does every unsafe fn have # Safety documentation?
• Are the safety comments specific and verifiable, not vague?
• Is the unsafe code minimized (smallest possible unsafe block)?

2. Pointer Validity

For each pointer dereference:

• Non-null: Is null checked before dereference?
• Aligned: Is alignment verified or guaranteed by construction?
• Valid: Does the pointer point to allocated memory?
• Initialized: Is the memory initialized before reading?
• Lifetime: Is the memory valid for the entire use duration?
• Unique: For &mut, is there only one mutable reference?

3. Memory Safety

• No aliasing: Are & and &mut never created to the same memory simultaneously?
• No use-after-free: Is memory not accessed after deallocation?
• No double-free: Is memory freed exactly once?
• No data races: Is concurrent access properly synchronized?
• Bounds checked: Are array/slice accesses in bounds?

4. Type Safety

• Transmute: Are transmuted types actually compatible?
• Repr: Do FFI types have #[repr(C)]?
• Enum values: Are enum discriminants validated from external sources?
• Unions: Is the correct union field accessed?

5. Panic Safety

• What state is the program in if this code panics?
• Are partially constructed objects properly cleaned up?
• Do Drop implementations see valid state?
• Is there a panic guard if needed?

6. FFI-Specific Checks

• Types: Do Rust types match C types exactly?
• Strings: Are strings properly null-terminated?
• Ownership: Is it clear who owns/frees memory?
• Thread safety: Are callbacks thread-safe?
• Panic boundary: Are panics caught before crossing FFI?
• Error handling: Are C-style errors properly handled?

7. Concurrency Checks

• Send/Sync: Are manual implementations actually sound?
• Atomics: Are memory orderings correct?
• Locks: Is there potential for deadlock?
• Data races: Is all shared mutable state synchronized?

8. Red Flags (Require Extra Scrutiny)

Pattern	Concern
transmute	Type compatibility, provenance
as on pointers	Alignment, type punning
static mut	Data races
*const T as *mut T	Aliasing violation
Manual Send/Sync	Thread safety
assume_init	Initialization
set_len on Vec	Uninitialized memory
from_raw_parts	Lifetime, validity
offset/add/sub	Out of bounds
FFI callbacks	Panic safety

9. Verification Questions

Ask the author:

• "What would happen if [X invariant] was violated?"
• "How do you know [pointer/reference] is valid here?"
• "What if this panics at [specific line]?"
• "Who is responsible for freeing this memory?"

10. Testing Requirements

• Has this been tested with Miri?
• Are there unit tests covering edge cases?
• Are there tests for error conditions?
• Has concurrent code been tested under stress?

Review Severity Guide

Severity	Requires
transmute	Two reviewers, Miri test
Manual Send/Sync	Thread safety expert review
FFI	Documentation of C interface
static mut	Justification for not using atomic/mutex
Pointer arithmetic	Bounds proof

Sample Review Comments

// Good SAFETY comment ✓
// SAFETY: index was checked to be < len on line 42

// Needs improvement ✗
// SAFETY: This is safe because we know it works

// Missing information ✗
// SAFETY: ptr is valid
// (Why is it valid? How do we know?)