Advance class and function table pointers every compilation invocation

This is a small refactor to how the AT compilation step works. Previously, each invocation of compile_file() would move the global class and function table pointers to the end of Zend's respective tables. This caused an insidious bug where blacklisted files would occasionally insert whitelisted classes into Zend's class table, causing havoc when later compiling subclasses of that class. Now, we iterate over all new entries in Zend's class and function tables upon each invocation of compile_file(). This causes negligible performance overhead when there are only a few classes and functions per PHP file, which is the typical case.
box · Jul 14, 2014 · af42f95 · af42f95
1 parent a9f0c00
commit af42f95
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 85 deletions.
diff --git a/augmented_annotation_compiler.cc b/augmented_annotation_compiler.cc
@@ -12,7 +12,7 @@
 int compile_user_function(zend_op_array *func)
 {
 	// first, ensure that this function hasn't already been compiled
-	// if it has, ensure that the literals array is the right thing and then exit
+	// if it has, ensure everything is correctly assigned
 	zend_literal *enforcer = get_enforcement_literal(func);
 	if (enforcer) {
 		if (enforcer != func->literals) {
@@ -152,26 +152,32 @@ int compile_new_global_user_functions(TSRMLS_D)
 	for (; zend_hash_get_current_data_ex(function_table, (void**) &func, &temp_pos) == SUCCESS;
 	        zend_hash_move_forward_ex(function_table, &temp_pos)) {
 
-		// if this function has a doc comment, compile it!
-		if (func->type == ZEND_USER_FUNCTION) {
-			DPRINTF("ATTEMPTING TO COMPILE name = %s\n", func->common.function_name);
-
-			// if this function lives in a namespace, be sure to set the namespace prefix so we
-			// can use it later when compiling classname annotations
-			int namespace_prefix_len =
-				get_namespace_prefix_length(func->common.function_name, strlen(func->common.function_name));
-			if ( namespace_prefix_len ) {
-				ATCG(current_namespaced_entity) = func->common.function_name;
-				ATCG(current_namespace_prefix_len) = namespace_prefix_len;
-			}
+		// always advance the global function table pointer
+		ATCG(func_pos) = temp_pos;
 
-			if (compile_user_function(&(func->op_array))) {
-				num_funcs_added++;
-			}
+		// only compile valid functions in whitelisted files
+		if (func->type != ZEND_USER_FUNCTION ||
+				!should_compile_file(func->op_array.filename TSRMLS_CC)) {
+			continue;
+		}
+
+		DPRINTF("ATTEMPTING TO COMPILE name = %s\n", func->common.function_name);
 
-			ATCG(current_namespaced_entity) = NULL;
-			ATCG(current_namespace_prefix_len) = 0;
+		// if this function lives in a namespace, be sure to set the namespace prefix so we
+		// can use it later when compiling classname annotations
+		int namespace_prefix_len =
+			get_namespace_prefix_length(func->common.function_name, strlen(func->common.function_name));
+		if ( namespace_prefix_len ) {
+			ATCG(current_namespaced_entity) = func->common.function_name;
+			ATCG(current_namespace_prefix_len) = namespace_prefix_len;
 		}
+
+		if (compile_user_function(&(func->op_array))) {
+			num_funcs_added++;
+		}
+
+		ATCG(current_namespaced_entity) = NULL;
+		ATCG(current_namespace_prefix_len) = 0;
 	}
 	return num_funcs_added;
 }
@@ -190,8 +196,8 @@ int add_class_instance_methods(zend_class_entry *ce TSRMLS_DC)
 	HashTable *func_table = &(ce->function_table);
 
 	for (zend_hash_internal_pointer_reset_ex(func_table, &method_pos);
-	        zend_hash_get_current_data_ex(func_table, (void **) &func, &method_pos) == SUCCESS;
-	        zend_hash_move_forward_ex(func_table, &method_pos)) {
+			zend_hash_get_current_data_ex(func_table, (void **) &func, &method_pos) == SUCCESS;
+			zend_hash_move_forward_ex(func_table, &method_pos)) {
 
 		// skip functions that aren't user functions
 		if (func->type != ZEND_USER_FUNCTION) {
@@ -201,9 +207,9 @@ int add_class_instance_methods(zend_class_entry *ce TSRMLS_DC)
 		DPRINTF("inspecting instance method %s at %p\n", func->common.function_name, func);
 		DPRINTF("instance method filename = %s\n", func->op_array.filename);
 
-		// we must once again ensure that we should compile this method using the blacklist + whitelist
-		// NOTE: this normally would not be necessary, but zend makes some weird optimizations
-		// when compiling an empty class that extends another class
+		// we must once again ensure that we should compile this method using the
+		// blacklist + whitelist NOTE: this normally would not be necessary, but
+		// zend re-uses op_arrays for inherited instance methods
 		// PS: To save CPU cycles, only check the whitelist + blacklist if this
 		// function resides in a different file than the class
 		if (strncmp(class_file, func->op_array.filename, class_file_strlen) == 0 ||
@@ -243,27 +249,34 @@ int compile_new_instance_method_annotations(TSRMLS_D)
 	for (; zend_hash_get_current_data_ex(class_table, (void **) &pce, &temp_pos) == SUCCESS;
 	        zend_hash_move_forward_ex(class_table, &temp_pos)) {
 
-		if ((*pce)->type == ZEND_USER_CLASS) {
-			DPRINTF("\t\t\tADDING new type enforcement info for class %s\n", (*pce)->name);
+		// always advance our global class table position
+		ATCG(class_pos) = temp_pos;
 
-			int namespace_prefix_len = get_namespace_prefix_length((*pce)->name, (int) (*pce)->name_length);
-			if (namespace_prefix_len) {
-				ATCG(current_namespaced_entity) = (*pce)->name;
-				ATCG(current_namespace_prefix_len) = namespace_prefix_len;
-			}
+		// only compile valid classes declared in whitelisted files
+		if ((*pce)->type != ZEND_USER_CLASS ||
+				!should_compile_file((*pce)->info.user.filename TSRMLS_CC)) {
+			continue;
+		}
 
-			// now, we must iterate through its methods!
-			num_instance_methods_added = add_class_instance_methods(*pce TSRMLS_CC);
-			if (num_instance_methods_added == -1) {
-				DPRINTF("ERROR ADDING ANNOTATIONS FOR CLASS %s\n", (*pce)->name);
-			} else {
-				num_methods_added += num_instance_methods_added;
-			}
+		DPRINTF("\t\t\tADDING new type enforcement info for class %s\n", (*pce)->name);
 
-			// always reset the current namespaced entity..
-			ATCG(current_namespaced_entity) = NULL;
-			ATCG(current_namespace_prefix_len) = 0;
+		int namespace_prefix_len = get_namespace_prefix_length((*pce)->name, (int) (*pce)->name_length);
+		if (namespace_prefix_len) {
+			ATCG(current_namespaced_entity) = (*pce)->name;
+			ATCG(current_namespace_prefix_len) = namespace_prefix_len;
 		}
+
+		// now, we must iterate through its methods!
+		num_instance_methods_added = add_class_instance_methods(*pce TSRMLS_CC);
+		if (num_instance_methods_added == -1) {
+			DPRINTF("ERROR ADDING ANNOTATIONS FOR CLASS %s\n", (*pce)->name);
+		} else {
+			num_methods_added += num_instance_methods_added;
+		}
+
+		// always reset the current namespaced entity..
+		ATCG(current_namespaced_entity) = NULL;
+		ATCG(current_namespace_prefix_len) = 0;
 	}
 
 	return num_methods_added;
@@ -277,16 +290,5 @@ int compile_new_instance_method_annotations(TSRMLS_D)
 int compile_new_user_function_annotations(TSRMLS_D)
 {
 	return compile_new_instance_method_annotations(TSRMLS_C) +
-	       compile_new_global_user_functions(TSRMLS_C);
-}
-
-/**
- * Init the class and function table last_position pointers to be at the end
- * of their respective hash tables
- * ASSUMPTION: zend_compile appends new classes/functions
- */
-void prepare_compiler_state(TSRMLS_D)
-{
-	zend_hash_internal_pointer_end_ex(CG(class_table), &ATCG(class_pos));
-	zend_hash_internal_pointer_end_ex(CG(function_table), &ATCG(func_pos));
+		compile_new_global_user_functions(TSRMLS_C);
 }
diff --git a/augmented_annotation_compiler.h b/augmented_annotation_compiler.h
@@ -3,12 +3,7 @@
 
 #include "zend_compile.h"
 
-#define ENFORCE_OPCODE 159
-
-void prepare_compiler_state(TSRMLS_D);
 int compile_new_user_function_annotations(TSRMLS_D);
-int compile_user_function(zend_op_array *func);
-bool should_compile_file(const char* path TSRMLS_DC);
 zend_literal *get_enforcement_literal(zend_op_array *op_array);
 
 #endif /* PHP_AUGMENTED_ANNOTATION_COMPILER_H */
diff --git a/augmented_types.cc b/augmented_types.cc
@@ -86,26 +86,20 @@ static int augmented_types_start(zend_extension *extension)
 
 zend_op_array *augmented_types_compile_file(zend_file_handle *file_handle, int type TSRMLS_DC)
 {
-	// must be called before old_compile_file to ensure we can accurately judge
-	// what functions and classes old_compile_file adds
-	prepare_compiler_state(TSRMLS_C);
-
 	zend_op_array *op_array = old_compile_file(file_handle, type TSRMLS_CC);
 
 	DPRINTF("FROM COMPILE::::\n");
 	DPRINTF("\t\t\tFor filename = %s\n", file_handle->filename);
 
-	if (should_compile_file(file_handle->filename TSRMLS_CC)) {
-		int num_annotations_compiled = compile_new_user_function_annotations(TSRMLS_C);
-
-		if (AT_DEBUG) {
-			if (num_annotations_compiled > 0) {
-				DPRINTF("success compiling annotations! %d new annotations compiled\n\n", num_annotations_compiled);
-			} else if (num_annotations_compiled == 0) {
-				DPRINTF("no new annotations compiled\n\n");
-			} else {
-				DPRINTF("failure compiling annotations\n\n");
-			}
+	int num_annotations_compiled = compile_new_user_function_annotations(TSRMLS_C);
+
+	if (AT_DEBUG) {
+		if (num_annotations_compiled > 0) {
+			DPRINTF("success compiling annotations! %d new annotations compiled\n\n", num_annotations_compiled);
+		} else if (num_annotations_compiled == 0) {
+			DPRINTF("no new annotations compiled\n\n");
+		} else {
+			DPRINTF("failure compiling annotations\n\n");
 		}
 	}
 
@@ -114,14 +108,8 @@ zend_op_array *augmented_types_compile_file(zend_file_handle *file_handle, int t
 
 zend_op_array* augmented_types_compile_string(zval* source_string, char *filename TSRMLS_DC)
 {
-	prepare_compiler_state(TSRMLS_C);
 	zend_op_array *res = old_compile_string(source_string, filename TSRMLS_CC);
-
-	// check if we should compile new functions produced by this eval-ed string
-	if (should_compile_file(filename TSRMLS_CC)) {
-		compile_new_user_function_annotations(TSRMLS_C);
-	}
-
+	compile_new_user_function_annotations(TSRMLS_C);
 	return res;
 }
 

diff --git a/augmented_util.cc b/augmented_util.cc
@@ -43,9 +43,6 @@ void print_op_code(zend_uchar opcode)
 	case ZEND_ECHO:
 		DPRINTF("ZEND_ECHO\n");
 		break;
-	case ENFORCE_OPCODE:
-		DPRINTF("ENFORCE_OPCODE\n");
-		break;
 	default:
 		DPRINTF("opcode = %u\n", opcode);
 	}

diff --git a/tests/class_hierarchy_enforcement_works.phpt b/tests/class_hierarchy_enforcement_works.phpt
@@ -30,7 +30,14 @@ class Bar extends Foo
 }
 
 $o = new Bar();
-echo $o->inc_int(2);
+var_dump( $o->inc_int(2) );
+$o->barbar();
+$o->inc_int("foo");
+
 ?>
---EXPECT--
-44
+--EXPECTREGEX--
+int\(44\)
+barbarbar
+
+Fatal error:.*
+.*
diff --git a/tests/cross_file_inheritance_works.phpt b/tests/cross_file_inheritance_works.phpt
@@ -0,0 +1,19 @@
+--TEST--
+Test to ensure that inheritance in classes across different files works
+--FILE--
+<?php
+echo augmented_types_whitelist(["tests/helper/superclass.php"])."\n";
+echo augmented_types_blacklist(["tests/helper/subclass.php"])."\n";
+require_once "tests/helper/subclass.php";
+
+$a = new FooSub();
+var_dump( $a->childMethod("foo") );
+$a->parentMethod("foo");
+?>
+--EXPECTREGEX--
+1
+1
+int\(42\)
+
+Fatal error.*
+.*
diff --git a/tests/helper/subclass.php b/tests/helper/subclass.php
@@ -0,0 +1,15 @@
+<?php
+
+require_once "tests/helper/superclass.php";
+class FooSub extends Foo {
+	/**
+	 * @param int
+	 * @return int
+	 */
+	public function childMethod($a) {
+		if (is_int($a)) {
+			return $a + 42;
+		}
+		return 42;
+	}
+}
diff --git a/tests/helper/superclass.php b/tests/helper/superclass.php
@@ -0,0 +1,11 @@
+<?php
+
+class Foo {
+	/**
+	 * @param int
+	 * @return int
+	 */
+	public function parentMethod($a) {
+		return $a + 42;
+	}
+}
diff --git a/tests/variadic_functions_work.phpt b/tests/variadic_functions_work.phpt
@@ -23,11 +23,29 @@ function multiply_all($start)
 echo implode(multiply_all(2.0, 1.1, 2.2, 3.3), " ") . "\n";
 echo implode(multiply_all(1.0)) . "\n";
 
+/**
+* @param *string|int|null
+* @return void
+*/
+function foo()
+{
+	$args = func_get_args();
+	foreach ($args as $arg) {
+		var_dump($arg);
+	}
+}
+
+foo("hello", 42, "bye", null);
+
 echo multiply_all(2.0, 5);
 ?>
 --EXPECTREGEX--
 2.2 4.4 6.6
 
+string\(5\) "hello"
+int\(42\)
+string\(3\) "bye"
+NULL
 
 Fatal error: Wrong type encountered for argument 2 of function multiply_all, was expecting a \*\(float\) but got a \(int\) 5
 .*