Fixed bug #73989 (PHP 7.1 Segfaults within Symfony test suite)

diff --git a/NEWS b/NEWS
index aed2a09e5d68334d17cb173814345073747fd2f4..f37c3b345f108bd9423f44e98e64a091c79cf7ed 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,8 @@ PHP                                                                        NEWS
 ?? ??? 2017 PHP 7.0.17
 
 - Core:
+  . Fixed bug #73989 (PHP 7.1 Segfaults within Symfony test suite).
+    (Dmitry, Laruence)
   . Fixed bug #74084 (Out of bound read - zend_mm_alloc_small). (Laruence)
   . Fixed bug #73807 (Performance problem with processing large post request).
     (Nikita)

diff --git a/Zend/tests/bug73989.phpt b/Zend/tests/bug73989.phpt
new file mode 100644 (file)
index 0000000..48e5a7a
--- /dev/null
+++ b/Zend/tests/bug73989.phpt
@@ -0,0 +1,28 @@
+--TEST--
+Bug #73989 (PHP 7.1 Segfaults within Symfony test suite)
+--FILE--
+<?php
+class Cycle
+{
+    private $thing;
+
+    public function __construct()
+    {
+               $obj = $this;
+        $this->thing = function() use($obj) {};
+    }
+
+    public function __destruct()
+    {
+               ($this->thing)();
+    }
+
+}
+
+for ($i = 0; $i < 10000; ++$i) {
+    $obj = new Cycle();
+}
+echo "OK\n";
+?>
+--EXPECT--
+OK

diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c
index dcfbcfb08636da92bc875959d027ebe387d9ebf5..0b9ce8ccc5b6b1ba094f85d7be6c33738c663f4e 100644 (file)
--- a/Zend/zend_gc.c
+++ b/Zend/zend_gc.c
@@ -25,26 +25,12 @@
 /* one (0) is reserved */
 #define GC_ROOT_BUFFER_MAX_ENTRIES 10001
 
-#define GC_FAKE_BUFFER_FLAG 0x80
-#define GC_TYPE_MASK        0x7f
-
 #define GC_HAS_DESTRUCTORS  (1<<0)
 
 #ifndef ZEND_GC_DEBUG
 # define ZEND_GC_DEBUG 0
 #endif
 
-#define GC_NUM_ADDITIONAL_ENTRIES \
-       ((4096 - ZEND_MM_OVERHEAD - sizeof(void*) * 2) / sizeof(gc_root_buffer))
-
-typedef struct _gc_additional_bufer gc_additional_buffer;
-
-struct _gc_additional_bufer {
-       uint32_t              used;
-       gc_additional_buffer *next;
-       gc_root_buffer        buf[GC_NUM_ADDITIONAL_ENTRIES];
-};
-
 #ifdef ZTS
 ZEND_API int gc_globals_id;
 #else
@@ -53,9 +39,6 @@ ZEND_API zend_gc_globals gc_globals;
 
 ZEND_API int (*gc_collect_cycles)(void);
 
-#define GC_REMOVE_FROM_ROOTS(current) \
-       gc_remove_from_roots((current))
-
 #if ZEND_GC_DEBUG > 1
 # define GC_TRACE(format, ...) fprintf(stderr, format "\n", ##__VA_ARGS__);
 # define GC_TRACE_REF(ref, format, ...) \
@@ -123,6 +106,12 @@ static zend_always_inline void gc_remove_from_roots(gc_root_buffer *root)
        GC_BENCH_DEC(root_buf_length);
 }
 
+static zend_always_inline void gc_remove_from_additional_roots(gc_root_buffer *root)
+{
+       root->next->prev = root->prev;
+       root->prev->next = root->next;
+}
+
 static void root_buffer_dtor(zend_gc_globals *gc_globals)
 {
        if (gc_globals->buf) {
@@ -149,6 +138,8 @@ static void gc_globals_ctor_ex(zend_gc_globals *gc_globals)
        gc_globals->gc_runs = 0;
        gc_globals->collected = 0;
 
+       gc_globals->additional_buffer = NULL;
+
 #if GC_BENCH
        gc_globals->root_buf_length = 0;
        gc_globals->root_buf_peak = 0;
@@ -204,6 +195,8 @@ ZEND_API void gc_reset(void)
                GC_G(first_unused) = NULL;
                GC_G(last_unused) = NULL;
        }
+
+       GC_G(additional_buffer) = NULL;
 }
 
 ZEND_API void gc_init(void)
@@ -276,21 +269,42 @@ ZEND_API void ZEND_FASTCALL gc_possible_root(zend_refcounted *ref)
        GC_BENCH_PEAK(root_buf_peak, root_buf_length);
 }
 
+static zend_always_inline gc_root_buffer* gc_find_additional_buffer(zend_refcounted *ref)
+{
+       gc_additional_buffer *additional_buffer = GC_G(additional_buffer);
+
+       /* We have to check each additional_buffer to find which one holds the ref */
+       while (additional_buffer) {
+               gc_root_buffer *root = additional_buffer->buf + (GC_ADDRESS(GC_INFO(ref)) - GC_ROOT_BUFFER_MAX_ENTRIES);
+               if (root->ref == ref) {
+                       return root;
+               }
+               additional_buffer = additional_buffer->next;
+       }
+
+       ZEND_ASSERT(0);
+       return NULL;
+}
+
 ZEND_API void ZEND_FASTCALL gc_remove_from_buffer(zend_refcounted *ref)
 {
        gc_root_buffer *root;
 
        ZEND_ASSERT(GC_ADDRESS(GC_INFO(ref)));
-       ZEND_ASSERT(GC_ADDRESS(GC_INFO(ref)) < GC_ROOT_BUFFER_MAX_ENTRIES);
 
        GC_BENCH_INC(zval_remove_from_buffer);
 
-       root = GC_G(buf) + GC_ADDRESS(GC_INFO(ref));
+       if (EXPECTED(GC_ADDRESS(GC_INFO(ref)) < GC_ROOT_BUFFER_MAX_ENTRIES)) {
+               root = GC_G(buf) + GC_ADDRESS(GC_INFO(ref));
+               gc_remove_from_roots(root);
+       } else {
+               root = gc_find_additional_buffer(ref);
+               gc_remove_from_additional_roots(root);
+       }
        if (GC_REF_GET_COLOR(ref) != GC_BLACK) {
                GC_TRACE_SET_COLOR(ref, GC_PURPLE);
        }
        GC_INFO(ref) = 0;
-       GC_REMOVE_FROM_ROOTS(root);
 
        /* updete next root that is going to be freed */
        if (GC_G(next_to_free) == root) {
@@ -641,7 +655,7 @@ static void gc_scan_roots(void)
        }
 }
 
-static void gc_add_garbage(zend_refcounted *ref, gc_additional_buffer **additional_buffer)
+static void gc_add_garbage(zend_refcounted *ref)
 {
        gc_root_buffer *buf = GC_G(unused);
 
@@ -664,25 +678,23 @@ static void gc_add_garbage(zend_refcounted *ref, gc_additional_buffer **addition
 #endif
        } else {
                /* If we don't have free slots in the buffer, allocate a new one and
-                * set it's address to GC_ROOT_BUFFER_MAX_ENTRIES that have special
+                * set it's address above GC_ROOT_BUFFER_MAX_ENTRIES that have special
                 * meaning.
                 */
-               if (!*additional_buffer || (*additional_buffer)->used == GC_NUM_ADDITIONAL_ENTRIES) {
+               if (!GC_G(additional_buffer) || GC_G(additional_buffer)->used == GC_NUM_ADDITIONAL_ENTRIES) {
                        gc_additional_buffer *new_buffer = emalloc(sizeof(gc_additional_buffer));
                        new_buffer->used = 0;
-                       new_buffer->next = *additional_buffer;
-                       *additional_buffer = new_buffer;
+                       new_buffer->next = GC_G(additional_buffer);
+                       GC_G(additional_buffer) = new_buffer;
                }
-               buf = (*additional_buffer)->buf + (*additional_buffer)->used;
-               (*additional_buffer)->used++;
+               buf = GC_G(additional_buffer)->buf + GC_G(additional_buffer)->used;
 #if 1
                /* optimization: color is already GC_BLACK (0) */
-               GC_INFO(ref) = GC_ROOT_BUFFER_MAX_ENTRIES;
+               GC_INFO(ref) = GC_ROOT_BUFFER_MAX_ENTRIES + GC_G(additional_buffer)->used;
 #else
-               GC_REF_SET_ADDRESS(ref, GC_ROOT_BUFFER_MAX_ENTRIES);
+               GC_REF_SET_ADDRESS(ref, GC_ROOT_BUFFER_MAX_ENTRIES) + GC_G(additional_buffer)->used;
 #endif
-               /* modify type to prevent indirect destruction */
-               GC_TYPE(ref) |= GC_FAKE_BUFFER_FLAG;
+               GC_G(additional_buffer)->used++;
        }
        if (buf) {
                GC_REFCOUNT(ref)++;
@@ -694,7 +706,7 @@ static void gc_add_garbage(zend_refcounted *ref, gc_additional_buffer **addition
        }
 }
 
-static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_additional_buffer **additional_buffer)
+static int gc_collect_white(zend_refcounted *ref, uint32_t *flags)
 {
        int count = 0;
        HashTable *ht;
@@ -727,7 +739,7 @@ tail_call:
 #else
                                if (!GC_ADDRESS(GC_INFO(ref))) {
 #endif
-                                       gc_add_garbage(ref, additional_buffer);
+                                       gc_add_garbage(ref);
                                }
                                if (obj->handlers->dtor_obj &&
                                    ((obj->handlers->dtor_obj != zend_objects_destroy_object) ||
@@ -751,7 +763,7 @@ tail_call:
                                        if (Z_REFCOUNTED_P(zv)) {
                                                ref = Z_COUNTED_P(zv);
                                                GC_REFCOUNT(ref)++;
-                                               count += gc_collect_white(ref, flags, additional_buffer);
+                                               count += gc_collect_white(ref, flags);
                                        /* count non-refcounted for compatibility ??? */
                                        } else if (Z_TYPE_P(zv) != IS_UNDEF) {
                                                count++;
@@ -773,7 +785,7 @@ tail_call:
 #else
                                if (!GC_ADDRESS(GC_INFO(ref))) {
 #endif
-                               gc_add_garbage(ref, additional_buffer);
+                               gc_add_garbage(ref);
                        }
                        ht = (zend_array*)ref;
                } else if (GC_TYPE(ref) == IS_REFERENCE) {
@@ -813,7 +825,7 @@ tail_call:
                        if (Z_REFCOUNTED_P(zv)) {
                                ref = Z_COUNTED_P(zv);
                                GC_REFCOUNT(ref)++;
-                               count += gc_collect_white(ref, flags, additional_buffer);
+                               count += gc_collect_white(ref, flags);
                                /* count non-refcounted for compatibility ??? */
                        } else if (Z_TYPE_P(zv) != IS_UNDEF) {
                                count++;
@@ -831,7 +843,7 @@ tail_call:
        return count;
 }
 
-static int gc_collect_roots(uint32_t *flags, gc_additional_buffer **additional_buffer)
+static int gc_collect_roots(uint32_t *flags)
 {
        int count = 0;
        gc_root_buffer *current = GC_G(roots).next;
@@ -840,8 +852,12 @@ static int gc_collect_roots(uint32_t *flags, gc_additional_buffer **additional_b
        while (current != &GC_G(roots)) {
                gc_root_buffer *next = current->next;
                if (GC_REF_GET_COLOR(current->ref) == GC_BLACK) {
+                       if (EXPECTED(GC_ADDRESS(GC_INFO(current->ref)) < GC_ROOT_BUFFER_MAX_ENTRIES)) {
+                               gc_remove_from_roots(current);
+                       } else {
+                               gc_remove_from_additional_roots(current);
+                       }
                        GC_INFO(current->ref) = 0; /* reset GC_ADDRESS() and keep GC_BLACK */
-                       GC_REMOVE_FROM_ROOTS(current);
                }
                current = next;
        }
@@ -850,7 +866,7 @@ static int gc_collect_roots(uint32_t *flags, gc_additional_buffer **additional_b
        while (current != &GC_G(roots)) {
                GC_REFCOUNT(current->ref)++;
                if (GC_REF_GET_COLOR(current->ref) == GC_WHITE) {
-                       count += gc_collect_white(current->ref, flags, additional_buffer);
+                       count += gc_collect_white(current->ref, flags);
                }
                current = current->next;
        }
@@ -886,13 +902,16 @@ static void gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buff
 tail_call:
        if (root ||
            (GC_ADDRESS(GC_INFO(ref)) != 0 &&
-            GC_REF_GET_COLOR(ref) == GC_BLACK &&
-            GC_ADDRESS(GC_INFO(ref)) != GC_ROOT_BUFFER_MAX_ENTRIES)) {
+            GC_REF_GET_COLOR(ref) == GC_BLACK)) {
                GC_TRACE_REF(ref, "removing from buffer");
                GC_REFCOUNT(ref)--;
                if (root) {
+                       if (EXPECTED(GC_ADDRESS(GC_INFO(root->ref)) < GC_ROOT_BUFFER_MAX_ENTRIES)) {
+                               gc_remove_from_roots(root);
+                       } else {
+                               gc_remove_from_additional_roots(root);
+                       }
                        GC_INFO(ref) = 0;
-                       GC_REMOVE_FROM_ROOTS(root);
                        root = NULL;
                } else {
                        GC_REMOVE_FROM_BUFFER(ref);
@@ -986,7 +1005,7 @@ ZEND_API int zend_gc_collect_cycles(void)
                zend_refcounted *p;
                gc_root_buffer to_free;
                uint32_t gc_flags = 0;
-               gc_additional_buffer *additional_buffer;
+               gc_additional_buffer *additional_buffer_snapshot;
 #if ZEND_GC_DEBUG
                zend_bool orig_gc_full;
 #endif
@@ -1010,8 +1029,8 @@ ZEND_API int zend_gc_collect_cycles(void)
 #endif
 
                GC_TRACE("Collecting roots");
-               additional_buffer = NULL;
-               count = gc_collect_roots(&gc_flags, &additional_buffer);
+               additional_buffer_snapshot = GC_G(additional_buffer);
+               count = gc_collect_roots(&gc_flags);
 #if ZEND_GC_DEBUG
                GC_G(gc_full) = orig_gc_full;
 #endif
@@ -1055,7 +1074,7 @@ ZEND_API int zend_gc_collect_cycles(void)
                                while (current != &to_free) {
                                        p = current->ref;
                                        GC_G(next_to_free) = current->next;
-                                       if ((GC_TYPE(p) & GC_TYPE_MASK) == IS_OBJECT) {
+                                       if (GC_TYPE(p) == IS_OBJECT) {
                                                zend_object *obj = (zend_object*)p;
 
                                                if (IS_OBJ_VALID(EG(objects_store).object_buckets[obj->handle]) &&
@@ -1092,7 +1111,7 @@ ZEND_API int zend_gc_collect_cycles(void)
                        p = current->ref;
                        GC_G(next_to_free) = current->next;
                        GC_TRACE_REF(p, "destroying");
-                       if ((GC_TYPE(p) & GC_TYPE_MASK) == IS_OBJECT) {
+                       if (GC_TYPE(p) == IS_OBJECT) {
                                zend_object *obj = (zend_object*)p;
 
                                if (EG(objects_store).object_buckets &&
@@ -1111,7 +1130,7 @@ ZEND_API int zend_gc_collect_cycles(void)
                                        EG(objects_store).free_list_head = obj->handle;
                                        p = current->ref = (zend_refcounted*)(((char*)obj) - obj->handlers->offset);
                                }
-                       } else if ((GC_TYPE(p) & GC_TYPE_MASK) == IS_ARRAY) {
+                       } else if (GC_TYPE(p) == IS_ARRAY) {
                                zend_array *arr = (zend_array*)p;
 
                                GC_TYPE(arr) = IS_NULL;
@@ -1133,10 +1152,10 @@ ZEND_API int zend_gc_collect_cycles(void)
                        current = next;
                }
 
-               while (additional_buffer != NULL) {
-                       gc_additional_buffer *next = additional_buffer->next;
-                       efree(additional_buffer);
-                       additional_buffer = next;
+               while (GC_G(additional_buffer) != additional_buffer_snapshot) {
+                       gc_additional_buffer *next = GC_G(additional_buffer)->next;
+                       efree(GC_G(additional_buffer));
+                       GC_G(additional_buffer) = next;
                }
 
                GC_TRACE("Collection finished");

diff --git a/Zend/zend_gc.h b/Zend/zend_gc.h
index 8e3bc46ff0c75ed799370fe8de44d6355341ac1d..db67104aa6f6421edbfa5979d0b0f737a5a1e6af 100644 (file)
--- a/Zend/zend_gc.h
+++ b/Zend/zend_gc.h
@@ -67,6 +67,17 @@ typedef struct _gc_root_buffer {
        uint32_t                 refcount;
 } gc_root_buffer;
 
+#define GC_NUM_ADDITIONAL_ENTRIES \
+       ((4096 - ZEND_MM_OVERHEAD - sizeof(void*) * 2) / sizeof(gc_root_buffer))
+
+typedef struct _gc_additional_bufer gc_additional_buffer;
+
+struct _gc_additional_bufer {
+       uint32_t              used;
+       gc_additional_buffer *next;
+       gc_root_buffer        buf[GC_NUM_ADDITIONAL_ENTRIES];
+};
+
 typedef struct _zend_gc_globals {
        zend_bool         gc_enabled;
        zend_bool         gc_active;
@@ -93,6 +104,8 @@ typedef struct _zend_gc_globals {
        uint32_t zval_marked_grey;
 #endif
 
+       gc_additional_buffer *additional_buffer;
+
 } zend_gc_globals;
 
 #ifdef ZTS