Segfault in ev_timer_stop

[kelunik]

I'm getting a segfault when running "bin/link-check https://amphp.org/" from https://github.com/kelunik/link-check with the "ev" extension enabled. It doesn't happen always, but sometimes. I don't have a reduced test case yet.

Program received signal SIGSEGV, Segmentation fault.
0x00007fffec714f51 in ev_timer_stop (loop=0x555556917a10, w=0x7fffed07b6e0)
    at /home/kelunik/.pecl-ev-build/source/php7/../libev/ev.c:3915
3915            timers [active] = timers [timercnt + HEAP0];
(gdb) bt
#0  0x00007fffec714f51 in ev_timer_stop (loop=0x555556917a10, w=0x7fffed07b6e0)
    at /home/kelunik/.pecl-ev-build/source/php7/../libev/ev.c:3915
#1  0x0000555555ba69fc in ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER ()
    at /home/kelunik/.php-build/release/Zend/zend_vm_execute.h:907
#2  execute_ex (ex=0x555556917a10) at /home/kelunik/.php-build/release/Zend/zend_vm_execute.h:59752
#3  0x0000555555ae652c in zend_call_function (fci=0x7fffed01d290, fci@entry=0x7fffffffb8a0, 
    fci_cache=<optimized out>, fci_cache@entry=0x7fffffffb870)
    at /home/kelunik/.php-build/release/Zend/zend_execute_API.c:817
#4  0x0000555555b1549f in zend_call_method (object=object@entry=0x7fffffffb980, obj_ce=0x7fffed103400, 
    fn_proxy=fn_proxy@entry=0x7fffffffb978, 
    function_name=function_name@entry=0x555556264d52 "__destruct", 
    function_name_len=function_name_len@entry=10, retval_ptr=retval_ptr@entry=0x0, param_count=0, 
    arg1=0x0, arg2=0x0) at /home/kelunik/.php-build/release/Zend/zend_interfaces.c:100
#5  0x0000555555b306e2 in zend_objects_destroy_object (object=<optimized out>)
    at /home/kelunik/.php-build/release/Zend/zend_objects.c:146
#6  0x0000555555b35525 in zend_objects_store_call_destructors (
    objects=objects@entry=0x55555667aa18 <executor_globals+824>)
    at /home/kelunik/.php-build/release/Zend/zend_objects_API.c:58
#7  0x0000555555ae4ceb in shutdown_destructors ()
    at /home/kelunik/.php-build/release/Zend/zend_execute_API.c:239
#8  0x0000555555af6297 in zend_call_destructors () at /home/kelunik/.php-build/release/Zend/zend.c:1019
#9  0x0000555555a90dd5 in php_request_shutdown (dummy=<optimized out>)
    at /home/kelunik/.php-build/release/main/main.c:1826
#10 0x0000555555ba8513 in do_cli (argc=3, argv=0x5555566af210)
    at /home/kelunik/.php-build/release/sapi/cli/php_cli.c:1178
#11 0x00005555556cd002 in main (argc=3, argv=0x5555566af210)
    at /home/kelunik/.php-build/release/sapi/cli/php_cli.c:1404

[Ruslan Osmanov]

Does it work with the following patch for vendor/amphp/amp/lib/Loop/EvDriver.php?

--- EvDriver.php    2017-10-04 21:28:22.774063311 +0700
+++ /tmp/EvDriver.php   2017-10-04 21:28:18.353002410 +0700
@@ -122,8 +122,11 @@
     }

     public function __destruct() {
-        foreach ($this->events as $event) {
-            $event->stop();
+        foreach ($this->events as &$event) {
+            if ($event->is_active) {
+                $event->stop();
+                $event = null;
+            }
         }
     }

@@ -133,8 +136,11 @@
     public function run() {
         $active = self::$activeSignals;

-        foreach ($active as $event) {
-            $event->stop();
+        foreach ($active as &$event) {
+            if ($event->is_active) {
+                $event->stop();
+                $event = null;
+            }
         }

         self::$activeSignals = &$this->signals;
@@ -147,7 +153,9 @@
             parent::run();
         } finally {
             foreach ($this->signals as $event) {
-                $event->stop();
+                if ($event->is_active) {
+                    $event->stop();
+                }
             }

             self::$activeSignals = &$active;
@@ -230,7 +238,11 @@
      */
     protected function deactivate(Watcher $watcher) {
         if (isset($this->events[$id = $watcher->id])) {
-            $this->events[$id]->stop();
+            //$this->events[$id]->stop();
+            if ($this->events[$id]->is_active) {
+                $this->events[$id]->stop();
+                $this->events[$id] = null;
+            }
             if ($watcher->type === Watcher::SIGNAL) {
                 unset($this->signals[$id]);
             }

[Ruslan Osmanov]

I do not know exactly what is causing heap corruption within the libev structures, but it is somehow concerned with the watcher destruction phase. In order to avoid this issue, follow recommendations below.

1. If you do not need an Event object, destroy it explicitly by assigning the PHP variable to null
2. Do not modify a watcher as long as it is active (has not been stopped)

Note, you do not need to stop a watcher before destructing it (by assigning to null, for instance), as it will be stopped automatically by internal object handlers.

[kelunik]

Sorry for the long delay. I finally tested your suggested changes. They don't work, because they leave null values in the array and then result in property accesses on null-objects, but I modified it and have a way simpler fix now.

https://github.com/amphp/amp/pull/192/files#diff-ba85a498c8b886f76de4156bc55997bc

Thanks!