Review fixes + optimize property reads + fix leak

kocsismate · kocsismate · commit 9ffcb5df1961 · 2026-06-13T23:12:01.000+02:00
diff --git a/ext/uri/php_uri.c b/ext/uri/php_uri.c
@@ -47,8 +47,8 @@ zend_class_entry *php_uri_ce_whatwg_url_validation_error;
 static zend_object_handlers object_handlers_rfc3986_uri;
 static zend_object_handlers object_handlers_whatwg_uri;
 
-typedef bool (*php_uri_string_component_validator)(const zend_string *component);
-typedef bool (*php_uri_long_component_validator)(zend_long component);
+typedef bool (*php_uri_component_validator_string)(const zend_string *component);
+typedef bool (*php_uri_component_validator_long)(zend_long component);
 
 static const zend_module_dep uri_deps[] = {
 	ZEND_MOD_REQUIRED("lexbor")
@@ -57,6 +57,14 @@ static const zend_module_dep uri_deps[] = {
 
 static zend_array uri_parsers;
 
+#define Z_RFC3986_URI_PROP_SCHEME_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 0)
+#define Z_RFC3986_URI_PROP_USERINFO_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 1)
+#define Z_RFC3986_URI_PROP_HOST_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 2)
+#define Z_RFC3986_URI_PROP_PORT_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 3)
+#define Z_RFC3986_URI_PROP_PATH_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 4)
+#define Z_RFC3986_URI_PROP_QUERY_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 5)
+#define Z_RFC3986_URI_PROP_FRAGMENT_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 6)
+
 static HashTable *uri_get_debug_properties(php_uri_object *object)
 {
 	const HashTable *std_properties = zend_std_get_properties(&object->std);
@@ -1057,19 +1065,20 @@ PHP_METHOD(Uri_Rfc3986_UriBuilder, reset)
 {
 	ZEND_PARSE_PARAMETERS_NONE();
 
-	zend_update_property_null(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("scheme"));
-	zend_update_property_null(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("userinfo"));
-	zend_update_property_null(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("host"));
-	zend_update_property_stringl(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("path"), "", 0);
-	zend_update_property_null(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("query"));
-	zend_update_property_null(php_uri_ce_rfc3986_uri_builder, Z_OBJ_P(ZEND_THIS), ZEND_STRL("fragment"));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_SCHEME_P(ZEND_THIS));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_USERINFO_P(ZEND_THIS));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_HOST_P(ZEND_THIS));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_PORT_P(ZEND_THIS));
+	ZVAL_EMPTY_STRING(Z_RFC3986_URI_PROP_PATH_P(ZEND_THIS));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_QUERY_P(ZEND_THIS));
+	ZVAL_NULL(Z_RFC3986_URI_PROP_FRAGMENT_P(ZEND_THIS));
 
 	RETVAL_COPY(ZEND_THIS);
 }
 
 ZEND_ATTRIBUTE_NONNULL static void php_uri_builder_set_component_string(
 	INTERNAL_FUNCTION_PARAMETERS, const char *name, size_t name_length,
-	const php_uri_string_component_validator validator
+	const php_uri_component_validator_string validator
 ) {
 	zend_string *component;
 
@@ -1089,7 +1098,7 @@ ZEND_ATTRIBUTE_NONNULL static void php_uri_builder_set_component_string(
 
 ZEND_ATTRIBUTE_NONNULL static void php_uri_builder_set_component_string_or_null(
 	INTERNAL_FUNCTION_PARAMETERS, const char *name, size_t name_length,
-	const php_uri_string_component_validator validator
+	const php_uri_component_validator_string validator
 ) {
 	zend_string *component;
 
@@ -1113,7 +1122,7 @@ ZEND_ATTRIBUTE_NONNULL static void php_uri_builder_set_component_string_or_null(
 
 ZEND_ATTRIBUTE_NONNULL_ARGS(1) static void php_uri_builder_set_component_long_or_null(
 	INTERNAL_FUNCTION_PARAMETERS, const char *name, size_t name_length,
-	const php_uri_long_component_validator validator
+	const php_uri_component_validator_long validator
 ) {
 	zend_long component;
 	bool component_is_null;
@@ -1208,16 +1217,13 @@ PHP_METHOD(Uri_Rfc3986_UriBuilder, build)
 		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(base_url, php_uri_ce_rfc3986_uri)
 	ZEND_PARSE_PARAMETERS_END();
 
-	zend_object *obj = Z_OBJ_P(ZEND_THIS);
-	zval tmp;
-
-	const zval *scheme = zend_read_property(obj->ce, obj, ZEND_STRL("scheme"), false, &tmp);
-	const zval *userinfo = zend_read_property(obj->ce, obj, ZEND_STRL("userinfo"), false, &tmp);
-	const zval *host = zend_read_property(obj->ce, obj, ZEND_STRL("host"), false, &tmp);
-	const zval *port = zend_read_property(obj->ce, obj, ZEND_STRL("port"), false, &tmp);
-	const zval *path = zend_read_property(obj->ce, obj, ZEND_STRL("path"), false, &tmp);
-	const zval *query = zend_read_property(obj->ce, obj, ZEND_STRL("query"), false, &tmp);
-	const zval *fragment = zend_read_property(obj->ce, obj, ZEND_STRL("fragment"), false, &tmp);
+	const zval *scheme = Z_RFC3986_URI_PROP_SCHEME_P(ZEND_THIS);
+	const zval *userinfo = Z_RFC3986_URI_PROP_USERINFO_P(ZEND_THIS);
+	const zval *host = Z_RFC3986_URI_PROP_HOST_P(ZEND_THIS);
+	const zval *port = Z_RFC3986_URI_PROP_PORT_P(ZEND_THIS);
+	const zval *path = Z_RFC3986_URI_PROP_PATH_P(ZEND_THIS);
+	const zval *query = Z_RFC3986_URI_PROP_QUERY_P(ZEND_THIS);
+	const zval *fragment = Z_RFC3986_URI_PROP_FRAGMENT_P(ZEND_THIS);
 
 	zend_string *uri_str = php_uri_parser_rfc3986_recompose_from_zval(scheme, userinfo, host, port, path, query, fragment);
 	if (uri_str == NULL) {
diff --git a/ext/uri/tests/rfc3986/builder/all_success_basic.phpt b/ext/uri/tests/rfc3986/builder/all_success_basic.phpt
diff --git a/ext/uri/tests/rfc3986/builder/all_success_with_reset.phpt b/ext/uri/tests/rfc3986/builder/all_success_with_reset.phpt
@@ -0,0 +1,63 @@
+--TEST--
+Test Uri\Rfc3986\UriBuilder all components - success - calling reset() afterwards
+--FILE--
+<?php
+
+$builder = new Uri\Rfc3986\UriBuilder()
+    ->setScheme("https")
+    ->setUserInfo("user:info")
+    ->setHost("example.com")
+    ->setPort(443)
+    ->setPath("/foo/bar/baz")
+    ->setQuery("foo=1&bar=baz")
+    ->setFragment("fragment");
+$uri = $builder->build();
+
+var_dump($uri->toRawString());
+var_dump($uri);
+
+$uri = $builder->reset()->build();
+
+var_dump($uri->toRawString());
+var_dump($uri);
+
+?>
+--EXPECTF--
+string(68) "https://user:info@example.com:443/foo/bar/baz?foo=1&bar=baz#fragment"
+object(Uri\Rfc3986\Uri)#%d (%d) {
+  ["scheme"]=>
+  string(5) "https"
+  ["username"]=>
+  string(4) "user"
+  ["password"]=>
+  string(4) "info"
+  ["host"]=>
+  string(11) "example.com"
+  ["port"]=>
+  int(443)
+  ["path"]=>
+  string(12) "/foo/bar/baz"
+  ["query"]=>
+  string(13) "foo=1&bar=baz"
+  ["fragment"]=>
+  string(8) "fragment"
+}
+string(0) ""
+object(Uri\Rfc3986\Uri)#%d (%d) {
+  ["scheme"]=>
+  NULL
+  ["username"]=>
+  NULL
+  ["password"]=>
+  NULL
+  ["host"]=>
+  NULL
+  ["port"]=>
+  NULL
+  ["path"]=>
+  string(0) ""
+  ["query"]=>
+  NULL
+  ["fragment"]=>
+  NULL
+}
diff --git a/ext/uri/tests/rfc3986/builder/path_error_special_char.phpt b/ext/uri/tests/rfc3986/builder/path_error_special_char.phpt
@@ -1,5 +1,5 @@
 --TEST--
-Test Uri\Rfc3986\UriBuilder::setUserInfo() - error - contains invalid special character
+Test Uri\Rfc3986\UriBuilder::setPath() - error - contains invalid special character
 --FILE--
 <?php
 
diff --git a/ext/uri/uri_parser_rfc3986.c b/ext/uri/uri_parser_rfc3986.c
@@ -733,19 +733,20 @@ ZEND_ATTRIBUTE_NONNULL bool php_uri_parser_rfc3986_validate_port(zend_long port)
 	const char *p = ZSTR_VAL(tmp);
 	const size_t len = ZSTR_LEN(tmp);
 
-	return uriIsWellFormedPortA(p, p + len);
+	bool result = uriIsWellFormedPortA(p, p + len);
+	zend_string_release(tmp);
+
+	return result;
 }
 
 ZEND_ATTRIBUTE_NONNULL bool php_uri_parser_rfc3986_validate_path(const zend_string *path)
 {
 	const char *p = ZSTR_VAL(path);
 	const size_t len = ZSTR_LEN(path);
 
-	/*
-	 It's checked during the build() method whether the path begins with a "/" when there's a host
-	 that's why a false hasHost argument is passed to uriIsWellFormedPathA() for now.
-	*/
-	return uriIsWellFormedPathA(p, p + len, false);
+	/* The build() method checks whether the path begins with a "/" when there's a host.
+	 * In order to skip doing the same check, a false hasHost argument is passed to uriIsWellFormedPathA(). */
+	return uriIsWellFormedPathA(p, p + len, /* hasHost */ false);
 }
 
 ZEND_ATTRIBUTE_NONNULL bool php_uri_parser_rfc3986_validate_query(const zend_string *query)
@@ -766,18 +767,26 @@ ZEND_ATTRIBUTE_NONNULL bool php_uri_parser_rfc3986_validate_fragment(const zend_
 
 ZEND_ATTRIBUTE_NONNULL zend_string *php_uri_parser_rfc3986_recompose_from_zval(
 	const zval *scheme, const zval *userinfo, const zval *host, const zval *port,
-	const zval *path, const zval *query, const zval *fragment)
-{
-	/* The userinfo and port components can only ever be present if the host is present */
-	if ((Z_TYPE_P(userinfo) != IS_NULL || Z_TYPE_P(port) != IS_NULL) && Z_TYPE_P(host) == IS_NULL) {
-		zend_throw_exception(php_uri_ce_invalid_uri_exception, "The URI must contain a host if either the userinfo or the port component is present", 0);
-		return NULL;
-	}
+	const zval *path, const zval *query, const zval *fragment
+) {
+	if (Z_TYPE_P(host) == IS_NULL) {
+		/* The path must not begin with "//" if the URI doesn't contain a host */
+		if (zend_string_starts_with_literal(Z_STR_P(path), "//")) {
+			zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \"//\" when the URI doesn't contain a host", 0);
+			return NULL;
+		}
 
-	/* The path must be either empty or begin with a "/" if the URI contains a host */
-	if (Z_TYPE_P(host) != IS_NULL && (Z_STRLEN_P(path) > 0 && Z_STRVAL_P(path)[0] != '/')) {
-		zend_throw_exception(php_uri_ce_invalid_uri_exception, "A path must be either empty or begin with \"/\" when the URI contains a host", 0);
-		return NULL;
+		/* The userinfo and port components must not be present if the URI doesn't contain a host */
+		if (Z_TYPE_P(userinfo) != IS_NULL || Z_TYPE_P(port) != IS_NULL) {
+			zend_throw_exception(php_uri_ce_invalid_uri_exception, "The URI must contain a host if either the userinfo or the port component is present", 0);
+			return NULL;
+		}
+	} else {
+		/* The path must be either empty or begin with a "/" if the URI contains a host */
+		if (Z_STRLEN_P(path) > 0 && Z_STRVAL_P(path)[0] != '/') {
+			zend_throw_exception(php_uri_ce_invalid_uri_exception, "A path must be either empty or begin with \"/\" when the URI contains a host", 0);
+			return NULL;
+		}
 	}
 
 	/* The first segment of the path must not contain ":" if the URI doesn't contain a scheme */
@@ -793,12 +802,6 @@ ZEND_ATTRIBUTE_NONNULL zend_string *php_uri_parser_rfc3986_recompose_from_zval(
 		}
 	}
 
-	/* The path must not begin with "//" if the URI doesn't contain a host */
-	if (Z_TYPE_P(host) == IS_NULL && Z_STRLEN_P(path) >= 2 && Z_STRVAL_P(path)[0] == '/' && Z_STRVAL_P(path)[1] == '/') {
-		zend_throw_exception(php_uri_ce_invalid_uri_exception, "The path must not begin with \"//\" when the URI doesn't contain a host", 0);
-		return NULL;
-	}
-
 	/* result = "" */
 	smart_str uri_str = {0};
 

-Original file line number
+Diff line change
@@ @@ -1,5 +1,5 @@ @@
 --TEST--
 -Test Uri\Rfc3986\UriBuilder::setUserInfo() - error - contains invalid special character
 +Test Uri\Rfc3986\UriBuilder::setPath() - error - contains invalid special character
 --FILE--
 <?php