fix!: Soundness of Zend API

.github/workflows/build.yml

@@ -32,7 +32,7 @@ jobs:
       - name: Setup Mago
         uses: nhedger/setup-mago@v2
         with:
-          version: 1.45.0
+          version: 1.47.6
       - name: Check PHP formatting
         run: mago format --check
       - name: Lint PHP

benches/benches/binary_bench.rs

@@ -122,6 +122,11 @@ fn array_interned_keys(cnt: usize) -> ExitStatus {
     run_php("array_interned_keys.php", cnt)
 }
 
+#[divan::bench(args = [1, 10, 100_000])]
+fn binary_slice_reads(cnt: usize) -> ExitStatus {
+    run_php("binary_slice.php", cnt)
+}
+
 fn main() {
     setup();
     divan::main();

benches/benches/binary_slice.php

@@ -0,0 +1,9 @@
+<?php
+
+declare(strict_types = 1);
+
+$packed = pack('P*', ...range(1, 64));
+
+foreach (range(1, $argv[1]) as $i) {
+    bench_binary_slice_sum($packed);
+}

benches/ext/src/lib.rs

@@ -7,11 +7,17 @@
 )]
 
 use ext_php_rs::{
+    binary_slice::BinarySlice,
     boxed::ZBox,
     prelude::*,
     types::{ZendHashTable, ZendStr},
 };
 
+#[php_function]
+pub fn bench_binary_slice_sum(values: BinarySlice<u64>) -> u64 {
+    values.iter().sum()
+}
+
 #[php_function]
 pub fn bench_function(n: u64) -> u64 {
     n
@@ -137,6 +143,7 @@ pub fn build_module(module: ModuleBuilder) -> ModuleBuilder {
         .function(wrap_function!(bench_callback_function))
         .function(wrap_function!(bench_array_with_str_ref_keys))
         .function(wrap_function!(bench_array_with_interned_keys))
+        .function(wrap_function!(bench_binary_slice_sum))
         .class::<BenchClass>()
         .class::<BenchProps>()
         .globals(&INTERNED_KEYS)

flake.nix

@@ -56,10 +56,10 @@
       # local dev and CI (nhedger/setup-mago) run the exact same version.
       mago = pkgs.stdenvNoCC.mkDerivation rec {
         pname = "mago";
-        version = "1.45.0";
+        version = "1.47.6";
         src = pkgs.fetchurl {
           url = "https://github.com/carthage-software/mago/releases/download/${version}/mago-${version}-x86_64-unknown-linux-musl.tar.gz";
-          hash = "sha256-aNsEDrmx3uGPvf9iTBN1TPdM0z58W2CZHh/jX9mxkNE=";
+          hash = "sha256-1A7DxEHspUvhsbpU/hdTD41CTS3CP4x8puptppq25Zg=";
         };
         installPhase = ''
           runHook preInstall

guide/src/advanced/custom_sapi.md

@@ -118,7 +118,9 @@ let module = MySapi::build_module().expect("failed to build SAPI module");
 ```
 
 The returned `SapiModule` can then be passed to `sapi_startup()` and
-`php_module_startup()` just like a manually-built one.
+`php_module_startup()` just like a manually-built one. Both copy the struct by
+value (`sapi_module = *sf`), so a local variable is enough: keep it alive until
+`sapi_shutdown()` has returned and pass `&raw mut module`.
 
 The builder places the SAPI's `name`, `pretty_name`, `executable_location` and
 `php_ini_path_override` strings on the heap. PHP never frees them, so after

guide/src/migration-guides/v0.16.md

@@ -421,3 +421,142 @@ the one the CLI was built with. `Module::enums` is always present and
 `Class::closure()` is always available; the `enum` and `closure` features only
 decide whether `ext-php-rs` fills them.
 
+
+## `PackSlice::unpack_into` Requires an Unsafe Block
+
+`PackSlice::unpack_into` builds a `&[T]` over the bytes of a `zend_string`, so
+its length and alignment preconditions are now part of the signature.
+`Zval::binary_slice` checks both and stays safe. `Pack::unpack_into` stays safe
+as well: it now reads unaligned and never relied on alignment.
+
+### Before (v0.15)
+
+```rust,ignore
+let values: &[u64] = u64::unpack_into(zend_str);
+```
+
+### After (v0.16)
+
+```rust,ignore
+// Preferred: the checked entry point.
+let values: &[u64] = zval.binary_slice::<u64>()?;
+
+// Or, when the checks are done by hand:
+// SAFETY: `zend_str.len` is a multiple of 8 and `val` is 8-byte aligned.
+let values: &[u64] = unsafe { u64::unpack_into(zend_str) };
+```
+
+`Zval::binary_slice` also returns `None` when the string bytes are not aligned
+for `T`. Zend allocates every string on an 8-byte boundary, so this only fires
+for types wider than 8 bytes or strings from a foreign allocator.
+
+## `Zval::set_ptr` Requires an Unsafe Block
+
+`Zval::set_ptr` stores a raw pointer in an `IS_PTR` zval. The engine reads
+`IS_PTR` entries with a fixed type in several of its tables (property tables as
+`zend_property_info *`, constant tables as `zend_class_constant *`, INI
+directives as `zend_ini_entry *`), so storing an arbitrary pointer and letting
+the zval reach one of those tables is undefined behaviour.
+
+### Before (v0.15)
+
+```rust,ignore
+zval.set_ptr(ptr);
+```
+
+### After (v0.16)
+
+```rust,ignore
+// SAFETY: `ptr` outlives the zval and the zval never reaches an engine table.
+unsafe { zval.set_ptr(ptr) };
+```
+
+`set_ptr` now also releases the previous value of the zval instead of
+overwriting it in place.
+
+## Iterators Yield Owned Values
+
+`ZendIterator::get_current_data` returned `&'a Zval` with an unconstrained
+`'a`, so callers could keep the reference past the next `move_forward`. Zend
+releases that zval on every step (`zend_user_it_invalidate_current` runs
+`zval_ptr_dtor` on it), so the reference dangled. The reference is now tied to
+the `&mut self` borrow, and both `ZendIterator`'s and `Iterable`'s `Iter` yield
+`(Zval, Zval)`: the value is a shallow clone, the same copy `foreach` makes in
+`ZEND_FE_FETCH_R`.
+
+### Before (v0.15)
+
+```rust,ignore
+let values: Vec<&Zval> = iterator.iter()?.map(|(_, v)| v).collect();
+```
+
+### After (v0.16)
+
+```rust,ignore
+let values: Vec<Zval> = iterator.iter()?.map(|(_, v)| v).collect();
+```
+
+Destructuring `for (k, v) in iterator.iter()?` keeps working; `v` is a `Zval`
+instead of a `&Zval`, so drop the explicit `&` where you spelled the type.
+
+## `ZBox::into_raw` Returns `*mut T`
+
+`ZBox::into_raw` fabricated a `&'static mut T`. It now mirrors `Box::into_raw`
+and returns `*mut T`, which `ZBox::from_raw` accepts back unchanged.
+
+### Before (v0.15)
+
+```rust,ignore
+let obj: &'static mut ZendObject = zbox.into_raw();
+zval.set_object(obj);
+```
+
+### After (v0.16)
+
+```rust,ignore
+let obj: *mut ZendObject = zbox.into_raw();
+// SAFETY: `into_raw` yields a valid, exclusively owned object.
+zval.set_object(unsafe { &mut *obj });
+```
+
+## Heap-Leaking `into_raw` Constructors Removed
+
+`FunctionEntry::into_raw`, `IniEntryDef::into_raw`, `SapiModule::into_raw` and
+the deprecated `ModuleEntry::into_raw` boxed a value and returned the pointer
+with no owner. None of them was needed: `ModuleBuilder` and `ClassBuilder` hand
+function tables to the engine themselves, `IniEntryDef::register` takes a
+`Vec`, and `sapi_startup`/`php_module_startup` copy the `sapi_module_struct`
+by value (`sapi_module = *sf`), so a local is enough.
+
+### Before (v0.15)
+
+```rust,ignore
+let sapi = builder.build()?.into_raw();
+unsafe { sapi_startup(sapi) };
+// ...
+unsafe { cleanup_sapi_allocations(sapi); drop(Box::from_raw(sapi)) };
+```
+
+### After (v0.16)
+
+```rust,ignore
+let mut sapi = builder.build()?;
+unsafe { sapi_startup(&raw mut sapi) };
+// ...
+unsafe { cleanup_sapi_allocations(&raw mut sapi) };
+```
+
+Keep the `SapiModule` alive until `sapi_shutdown()` has returned: the engine
+copies the struct but keeps reading the strings it points to.
+
+## `GlobalConstantFlags::CaseSensitive` Removed
+
+The flag was deprecated and had no effect since PHP 8.0, where constants are
+always case-sensitive. Remove it from any flag set you build.
+
+## `alloc::emalloc` Honours `Layout::align`
+
+`emalloc` ignored the alignment of the `Layout` it was given. The Zend allocator
+guarantees `ZEND_MM_ALIGNMENT` (8 bytes) and has no aligned-allocation entry
+point, so `emalloc` now returns a null pointer for any `Layout` whose alignment
+exceeds 8 instead of handing back under-aligned memory.

guide/src/types/binary_slice.md

@@ -16,6 +16,12 @@ string pointer, with the length of the array being the length of the string.
 implemented on most primitive numbers (i8, i16, i32, i64, u8, u16, u32, u64,
 isize, usize, f32, f64).
 
+The conversion fails (the argument is rejected) when the string length is not a
+whole multiple of `size_of::<T>()` or when the string bytes are not aligned for
+`T`. Zend allocates strings on 8-byte boundaries, so the alignment check only
+fails for exotic allocators. `PackSlice::unpack_into` itself is `unsafe`; use
+`Zval::binary_slice`, which performs both checks.
+
 [`pack`]: https://www.php.net/manual/en/function.pack.php
 [`unpack`]: https://www.php.net/manual/en/function.unpack.php
 

guide/src/types/iterable.md

@@ -10,6 +10,9 @@ Converting from a zval to a `Iterable` is valid when the value is either an arra
 that implements the `Traversable` interface. This means that any value that can be used in a
 `foreach` loop can be converted into a `Iterable`.
 
+Both variants yield owned `(Zval, Zval)` pairs; see [`ZendIterator`](./iterator.md)
+for why the `Traversable` side cannot lend references.
+
 ## Rust example
 
 ```rust,no_run

guide/src/types/iterator.md

@@ -13,6 +13,10 @@ used but also a the result of a `query` call with `PDO`.
 
 If you want a more universal `iterable` type that also supports arrays, see [Iterable](./iterable.md).
 
+Each iteration yields an owned `(Zval, Zval)` pair. The value is a shallow clone
+of the zval Zend hands out, because the engine releases its own copy on the
+next `move_forward`; this is the same copy `foreach` performs.
+
 ## Rust example
 
 ```rust,no_run

src/alloc.rs

@@ -3,24 +3,31 @@
 
 use cfg_if::cfg_if;
 
-use crate::ffi::{_efree, _emalloc, _estrdup};
+use crate::ffi::{_efree, _emalloc, _estrdup, ZEND_MM_ALIGNMENT};
 use std::{
     alloc::Layout,
     ffi::{CString, c_char, c_void},
 };
 
 /// Uses the PHP memory allocator to allocate request-bound memory.
 ///
+/// The Zend allocator only guarantees `ZEND_MM_ALIGNMENT` (8 bytes) and has
+/// no aligned-allocation entry point, so a `layout` requesting a stricter
+/// alignment cannot be honoured and yields a null pointer.
+///
 /// # Parameters
 ///
 /// * `layout` - The layout of the requested memory.
 ///
 /// # Returns
 ///
-/// A pointer to the memory allocated.
+/// A pointer to the memory allocated, or null if `layout.align()` exceeds
+/// `ZEND_MM_ALIGNMENT`.
 #[must_use]
 pub fn emalloc(layout: Layout) -> *mut u8 {
-    // TODO account for alignment
+    if layout.align() > ZEND_MM_ALIGNMENT.unsigned_abs() {
+        return std::ptr::null_mut();
+    }
     let size = layout.size();
 
     (unsafe {

src/binary.rs

@@ -123,7 +123,8 @@ pub unsafe trait Pack: Clone {
     /// format. Note that the data *must* be all one type, as this
     /// implementation only unpacks one type.
     ///
-    /// # Safety
+    /// Trailing bytes that do not fill a whole `Self` are ignored. The bytes
+    /// are read unaligned, so any valid `zend_string` is accepted.
     ///
     /// There is no way to tell if the data stored in the string is actually of
     /// the given type. The results of this function can also differ from
@@ -154,25 +155,18 @@ macro_rules! pack_impl {
             }
 
             fn unpack_into(s: &zend_string) -> Vec<Self> {
-                #[allow(clippy::cast_lossless)]
-                let bytes = ($d / 8) as u64;
-                let len = (s.len as u64) / bytes;
-                let mut result =
-                    Vec::with_capacity(len.try_into().expect("Capacity integer overflow"));
-                // TODO: Check alignment
+                let len = s.len / ($d as usize / 8);
+                // Only `read_unaligned` dereferences this pointer, so the alignment
+                // the cast would normally promise is never relied on.
                 #[allow(clippy::cast_ptr_alignment)]
                 let ptr = s.val.as_ptr().cast::<$t>();
 
-                // SAFETY: We calculate the length of memory that we can legally read based on
-                // the side of the type, therefore we never read outside the memory we
-                // should.
-                for i in 0..len {
-                    result.push(unsafe {
-                        *ptr.offset(i.try_into().expect("Offset integer overflow"))
-                    });
-                }
-
-                result
+                // SAFETY: a valid `zend_string` has `len` readable bytes at `val`, and
+                // `len * size_of::<Self>() <= s.len`. `read_unaligned` has no alignment
+                // requirement, so a 1-byte-aligned `val` is fine.
+                (0..len)
+                    .map(|i| unsafe { ptr.add(i).read_unaligned() })
+                    .collect()
             }
         }
     };

src/binary_slice.rs

@@ -88,20 +88,32 @@ pub unsafe trait PackSlice: Clone {
     /// format. Note that the data *must* be all one type, as this
     /// implementation only unpacks one type.
     ///
-    /// # Safety
-    ///
     /// There is no way to tell if the data stored in the string is actually of
     /// the given type. The results of this function can also differ from
     /// platform-to-platform due to the different representation of some
     /// types on different platforms. Consult the [`pack`] function
     /// documentation for more details.
     ///
+    /// # Safety
+    ///
+    /// The returned slice aliases the string's bytes, so the caller must
+    /// guarantee:
+    ///
+    /// * `s.len` is a whole multiple of `size_of::<Self>()`.
+    /// * `s.val` is aligned to `align_of::<Self>()`. Zend allocates strings on
+    ///   `ZEND_MM_ALIGNMENT` (8 bytes) boundaries and `val` sits at an 8-byte
+    ///   offset, but the `zend_string` type itself only promises 1-byte
+    ///   alignment for `val`.
+    ///
+    /// [`Zval::binary_slice`](crate::types::Zval::binary_slice) performs both
+    /// checks and is the safe entry point.
+    ///
     /// # Parameters
     ///
     /// * `s` - The Zend string containing the binary data.
     ///
     /// [`pack`]: https://www.php.net/manual/en/function.pack.php
-    fn unpack_into(s: &zend_string) -> &[Self];
+    unsafe fn unpack_into(s: &zend_string) -> &[Self];
 }
 
 /// Implements the [`PackSlice`] trait for a given type.
@@ -112,12 +124,13 @@ macro_rules! pack_slice_impl {
 
     ($t: ty, $d: expr) => {
         unsafe impl PackSlice for $t {
-            fn unpack_into(s: &zend_string) -> &[Self] {
-                let bytes = ($d / 8) as usize;
-                let len = (s.len as usize) / bytes;
-                // TODO: alignment needs fixing?
+            unsafe fn unpack_into(s: &zend_string) -> &[Self] {
+                let len = s.len / ($d as usize / 8);
+                // The caller guarantees `val` is aligned for `Self`, see `PackSlice`.
                 #[allow(clippy::cast_ptr_alignment)]
                 let ptr = s.val.as_ptr().cast::<$t>();
+                // SAFETY: the caller guarantees alignment and that `len` whole elements
+                // fit in the string's `s.len` readable bytes.
                 unsafe { from_raw_parts(ptr, len) }
             }
         }