First round of reviews

Author: Girgias

ext/pdo/pdo_stmt.c

@@ -656,7 +656,7 @@ static bool pdo_call_fetch_object_constructor(zend_function *constructor, HashTa
 	};
 
 	zend_call_function(&fci, &fcc);
-	bool failed = Z_TYPE(retval_constructor_call) == IS_UNDEF;
+	bool failed = Z_ISUNDEF(retval_constructor_call);;
 	zval_ptr_dtor(&retval_constructor_call);
 
 	return failed;
@@ -755,11 +755,11 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
 			if (flags & PDO_FETCH_CLASSTYPE) {
 				zval ce_name_from_column;
 				fetch_value(stmt, &ce_name_from_column, column_index_to_fetch++, NULL);
+				/* This used to use try_convert_to_string() which would silently support integers, floats, null
+				 * even if any such value could not generate a valid class name, as no class was found it would
+				 * then proceed to use stdClass */
 				if (Z_TYPE(ce_name_from_column) == IS_STRING) {
 					ce = zend_lookup_class(Z_STR(ce_name_from_column));
-					if (ce == NULL) {
-						ce = zend_standard_class_def;
-					}
 				}
 				/* Use default CE if present */
 				if (ce == NULL) {
@@ -779,7 +779,7 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
 			ZEND_ASSERT(ce != NULL);
 			if (flags & PDO_FETCH_SERIALIZE) {
 				if (!ce->unserialize) {
-					// TODO, should we mention the class? Considering this option is broken and deprecated...
+					/* As this option is deprecated we do not bother to mention the class name. */
 					pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "cannot unserialize class");
 					return false;
 				}
@@ -1071,30 +1071,22 @@ PHP_METHOD(PDOStatement, fetchObject)
 	old_ce = stmt->fetch.cls.ce;
 	old_ctor_args = stmt->fetch.cls.ctor_args;
 	stmt->fetch.cls.ctor_args = NULL;
+	if (ce == NULL) {
+		ce = zend_standard_class_def;
+	}
 
-	if (ce) {
-		stmt->fetch.cls.ce = ce;
-		if (ctor_args && zend_hash_num_elements(ctor_args)) {
-			if (ce->constructor == NULL) {
-				zend_argument_value_error(2, "must be empty when class provided in argument #1 ($class) does not have a constructor");
-				goto reset_old_ctor_args;
-			}
-			GC_TRY_ADDREF(ctor_args);
-			stmt->fetch.cls.ctor_args = ctor_args;
-		}
-	} else {
-		stmt->fetch.cls.ce = zend_standard_class_def;
+	if (ctor_args && zend_hash_num_elements(ctor_args) && ce->constructor == NULL) {
+		zend_argument_value_error(2, "must be empty when class provided in argument #1 ($class) does not have a constructor");
+		RETURN_THROWS();
 	}
+	stmt->fetch.cls.ce = ce;
+	stmt->fetch.cls.ctor_args = ctor_args;
 
 	if (!do_fetch(stmt, return_value, PDO_FETCH_CLASS, PDO_FETCH_ORI_NEXT, /* offset */ 0, NULL)) {
 		PDO_HANDLE_STMT_ERR();
 		RETVAL_FALSE;
 	}
 
-	reset_old_ctor_args:
-	if (stmt->fetch.cls.ctor_args != NULL) {
-		zend_hash_release(stmt->fetch.cls.ctor_args);
-	}
 	stmt->fetch.cls.ce = old_ce;
 	stmt->fetch.cls.ctor_args = old_ctor_args;
 }
@@ -1149,7 +1141,6 @@ PHP_METHOD(PDOStatement, fetchAll)
 	zval *arg2 = NULL;
 	zend_class_entry *old_ce;
 	HashTable *old_ctor_args, *ctor_args = NULL;
-	bool is_default_fetch_mode = false;
 
 	ZEND_PARSE_PARAMETERS_START(0, 3)
 		Z_PARAM_OPTIONAL
@@ -1171,31 +1162,34 @@ PHP_METHOD(PDOStatement, fetchAll)
 
 	/* TODO Would be good to reuse part of pdo_stmt_setup_fetch_mode() in some way */
 	switch (fetch_mode) {
-		case PDO_FETCH_CLASS:
+		case PDO_FETCH_CLASS: {
 			/* Figure out correct class */
+			zend_class_entry *fetch_class = NULL;
 			if (arg2) {
 				if (Z_TYPE_P(arg2) != IS_STRING) {
 					zend_argument_type_error(2, "must be of type string, %s given", zend_zval_value_name(arg2));
-					goto reset_old_ctor_args;
+					RETURN_THROWS();
 				}
-				stmt->fetch.cls.ce = zend_lookup_class(Z_STR_P(arg2));
-				if (!stmt->fetch.cls.ce) {
+				fetch_class = zend_lookup_class(Z_STR_P(arg2));
+				if (fetch_class == NULL) {
 					zend_argument_type_error(2, "must be a valid class");
-					goto reset_old_ctor_args;
+					RETURN_THROWS();
 				}
 			} else {
-				stmt->fetch.cls.ce = zend_standard_class_def;
+				fetch_class = zend_standard_class_def;
 			}
 
 			if (ctor_args && zend_hash_num_elements(ctor_args) > 0) {
-				if (!stmt->fetch.cls.ce->constructor) {
+				if (fetch_class->constructor == NULL) {
 					zend_argument_value_error(3, "must be empty when class provided in argument #2 ($class) does not have a constructor");
-					goto reset_old_ctor_args;
+					RETURN_THROWS();
 				}
-				GC_TRY_ADDREF(ctor_args);
 				stmt->fetch.cls.ctor_args = ctor_args;
 			}
+			stmt->fetch.cls.ce = fetch_class;
+
 			break;
+		}
 
 		case PDO_FETCH_FUNC: /* Cannot be a default fetch mode */
 			if (ZEND_NUM_ARGS() != 2) {
@@ -1250,7 +1244,6 @@ PHP_METHOD(PDOStatement, fetchAll)
 		flags |= stmt->default_fetch_type & PDO_FETCH_FLAGS;
 		fetch_mode = stmt->default_fetch_type & ~PDO_FETCH_FLAGS;
 		how = fetch_mode | flags;
-		is_default_fetch_mode = true;
 	}
 
 	PDO_STMT_CLEAR_ERR();
@@ -1290,15 +1283,9 @@ PHP_METHOD(PDOStatement, fetchAll)
 		}
 	}
 
-	/* Restore defaults which were changed by PDO_FETCH_CLASS mode */
-	if (!is_default_fetch_mode && fetch_mode == PDO_FETCH_CLASS) {
-reset_old_ctor_args:
-		if (stmt->fetch.cls.ctor_args != NULL) {
-			zend_hash_release(stmt->fetch.cls.ctor_args);
-		}
-		stmt->fetch.cls.ce = old_ce;
-		stmt->fetch.cls.ctor_args = old_ctor_args;
-	}
+	/* Restore defaults */
+	stmt->fetch.cls.ce = old_ce;
+	stmt->fetch.cls.ctor_args = old_ctor_args;
 
 	PDO_HANDLE_STMT_ERR();
 }
@@ -1594,6 +1581,24 @@ PHP_METHOD(PDOStatement, getColumnMeta)
 }
 /* }}} */
 
+void pdo_stmt_free_default_fetch_mode(pdo_stmt_t *stmt)
+{
+	enum pdo_fetch_type default_fetch_mode = stmt->default_fetch_type & ~PDO_FETCH_FLAGS;
+	if (default_fetch_mode == PDO_FETCH_INTO) {
+		// Assert that we have an object to fetch into?
+		if (stmt->fetch.into) {
+			OBJ_RELEASE(stmt->fetch.into);
+		}
+		stmt->fetch.into = NULL;
+	} else if (default_fetch_mode == PDO_FETCH_CLASS) {
+		if (stmt->fetch.cls.ctor_args != NULL) {
+			zend_hash_release(stmt->fetch.cls.ctor_args);
+		}
+		stmt->fetch.cls.ctor_args = NULL;
+		stmt->fetch.cls.ce = NULL;
+	}
+}
+
 /* {{{ Changes the default fetch mode for subsequent fetches (params have different meaning for different fetch modes) */
 
 bool pdo_stmt_setup_fetch_mode(pdo_stmt_t *stmt, zend_long mode, uint32_t mode_arg_num,
@@ -1604,22 +1609,7 @@ bool pdo_stmt_setup_fetch_mode(pdo_stmt_t *stmt, zend_long mode, uint32_t mode_a
 	uint32_t constructor_arg_num = mode_arg_num + 2;
 	uint32_t total_num_args = mode_arg_num + variadic_num_args;
 
-	switch (stmt->default_fetch_type) {
-		case PDO_FETCH_INTO:
-			if (stmt->fetch.into) {
-				OBJ_RELEASE(stmt->fetch.into);
-				stmt->fetch.into = NULL;
-			}
-			break;
-		case PDO_FETCH_CLASS:
-			if (stmt->fetch.cls.ctor_args != NULL) {
-				zend_hash_release(stmt->fetch.cls.ctor_args);
-				stmt->fetch.cls.ctor_args = NULL;
-			}
-			stmt->fetch.cls.ce = NULL;
-		default:
-			;
-	}
+	pdo_stmt_free_default_fetch_mode(stmt);
 
 	stmt->default_fetch_type = PDO_FETCH_BOTH;
 
@@ -2031,16 +2021,7 @@ PDO_API void php_pdo_free_statement(pdo_stmt_t *stmt)
 	}
 
 	pdo_stmt_reset_columns(stmt);
-
-	if (stmt->fetch.into && stmt->default_fetch_type == PDO_FETCH_INTO) {
-		OBJ_RELEASE(stmt->fetch.into);
-		stmt->fetch.into = NULL;
-	}
-
-	if (stmt->fetch.cls.ctor_args != NULL) {
-		zend_hash_release(stmt->fetch.cls.ctor_args);
-		stmt->fetch.cls.ctor_args = NULL;
-	}
+	pdo_stmt_free_default_fetch_mode(stmt);
 
 	if (!Z_ISUNDEF(stmt->database_object_handle)) {
 		zval_ptr_dtor(&stmt->database_object_handle);

ext/pdo/php_pdo_driver.h

@@ -612,14 +612,10 @@ struct _pdo_stmt_t {
 	union {
 		int column;
 		struct {
-			zval dummy; /* This exists due to alignment reasons with fetch.into */
 			HashTable *ctor_args;
 			zend_class_entry *ce;
 		} cls;
 		struct {
-			zval dummy; /* This exists due to alignment reasons with fetch.into */
-			HashTable *dummy_ht;  /* This exists to prevent conflict with cls.ctor_args */
-			zend_class_entry *dummy_ce;  /* This exists to prevent conflict with cls.ce */
 			zend_fcall_info_cache fcc;
 		} func;
 		zend_object *into;