fix(handler): enforce immutability when OPcache is enabled, and fail loudly when it cannot be

.github/workflows/ci.yml

@@ -48,11 +48,13 @@ jobs:
 
       # The JIT rewrites the executor internals z-engine hooks into, so it
       # stays off; FFI must be enabled for CLI scripts, not only preloading.
+      # OPcache itself has to be loaded: every .phpt pins opcache.enable_cli
+      # explicitly and one of them requires it to be switched on.
       - name: Install PHP
         uses: shivammathur/setup-php@v2
         with:
           php-version: ${{ matrix.php-version }}
-          extensions: ffi
+          extensions: ffi, opcache
           ini-values: memory_limit=-1, ffi.enable=1, opcache.jit=off
           coverage: none
           tools: composer:v2

README.md

@@ -49,6 +49,26 @@ ImmutableHandler::install();
 Probably, `Z-Engine` will provide an automatic self-registration later, but for now it's ok to perform initialization
 manually.
 
+### Installation is verified, not assumed
+
+`ImmutableHandler::install()` does not just install the engine handlers, it proves on a private throwaway class that a
+property write is really intercepted afterwards. If it is not, `install()` throws a `RuntimeException` naming the cause
+and the workaround instead of returning quietly. An environment where immutability is not enforced now fails loudly at
+start-up rather than letting every write outside a constructor succeed in silence.
+
+Two things can make the handlers unreachable:
+
+* **Classes linked before `install()` ran.** The library hooks classes as they start to implement `ImmutableInterface`,
+  so a class that was already linked — most notably anything pulled in through OPcache preloading (`opcache.preload`) —
+  is never seen. Call `install()` as early as possible, before any immutable class is loaded.
+* **OPcache** (`opcache.enable` / `opcache.enable_cli`). With OPcache active, the engine hands the
+  "interface gets implemented" callback a different `zend_class_entry` than the one it later uses at runtime, so the
+  property handlers used to land on a structure no object ever reads —
+  [lisachenko/z-engine#238](https://github.com/lisachenko/z-engine/issues/238). This library works around it by
+  installing the property handlers from its own `create_object` handler, which does receive the runtime class entry, so
+  immutability is enforced with OPcache enabled as well. Should a future engine defeat that too, the self-check above
+  turns it into a start-up error; disabling OPcache for the process remains the fallback.
+
 Applying immutability
 --------
 In order to make your object immutable, you just need to implement the `ImmutableInterface` interface marker in your

src/EnforcementProbe.php

@@ -0,0 +1,27 @@
+<?php
+/**
+ * Immutable object library
+ *
+ * @copyright Copyright 2020 Lisachenko Alexander <lisachenko.it@gmail.com>
+ *
+ * This source file is subject to the license that is bundled
+ * with this source code in the file LICENSE.
+ */
+declare(strict_types=1);
+
+namespace Immutable;
+
+/**
+ * Throwaway class used by ImmutableHandler::install() to verify that the property handlers
+ * it installs are really reached by the engine.
+ *
+ * It must not be referenced anywhere else: the self-check is only meaningful as long as this
+ * class is linked *after* the "interface gets implemented" handler has been installed, which
+ * is exactly the condition every user class has to satisfy as well.
+ *
+ * @internal
+ */
+final class EnforcementProbe implements ImmutableInterface
+{
+    public int $probe = 0;
+}

src/ImmutableHandler.php

@@ -13,11 +13,13 @@
 
 use Closure;
 use ReflectionMethod;
+use RuntimeException;
+use ZEngine\ClassExtension\Hook\CreateObjectHook;
 use ZEngine\ClassExtension\Hook\GetPropertyPointerHook;
 use ZEngine\ClassExtension\Hook\InterfaceGetsImplementedHook;
 use ZEngine\ClassExtension\Hook\UnsetPropertyHook;
 use ZEngine\ClassExtension\Hook\WritePropertyHook;
-use ZEngine\ClassExtension\ObjectCreateTrait;
+use ZEngine\ClassExtension\ObjectCreateInterface;
 use ZEngine\ClassExtension\ObjectGetPropertyPointerInterface;
 use ZEngine\ClassExtension\ObjectUnsetPropertyInterface;
 use ZEngine\ClassExtension\ObjectWritePropertyInterface;
@@ -28,38 +30,80 @@
  * This ImmutableHandler controls the behaviour of
  */
 final class ImmutableHandler implements
+    ObjectCreateInterface,
     ObjectWritePropertyInterface,
     ObjectGetPropertyPointerInterface,
     ObjectUnsetPropertyInterface
 {
+    /**
+     * Addresses of the zend_class_entry structures that already carry the property handlers
+     *
+     * The object handlers table z-engine installs into is keyed by the address of the class
+     * entry, so the bookkeeping here has to use the very same key.
+     *
+     * @var array<int, true>
+     */
+    private static array $preparedClassEntries = [];
+
+    /**
+     * Set while install() runs its enforcement self-check
+     */
+    private static bool $isProbingEnforcement = false;
+
+    /**
+     * Set by __fieldWrite() when the self-check write actually reached the handler
+     */
+    private static bool $wasProbeWriteIntercepted = false;
+
     public static function install(): void
     {
         $handler   = Closure::fromCallable([self::class, '__interfaceImplemented']);
         $interface = new ReflectionClass(ImmutableInterface::class);
         $interface->setInterfaceGetsImplementedHandler($handler);
+
+        self::verifyEnforcementIsActive();
     }
 
     public static function __interfaceImplemented(InterfaceGetsImplementedHook $hook): int
     {
-        $objectCreateHandler       = (new ReflectionMethod(ObjectCreateTrait::class, '__init'))->getClosure();
-        $objectFieldWriteHandler   = (new ReflectionMethod(self::class, '__fieldWrite'))->getClosure();
-        $objectFieldPointerHandler = (new ReflectionMethod(self::class, '__fieldPointer'))->getClosure();
-        $objectFieldUnsetHandler   = (new ReflectionMethod(self::class, '__fieldUnset'))->getClosure();
+        $objectCreateHandler = Closure::fromCallable([self::class, '__init']);
 
-        $implementor = $hook->getClass();
-        $implementor->setCreateObjectHandler($objectCreateHandler);
-        $implementor->setWritePropertyHandler($objectFieldWriteHandler);
-        $implementor->setGetPropertyPointerHandler($objectFieldPointerHandler);
-        $implementor->setUnsetPropertyHandler($objectFieldUnsetHandler);
+        // Only the create_object handler is installed here, because it lives in the class entry
+        // itself and therefore survives the OPcache round-trip through shared memory. The
+        // property handlers live in a separate object handlers table that z-engine keys by the
+        // address of the class entry, and with OPcache enabled the class entry seen here is not
+        // the one the engine uses at runtime - so they are installed lazily from __init(), which
+        // does receive the runtime class entry. See lisachenko/z-engine#238.
+        $hook->getClass()->setCreateObjectHandler($objectCreateHandler);
 
         return Core::SUCCESS;
     }
 
+    /**
+     * Performs low-level initialization of object during new instances creation
+     *
+     * @inheritDoc
+     */
+    public static function __init(CreateObjectHook $hook): object
+    {
+        // Must happen before proceed(): the object being created picks up the object handlers
+        // table for its class entry, and that table is what the property handlers are written to.
+        self::prepareClassEntry($hook->getClassType());
+
+        return $hook->proceed();
+    }
+
     /**
      * @inheritDoc
      */
     public static function __fieldWrite(WritePropertyHook $hook)
     {
+        if (self::$isProbingEnforcement) {
+            self::$wasProbeWriteIntercepted = true;
+
+            return $hook->getValue();
+        }
+
         $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS|DEBUG_BACKTRACE_PROVIDE_OBJECT, 3);
         $frame = $trace[2] ?? [];
         if (!isset($frame['class'])) {
@@ -92,4 +136,69 @@ public static function __fieldUnset(UnsetPropertyHook $hook): void
     {
         throw new \LogicException("Unset of immutable field is restricted");
     }
+
+    /**
+     * Installs the property handlers for the runtime class entry of an implementor
+     *
+     * @param object $classEntry Raw zend_class_entry pointer received by the create_object handler
+     */
+    private static function prepareClassEntry(object $classEntry): void
+    {
+        $classEntryAddress = Core::addressOf($classEntry);
+        if (isset(self::$preparedClassEntries[$classEntryAddress])) {
+            return;
+        }
+        // Marked as prepared up-front, so that any object creation happening while the handlers
+        // are being installed can not re-enter this method for the same class entry.
+        self::$preparedClassEntries[$classEntryAddress] = true;
+
+        $objectFieldWriteHandler   = Closure::fromCallable([self::class, '__fieldWrite']);
+        $objectFieldPointerHandler = Closure::fromCallable([self::class, '__fieldPointer']);
+        $objectFieldUnsetHandler   = Closure::fromCallable([self::class, '__fieldUnset']);
+
+        // Deliberately not guarded by a try/catch: swallowing a failure here would restore the
+        // silent no-op this whole mechanism exists to prevent. install() has already proven on a
+        // throwaway class that this code path works in the current environment.
+        $implementor = ReflectionClass::fromCData($classEntry);
+        $implementor->setWritePropertyHandler($objectFieldWriteHandler);
+        $implementor->setGetPropertyPointerHandler($objectFieldPointerHandler);
+        $implementor->setUnsetPropertyHandler($objectFieldUnsetHandler);
+    }
+
+    /**
+     * Proves on a throwaway class that a property write is really intercepted
+     *
+     * This is an empirical check on purpose: an environment can defeat the handlers in more ways
+     * than an ini setting can describe, and the only answer that matters is whether a write to an
+     * object of a class that implements ImmutableInterface reaches the handler or not.
+     *
+     * The probe records a flag instead of throwing, because the handler runs inside an FFI
+     * callback and an exception crossing that boundary is an uncatchable fatal error.
+     */
+    private static function verifyEnforcementIsActive(): void
+    {
+        self::$wasProbeWriteIntercepted = false;
+        self::$isProbingEnforcement     = true;
+        try {
+            // The probe class is linked here, i.e. after the handler above has been installed
+            $probe        = new EnforcementProbe();
+            $probe->probe = 1;
+        } finally {
+            self::$isProbingEnforcement = false;
+        }
+
+        if (self::$wasProbeWriteIntercepted) {
+            return;
+        }
+
+        throw new RuntimeException(
+            'Immutability can not be enforced in this environment: a write to a property of a class implementing '
+            . ImmutableInterface::class . ' is not intercepted, so writes outside of a constructor would silently '
+            . 'succeed instead of raising an error. Check that ImmutableHandler::install() runs before any class '
+            . 'implementing ' . ImmutableInterface::class . ' is linked - OPcache preloading links classes ahead of '
+            . 'it and those are never seen by the handler. If that is already the case, the engine hooks are not '
+            . 'taking effect at all (see https://github.com/lisachenko/z-engine/issues/238): disable OPcache for this '
+            . 'process with opcache.enable=0, or opcache.enable_cli=0 on CLI.',
+        );
+    }
 }

tests/Functional/testBoundClosureSetPropertyThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Updating property of immutable object via Bound closure throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testCanCloneImmutableObject.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Writing property of immutable object throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testChangePrivatePropertyWithSetterThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Updating property of immutable object with setter throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testChildSetProtectedPropertyThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Updating protected property of immutable object in child class throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testDirectPublicPropertyIssetReturnsTrue.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Writing property of immutable object throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testDirectPublicPropertySetThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Writing property of immutable object throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testDirectPublicPropertySetThrowsAnExceptionWithOpcacheEnabled.phpt

@@ -0,0 +1,26 @@
+--TEST--
+Writing property of immutable object throws an error when OPcache is enabled
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=1
+--SKIPIF--
+<?php
+if (!extension_loaded('Zend OPcache')) {
+    echo 'skip Zend OPcache is not available in this build';
+}
+--FILE--
+<?php
+declare(strict_types=1);
+
+use Immutable\Stub\TestObject;
+
+include __DIR__ . './../bootstrap.php';
+
+echo 'OPCACHE ', (ini_get('opcache.enable_cli') ? 'ON' : 'OFF'), PHP_EOL;
+
+$object = new TestObject(['publicProperty' => 200]);
+$object->publicProperty = 300;
+?>
+--EXPECTREGEX--
+OPCACHE ON[\s\S]*Immutable object could be modified only in constructor or static methods

tests/Functional/testInstallThrowsWhenEnforcementIsNotActive.phpt

@@ -0,0 +1,38 @@
+--TEST--
+Installation reports loudly when the installed handlers would not be reached
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
+--FILE--
+<?php
+declare(strict_types=1);
+
+use Immutable\EnforcementProbe;
+use Immutable\ImmutableHandler;
+use ZEngine\Core;
+
+ini_set('display_errors', 'on');
+
+include __DIR__ . './../../vendor/autoload.php';
+
+Core::init();
+
+// Link a class implementing ImmutableInterface *before* install() runs: the "interface gets
+// implemented" handler never sees it, so nothing can be hooked on it afterwards. This is what
+// OPcache preloading does to every preloaded class, and what OPcache used to do to every class.
+class_exists(EnforcementProbe::class);
+
+try {
+    ImmutableHandler::install();
+    echo 'NO EXCEPTION', PHP_EOL;
+} catch (RuntimeException $exception) {
+    echo 'RuntimeException', PHP_EOL;
+    echo str_contains($exception->getMessage(), 'Immutability can not be enforced') ? 'REASON OK' : 'REASON NO', PHP_EOL;
+    echo str_contains($exception->getMessage(), 'opcache.enable_cli=0') ? 'WORKAROUND OK' : 'WORKAROUND NO', PHP_EOL;
+}
+?>
+--EXPECT--
+RuntimeException
+REASON OK
+WORKAROUND OK

tests/Functional/testPropertyUnsetThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Writing property of immutable object throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);

tests/Functional/testReflectionPropertySetThrowsAnException.phpt

@@ -1,5 +1,9 @@
 --TEST--
 Updating property of immutable object via Reflection throws an error
+--INI--
+ffi.enable=1
+opcache.jit=off
+opcache.enable_cli=0
 --FILE--
 <?php
 declare(strict_types=1);