com php-src: Fixed bug #73989 (PHP 7.1 Segfaults within Symfony test suite): NEWS Zend/tests/bug73989.phpt Zend/zend_gc.c Zend /zend_gc.h

[email protected] (Xinchen Hui)
Newsgroups php.cvs
Message-ID <[email protected]>
Commit:    391735053181f3d166e4ebb58cf04a8acf3d1724
Author:    Xinchen Hui <[email protected]>         Mon, 13 Feb 2017 19:16:17 +0800
Parents:   eb1373e509a69a48e90e6dd4c2dda1348e19b776
Branches:  PHP-7.0 PHP-7.1 master

Link:       http://git.php.net/?p=php-src.git;a=commitdiff;h=391735053181f3d166e4ebb58cf04a8acf3d1724

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

Bugs:
https://bugs.php.net/73989

Changed paths:
  M  NEWS
  A  Zend/tests/bug73989.phpt
  M  Zend/zend_gc.c
  M  Zend/zend_gc.h
diff_391735053181f3d166e4ebb58cf04a8acf3d1724.txt (text/plain, 12.3 KB)
diff --git a/NEWS b/NEWS
index aed2a09..f37c3b3 100644
--- 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
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 dcfbcfb..0b9ce8c 100644
--- 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 8e3bc46..db67104 100644
--- 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.