From d1ab12f61776f8f30241f2bd5347e1177ed12939 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Sat, 23 Nov 2024 11:56:28 -0500 Subject: [PATCH 01/17] feat: add CurlPersistentShareHandle This commit introduces a new function, curl_persistent_share_init, that creates a persistent curl share handle by storing it within a hash table in the curl module's global variables. Persisting a curl share handle allows PHP userspace to cache things like DNS lookups or connections between multiple PHP requests, thus reducing work for subsequent requests. The function takes an array of CURL_LOCK_DATA_* constants, which are used to either construct a new curl share handle with the given options, or to find an existing one with the same set of options. CURL_LOCK_DATA_COOKIE is not allowed here, however, as it is considered unsafe; a developer could accidentally share a user's cookies from an earlier request when handling a subsequent from a different user. --- Zend/Optimizer/zend_func_infos.h | 1 + ext/curl/curl.stub.php | 11 +++ ext/curl/curl_arginfo.h | 18 ++++- ext/curl/curl_private.h | 19 ++++- ext/curl/interface.c | 48 +++++++++--- ext/curl/php_curl.h | 1 + ext/curl/share.c | 124 +++++++++++++++++++++++++++++++ 7 files changed, 210 insertions(+), 12 deletions(-) diff --git a/Zend/Optimizer/zend_func_infos.h b/Zend/Optimizer/zend_func_infos.h index 8751ff30c695..e2c9426663e9 100644 --- a/Zend/Optimizer/zend_func_infos.h +++ b/Zend/Optimizer/zend_func_infos.h @@ -47,6 +47,7 @@ static const func_info_t func_infos[] = { F1("curl_share_init", MAY_BE_OBJECT), F1("curl_share_strerror", MAY_BE_STRING|MAY_BE_NULL), F1("curl_strerror", MAY_BE_STRING|MAY_BE_NULL), + F1("curl_persistent_share_init", MAY_BE_OBJECT), F1("curl_version", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_STRING|MAY_BE_ARRAY_OF_LONG|MAY_BE_ARRAY_OF_STRING|MAY_BE_ARRAY_OF_ARRAY|MAY_BE_FALSE), F1("date", MAY_BE_STRING), F1("gmdate", MAY_BE_STRING), diff --git a/ext/curl/curl.stub.php b/ext/curl/curl.stub.php index 8a20231da562..f482940ee751 100644 --- a/ext/curl/curl.stub.php +++ b/ext/curl/curl.stub.php @@ -3668,6 +3668,14 @@ final class CurlShareHandle { } +/** + * @strict-properties + * @not-serializable + */ +final class CurlPersistentShareHandle +{ +} + function curl_close(CurlHandle $handle): void {} /** @refcount 1 */ @@ -3753,6 +3761,9 @@ function curl_share_strerror(int $error_code): ?string {} /** @refcount 1 */ function curl_strerror(int $error_code): ?string {} +/** @refcount 1 */ +function curl_persistent_share_init(array $share_options): CurlPersistentShareHandle {} + /** * @return array|false * @refcount 1 diff --git a/ext/curl/curl_arginfo.h b/ext/curl/curl_arginfo.h index 664cda7d32a9..2b2f3b875922 100644 --- a/ext/curl/curl_arginfo.h +++ b/ext/curl/curl_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: e2800e5ecc33f092576c7afcdb98d89825809669 */ + * Stub hash: 4ce3dbe06ea07fb6724fd5c68f7426558ff32169 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_close, 0, 1, IS_VOID, 0) ZEND_ARG_OBJ_INFO(0, handle, CurlHandle, 0) @@ -139,6 +139,10 @@ ZEND_END_ARG_INFO() #define arginfo_curl_strerror arginfo_curl_multi_strerror +ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_curl_persistent_share_init, 0, 1, CurlPersistentShareHandle, 0) + ZEND_ARG_TYPE_INFO(0, share_options, IS_ARRAY, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_curl_version, 0, 0, MAY_BE_ARRAY|MAY_BE_FALSE) ZEND_END_ARG_INFO() @@ -177,6 +181,7 @@ ZEND_FUNCTION(curl_share_init); ZEND_FUNCTION(curl_share_setopt); ZEND_FUNCTION(curl_share_strerror); ZEND_FUNCTION(curl_strerror); +ZEND_FUNCTION(curl_persistent_share_init); ZEND_FUNCTION(curl_version); static const zend_function_entry ext_functions[] = { @@ -215,6 +220,7 @@ static const zend_function_entry ext_functions[] = { ZEND_FE(curl_share_setopt, arginfo_curl_share_setopt) ZEND_FE(curl_share_strerror, arginfo_curl_share_strerror) ZEND_FE(curl_strerror, arginfo_curl_strerror) + ZEND_FE(curl_persistent_share_init, arginfo_curl_persistent_share_init) ZEND_FE(curl_version, arginfo_curl_version) ZEND_FE_END }; @@ -1137,3 +1143,13 @@ static zend_class_entry *register_class_CurlShareHandle(void) return class_entry; } + +static zend_class_entry *register_class_CurlPersistentShareHandle(void) +{ + zend_class_entry ce, *class_entry; + + INIT_CLASS_ENTRY(ce, "CurlPersistentShareHandle", NULL); + class_entry = zend_register_internal_class_with_flags(&ce, NULL, ZEND_ACC_FINAL|ZEND_ACC_NO_DYNAMIC_PROPERTIES|ZEND_ACC_NOT_SERIALIZABLE); + + return class_entry; +} diff --git a/ext/curl/curl_private.h b/ext/curl/curl_private.h index 8e8e9586db58..558a6329dd4a 100644 --- a/ext/curl/curl_private.h +++ b/ext/curl/curl_private.h @@ -40,9 +40,20 @@ #define SAVE_CURL_ERROR(__handle, __err) \ do { (__handle)->err.no = (int) __err; } while (0) + +ZEND_BEGIN_MODULE_GLOBALS(curl) + HashTable persistent_curlsh; +ZEND_END_MODULE_GLOBALS(curl) + +ZEND_EXTERN_MODULE_GLOBALS(curl) + +#define CURL_G(v) ZEND_MODULE_GLOBALS_ACCESSOR(curl, v) + PHP_MINIT_FUNCTION(curl); PHP_MSHUTDOWN_FUNCTION(curl); PHP_MINFO_FUNCTION(curl); +PHP_GINIT_FUNCTION(curl); +PHP_GSHUTDOWN_FUNCTION(curl); typedef struct { zend_fcall_info_cache fcc; @@ -125,11 +136,13 @@ typedef struct { } php_curlm; typedef struct _php_curlsh { - CURLSH *share; + CURLSH *share; + struct { int no; } err; - zend_object std; + + zend_object std; } php_curlsh; php_curl *init_curl_handle_into_zval(zval *curl); @@ -153,6 +166,8 @@ static inline php_curlsh *curl_share_from_obj(zend_object *obj) { void curl_multi_register_handlers(void); void curl_share_register_handlers(void); +void curl_share_free_persistent_curlsh(zval *data); +void curl_persistent_share_register_handlers(void); void curlfile_register_class(void); zend_result curl_cast_object(zend_object *obj, zval *result, int type); diff --git a/ext/curl/interface.c b/ext/curl/interface.c index 16b7c6618bc6..937e0ce03478 100644 --- a/ext/curl/interface.c +++ b/ext/curl/interface.c @@ -67,6 +67,8 @@ #include "curl_arginfo.h" +ZEND_DECLARE_MODULE_GLOBALS(curl) + #ifdef PHP_CURL_NEED_OPENSSL_TSL /* {{{ */ static MUTEX_T *php_curl_openssl_tsl = NULL; @@ -215,7 +217,11 @@ zend_module_entry curl_module_entry = { NULL, PHP_MINFO(curl), PHP_CURL_VERSION, - STANDARD_MODULE_PROPERTIES + PHP_MODULE_GLOBALS(curl), + PHP_GINIT(curl), + PHP_GSHUTDOWN(curl), + NULL, + STANDARD_MODULE_PROPERTIES_EX }; /* }}} */ @@ -223,10 +229,22 @@ zend_module_entry curl_module_entry = { ZEND_GET_MODULE (curl) #endif +PHP_GINIT_FUNCTION(curl) +{ + zend_hash_init(&curl_globals->persistent_curlsh, 0, NULL, curl_share_free_persistent_curlsh, true); + GC_MAKE_PERSISTENT_LOCAL(&curl_globals->persistent_curlsh); +} + +PHP_GSHUTDOWN_FUNCTION(curl) +{ + zend_hash_destroy(&curl_globals->persistent_curlsh); +} + /* CurlHandle class */ zend_class_entry *curl_ce; zend_class_entry *curl_share_ce; +zend_class_entry *curl_persistent_share_ce; static zend_object_handlers curl_object_handlers; static zend_object *curl_create_object(zend_class_entry *class_type); @@ -410,6 +428,10 @@ PHP_MINIT_FUNCTION(curl) curl_share_ce = register_class_CurlShareHandle(); curl_share_register_handlers(); + + curl_persistent_share_ce = register_class_CurlPersistentShareHandle(); + curl_persistent_share_register_handlers(); + curlfile_register_class(); return SUCCESS; @@ -2276,16 +2298,24 @@ static zend_result _php_curl_setopt(php_curl *ch, zend_long option, zval *zvalue case CURLOPT_SHARE: { - if (Z_TYPE_P(zvalue) == IS_OBJECT && Z_OBJCE_P(zvalue) == curl_share_ce) { - php_curlsh *sh = Z_CURL_SHARE_P(zvalue); - curl_easy_setopt(ch->cp, CURLOPT_SHARE, sh->share); + if (Z_TYPE_P(zvalue) != IS_OBJECT) { + break; + } - if (ch->share) { - OBJ_RELEASE(&ch->share->std); - } - GC_ADDREF(&sh->std); - ch->share = sh; + if (Z_OBJCE_P(zvalue) != curl_share_ce && Z_OBJCE_P(zvalue) != curl_persistent_share_ce) { + break; } + + php_curlsh *sh = Z_CURL_SHARE_P(zvalue); + + curl_easy_setopt(ch->cp, CURLOPT_SHARE, sh->share); + + if (ch->share) { + OBJ_RELEASE(&ch->share->std); + } + + GC_ADDREF(&sh->std); + ch->share = sh; } break; diff --git a/ext/curl/php_curl.h b/ext/curl/php_curl.h index bc92c51121ec..cae04217e0af 100644 --- a/ext/curl/php_curl.h +++ b/ext/curl/php_curl.h @@ -37,6 +37,7 @@ extern zend_module_entry curl_module_entry; PHP_CURL_API extern zend_class_entry *curl_ce; PHP_CURL_API extern zend_class_entry *curl_share_ce; +PHP_CURL_API extern zend_class_entry *curl_persistent_share_ce; PHP_CURL_API extern zend_class_entry *curl_multi_ce; PHP_CURL_API extern zend_class_entry *curl_CURLFile_class; PHP_CURL_API extern zend_class_entry *curl_CURLStringFile_class; diff --git a/ext/curl/share.c b/ext/curl/share.c index 406982d20327..678ee24c9dcc 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -21,6 +21,7 @@ #endif #include "php.h" +#include "Zend/zend_exceptions.h" #include "curl_private.h" @@ -134,6 +135,109 @@ PHP_FUNCTION(curl_share_strerror) } /* }}} */ +/** + * Initialize a persistent curl share handle with the given share options, reusing an existing one if found. + * + * Throws an exception if the share options are invalid. + */ +PHP_FUNCTION(curl_persistent_share_init) +{ + zval *share_opts = NULL, *entry = NULL; + zend_ulong persistent_id = 0; + + php_curlsh *sh; + + CURLSHcode error; + + ZEND_PARSE_PARAMETERS_START(1, 1) + Z_PARAM_ARRAY(share_opts) + ZEND_PARSE_PARAMETERS_END(); + + object_init_ex(return_value, curl_persistent_share_ce); + sh = Z_CURL_SHARE_P(return_value); + + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { + ZVAL_DEREF(entry); + + zend_ulong option = zval_get_long_ex(entry, true); + + if (option == CURL_LOCK_DATA_COOKIE) { + zend_throw_exception_ex( + NULL, + 0, + "CURL_LOCK_DATA_COOKIE is not allowed with persistent curl share handles" + ); + + goto error; + } + + // Ensure that each additional option results in a unique persistent ID. + persistent_id += 1 << option; + } ZEND_HASH_FOREACH_END(); + + if (persistent_id) { + zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); + + if (persisted) { + sh->share = Z_PTR_P(persisted); + + return; + } + } + + // We could not find an existing share handle, so we'll have to create one. + sh->share = curl_share_init(); + + // Apply $share_options to the handle. + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { + ZVAL_DEREF(entry); + + error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(entry)); + + if (error != CURLSHE_OK) { + zend_throw_exception_ex( + NULL, + 0, + "Could not construct persistent cURL share handle: %s", + curl_share_strerror(error) + ); + + goto error; + } + } ZEND_HASH_FOREACH_END(); + + zend_hash_index_add_new_ptr( + &CURL_G(persistent_curlsh), + persistent_id, + sh->share + ); + + return; + + error: + if (sh->share) { + curl_share_cleanup(sh->share); + } + + RETURN_THROWS(); +} + +/** + * Free a persistent curl share handle from the module global HashTable. + * + * See PHP_GINIT_FUNCTION in ext/curl/interface.c. + */ +void curl_share_free_persistent_curlsh(zval *data) +{ + CURLSH *handle = Z_PTR_P(data); + + if (!handle) { + return; + } + + curl_share_cleanup(handle); +} + /* CurlShareHandle class */ static zend_object *curl_share_create_object(zend_class_entry *class_type) { @@ -171,3 +275,23 @@ void curl_share_register_handlers(void) { curl_share_handlers.clone_obj = NULL; curl_share_handlers.compare = zend_objects_not_comparable; } + +/* CurlPersistentShareHandle class */ + +static zend_function *curl_persistent_share_get_constructor(zend_object *object) { + zend_throw_error(NULL, "Cannot directly construct CurlPersistentShareHandle, use curl_persistent_share_init() instead"); + return NULL; +} + +static zend_object_handlers curl_persistent_share_handlers; + +void curl_persistent_share_register_handlers(void) { + curl_persistent_share_ce->create_object = curl_share_create_object; + curl_persistent_share_ce->default_object_handlers = &curl_persistent_share_handlers; + + memcpy(&curl_persistent_share_handlers, &std_object_handlers, sizeof(zend_object_handlers)); + curl_persistent_share_handlers.offset = XtOffsetOf(php_curlsh, std); + curl_persistent_share_handlers.get_constructor = curl_persistent_share_get_constructor; + curl_persistent_share_handlers.clone_obj = NULL; + curl_persistent_share_handlers.compare = zend_objects_not_comparable; +} From 928113069e463da00db07d82a49a2eb6c78bca77 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Sun, 24 Nov 2024 10:47:11 -0500 Subject: [PATCH 02/17] feat: add share options as class property Notably, I needed to expose php_array_data_compare_unstable_i, in order to avoid re-implementing a comparison function. ext/intl/collator_sort.c chose to copy the implementation, and could maybe benefit from using the exposed function instead. --- ext/curl/curl.stub.php | 1 + ext/curl/curl_arginfo.h | 8 +++++++- ext/curl/share.c | 6 +++++- ext/standard/array.c | 2 +- ext/standard/php_array.h | 1 + 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/ext/curl/curl.stub.php b/ext/curl/curl.stub.php index f482940ee751..a379649d8c5e 100644 --- a/ext/curl/curl.stub.php +++ b/ext/curl/curl.stub.php @@ -3674,6 +3674,7 @@ final class CurlShareHandle */ final class CurlPersistentShareHandle { + public readonly array $options; } function curl_close(CurlHandle $handle): void {} diff --git a/ext/curl/curl_arginfo.h b/ext/curl/curl_arginfo.h index 2b2f3b875922..63d76e7bf1eb 100644 --- a/ext/curl/curl_arginfo.h +++ b/ext/curl/curl_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 4ce3dbe06ea07fb6724fd5c68f7426558ff32169 */ + * Stub hash: c423f7216d0c75563556edfcf678e3111360ecfc */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_close, 0, 1, IS_VOID, 0) ZEND_ARG_OBJ_INFO(0, handle, CurlHandle, 0) @@ -1151,5 +1151,11 @@ static zend_class_entry *register_class_CurlPersistentShareHandle(void) INIT_CLASS_ENTRY(ce, "CurlPersistentShareHandle", NULL); class_entry = zend_register_internal_class_with_flags(&ce, NULL, ZEND_ACC_FINAL|ZEND_ACC_NO_DYNAMIC_PROPERTIES|ZEND_ACC_NOT_SERIALIZABLE); + zval property_options_default_value; + ZVAL_UNDEF(&property_options_default_value); + zend_string *property_options_name = zend_string_init("options", sizeof("options") - 1, 1); + zend_declare_typed_property(class_entry, property_options_name, &property_options_default_value, ZEND_ACC_PUBLIC|ZEND_ACC_READONLY, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_ARRAY)); + zend_string_release(property_options_name); + return class_entry; } diff --git a/ext/curl/share.c b/ext/curl/share.c index 678ee24c9dcc..01c96b2c39d1 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -21,6 +21,7 @@ #endif #include "php.h" +#include "ext/standard/php_array.h" #include "Zend/zend_exceptions.h" #include "curl_private.h" @@ -150,7 +151,7 @@ PHP_FUNCTION(curl_persistent_share_init) CURLSHcode error; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_ARRAY(share_opts) + Z_PARAM_ARRAY_EX(share_opts, 0, 1) ZEND_PARSE_PARAMETERS_END(); object_init_ex(return_value, curl_persistent_share_ce); @@ -175,6 +176,9 @@ PHP_FUNCTION(curl_persistent_share_init) persistent_id += 1 << option; } ZEND_HASH_FOREACH_END(); + zend_array_sort(Z_ARRVAL_P(share_opts), php_array_data_compare_unstable_i, 1); + zend_update_property(curl_persistent_share_ce, Z_OBJ_P(return_value), "options", sizeof("options") - 1, share_opts); + if (persistent_id) { zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); diff --git a/ext/standard/array.c b/ext/standard/array.c index 73a5f1ee4a32..5c8da54b51ff 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -283,7 +283,7 @@ static zend_always_inline int php_array_key_compare_string_locale_unstable_i(Buc } /* }}} */ -static zend_always_inline int php_array_data_compare_unstable_i(Bucket *f, Bucket *s) /* {{{ */ +PHPAPI zend_always_inline int php_array_data_compare_unstable_i(Bucket *f, Bucket *s) /* {{{ */ { int result = zend_compare(&f->val, &s->val); /* Special enums handling for array_unique. We don't want to add this logic to zend_compare as diff --git a/ext/standard/php_array.h b/ext/standard/php_array.h index 2a35af603808..2c48ea8e5fb9 100644 --- a/ext/standard/php_array.h +++ b/ext/standard/php_array.h @@ -28,6 +28,7 @@ PHP_MSHUTDOWN_FUNCTION(array); PHPAPI int php_array_merge(HashTable *dest, HashTable *src); PHPAPI int php_array_merge_recursive(HashTable *dest, HashTable *src); PHPAPI int php_array_replace_recursive(HashTable *dest, HashTable *src); +PHPAPI int php_array_data_compare_unstable_i(Bucket *f, Bucket *s); PHPAPI int php_multisort_compare(const void *a, const void *b); PHPAPI zend_long php_count_recursive(HashTable *ht); From 85c13d9dc6f48f7c60af433e1a981bc00a461a98 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Mon, 25 Nov 2024 13:11:51 -0500 Subject: [PATCH 03/17] test: persistent curl share handles I opted to use the existing Caddy testing infrastructure since it supports keepalives, whereas it seems the PHP development server does not. Alternatively I could write just enough of a socket listener to confirm that there is only ever a single connection coming from curl, but I believe it is safe to rely on connect_time being zero when a connection is reused. --- ext/curl/tests/curl_persistent_share_001.phpt | 46 +++++++++++++++++++ ext/curl/tests/curl_persistent_share_002.phpt | 42 +++++++++++++++++ ext/curl/tests/curl_persistent_share_003.phpt | 16 +++++++ ext/curl/tests/curl_persistent_share_004.phpt | 16 +++++++ ext/curl/tests/curl_persistent_share_005.phpt | 17 +++++++ ext/curl/tests/skipif-nocaddy.inc | 1 + 6 files changed, 138 insertions(+) create mode 100644 ext/curl/tests/curl_persistent_share_001.phpt create mode 100644 ext/curl/tests/curl_persistent_share_002.phpt create mode 100644 ext/curl/tests/curl_persistent_share_003.phpt create mode 100644 ext/curl/tests/curl_persistent_share_004.phpt create mode 100644 ext/curl/tests/curl_persistent_share_005.phpt diff --git a/ext/curl/tests/curl_persistent_share_001.phpt b/ext/curl/tests/curl_persistent_share_001.phpt new file mode 100644 index 000000000000..99aa363f178e --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_001.phpt @@ -0,0 +1,46 @@ +--TEST-- +Basic curl persistent share handle test +--EXTENSIONS-- +curl +--SKIPIF-- + +--FILE-- +options); + +$sh2 = curl_persistent_share_init([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT]); +$ch2 = get_localhost_curl_handle($sh2); + +// Expect the connect time on the subsequent request to be zero, since it's reusing the connection. +var_dump(curl_exec($ch1)); +var_dump(curl_exec($ch2)); +var_dump("second connect_time: " . (curl_getinfo($ch2)["connect_time"])); + +?> +--EXPECT-- +array(2) { + [0]=> + int(3) + [1]=> + int(5) +} +string(23) "Caddy is up and running" +string(23) "Caddy is up and running" +string(22) "second connect_time: 0" diff --git a/ext/curl/tests/curl_persistent_share_002.phpt b/ext/curl/tests/curl_persistent_share_002.phpt new file mode 100644 index 000000000000..daa52ad49d39 --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_002.phpt @@ -0,0 +1,42 @@ +--TEST-- +Curl persistent share handle test with different options +--EXTENSIONS-- +curl +--SKIPIF-- + +--FILE-- +options != $sh2->options); + +// Expect the connect time on the subsequent request to be non-zero, since it's reusing the connection. +var_dump(curl_exec($ch1)); +var_dump(curl_exec($ch2)); +var_dump("second connect_time: " . (curl_getinfo($ch2)["connect_time"])); + +?> +--EXPECTREGEX-- +bool\(true\) +string\(23\) "Caddy is up and running" +string\(23\) "Caddy is up and running" +string\(\d+\) "second connect_time: .*[1-9].*" diff --git a/ext/curl/tests/curl_persistent_share_003.phpt b/ext/curl/tests/curl_persistent_share_003.phpt new file mode 100644 index 000000000000..81a371221b7d --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_003.phpt @@ -0,0 +1,16 @@ +--TEST-- +Curl persistent share handle test with invalid option +--EXTENSIONS-- +curl +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught Exception: Could not construct persistent cURL share handle: Unknown share option in %scurl_persistent_share_003.php:3 +Stack trace: +#0 %scurl_persistent_share_003.php(3): curl_persistent_share_init(Array) +#1 {main} + thrown in %scurl_persistent_share_003.php on line 3 diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt new file mode 100644 index 000000000000..2c1a0b6c5278 --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -0,0 +1,16 @@ +--TEST-- +Curl persistent share handle test with disallowed option +--EXTENSIONS-- +curl +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught Exception: CURL_LOCK_DATA_COOKIE is not allowed with persistent curl share handles in %scurl_persistent_share_004.php:3 +Stack trace: +#0 %scurl_persistent_share_004.php(3): curl_persistent_share_init(Array) +#1 {main} + thrown in %scurl_persistent_share_004.php on line 3 diff --git a/ext/curl/tests/curl_persistent_share_005.phpt b/ext/curl/tests/curl_persistent_share_005.phpt new file mode 100644 index 000000000000..ab2811f5e760 --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_005.phpt @@ -0,0 +1,17 @@ +--TEST-- +Curl persistent share handles cannot be used with curl_share_setopt +--EXTENSIONS-- +curl +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught TypeError: curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlPersistentShareHandle given in %scurl_persistent_share_005.php:4 +Stack trace: +#0 %scurl_persistent_share_005.php(4): curl_share_setopt(Object(CurlPersistentShareHandle), %d, %d) +#1 {main} + thrown in %scurl_persistent_share_005.php on line 4 diff --git a/ext/curl/tests/skipif-nocaddy.inc b/ext/curl/tests/skipif-nocaddy.inc index 21a06c12af10..ae5442ff28ed 100644 --- a/ext/curl/tests/skipif-nocaddy.inc +++ b/ext/curl/tests/skipif-nocaddy.inc @@ -2,6 +2,7 @@ $ch = curl_init("https://localhost"); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); +curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false); $body = curl_exec($ch); From 73a6ba03bd0bbc645a8384c7ab781f5a96c86a65 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Wed, 27 Nov 2024 13:13:31 -0500 Subject: [PATCH 04/17] revert: diff to `php_curlsh` --- ext/curl/curl_private.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ext/curl/curl_private.h b/ext/curl/curl_private.h index 558a6329dd4a..f0cb61a7c4a6 100644 --- a/ext/curl/curl_private.h +++ b/ext/curl/curl_private.h @@ -136,13 +136,11 @@ typedef struct { } php_curlm; typedef struct _php_curlsh { - CURLSH *share; - + CURLSH *share; struct { int no; } err; - - zend_object std; + zend_object std; } php_curlsh; php_curl *init_curl_handle_into_zval(zval *curl); From 39ff28bd911440ed2521a6a5d4f6537a7ce4c9a6 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Wed, 27 Nov 2024 13:13:53 -0500 Subject: [PATCH 05/17] refactor: rename to `curl_share_init_persistent` --- Zend/Optimizer/zend_func_infos.h | 2 +- ext/curl/curl.stub.php | 6 ++-- ext/curl/curl_arginfo.h | 16 +++++----- ext/curl/curl_private.h | 2 +- ext/curl/interface.c | 8 ++--- ext/curl/php_curl.h | 2 +- ext/curl/share.c | 32 +++++++++---------- ext/curl/tests/curl_persistent_share_001.phpt | 6 ++-- ext/curl/tests/curl_persistent_share_002.phpt | 6 ++-- ext/curl/tests/curl_persistent_share_003.phpt | 4 +-- ext/curl/tests/curl_persistent_share_004.phpt | 4 +-- ext/curl/tests/curl_persistent_share_005.phpt | 6 ++-- 12 files changed, 47 insertions(+), 47 deletions(-) diff --git a/Zend/Optimizer/zend_func_infos.h b/Zend/Optimizer/zend_func_infos.h index e2c9426663e9..f2e0c2d3c9aa 100644 --- a/Zend/Optimizer/zend_func_infos.h +++ b/Zend/Optimizer/zend_func_infos.h @@ -46,8 +46,8 @@ static const func_info_t func_infos[] = { F1("curl_multi_strerror", MAY_BE_STRING|MAY_BE_NULL), F1("curl_share_init", MAY_BE_OBJECT), F1("curl_share_strerror", MAY_BE_STRING|MAY_BE_NULL), + F1("curl_share_init_persistent", MAY_BE_OBJECT), F1("curl_strerror", MAY_BE_STRING|MAY_BE_NULL), - F1("curl_persistent_share_init", MAY_BE_OBJECT), F1("curl_version", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_STRING|MAY_BE_ARRAY_OF_LONG|MAY_BE_ARRAY_OF_STRING|MAY_BE_ARRAY_OF_ARRAY|MAY_BE_FALSE), F1("date", MAY_BE_STRING), F1("gmdate", MAY_BE_STRING), diff --git a/ext/curl/curl.stub.php b/ext/curl/curl.stub.php index a379649d8c5e..9eb8bd5b49e8 100644 --- a/ext/curl/curl.stub.php +++ b/ext/curl/curl.stub.php @@ -3672,7 +3672,7 @@ final class CurlShareHandle * @strict-properties * @not-serializable */ -final class CurlPersistentShareHandle +final class CurlSharePersistentHandle { public readonly array $options; } @@ -3760,10 +3760,10 @@ function curl_share_setopt(CurlShareHandle $share_handle, int $option, mixed $va function curl_share_strerror(int $error_code): ?string {} /** @refcount 1 */ -function curl_strerror(int $error_code): ?string {} +function curl_share_init_persistent(array $share_options): CurlSharePersistentHandle {} /** @refcount 1 */ -function curl_persistent_share_init(array $share_options): CurlPersistentShareHandle {} +function curl_strerror(int $error_code): ?string {} /** * @return array|false diff --git a/ext/curl/curl_arginfo.h b/ext/curl/curl_arginfo.h index 63d76e7bf1eb..e873e9e9f327 100644 --- a/ext/curl/curl_arginfo.h +++ b/ext/curl/curl_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: c423f7216d0c75563556edfcf678e3111360ecfc */ + * Stub hash: 7d3cd96f8725c59be46817487bb8d06e04384269 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_curl_close, 0, 1, IS_VOID, 0) ZEND_ARG_OBJ_INFO(0, handle, CurlHandle, 0) @@ -137,12 +137,12 @@ ZEND_END_ARG_INFO() #define arginfo_curl_share_strerror arginfo_curl_multi_strerror -#define arginfo_curl_strerror arginfo_curl_multi_strerror - -ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_curl_persistent_share_init, 0, 1, CurlPersistentShareHandle, 0) +ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_curl_share_init_persistent, 0, 1, CurlSharePersistentHandle, 0) ZEND_ARG_TYPE_INFO(0, share_options, IS_ARRAY, 0) ZEND_END_ARG_INFO() +#define arginfo_curl_strerror arginfo_curl_multi_strerror + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_curl_version, 0, 0, MAY_BE_ARRAY|MAY_BE_FALSE) ZEND_END_ARG_INFO() @@ -180,8 +180,8 @@ ZEND_FUNCTION(curl_share_errno); ZEND_FUNCTION(curl_share_init); ZEND_FUNCTION(curl_share_setopt); ZEND_FUNCTION(curl_share_strerror); +ZEND_FUNCTION(curl_share_init_persistent); ZEND_FUNCTION(curl_strerror); -ZEND_FUNCTION(curl_persistent_share_init); ZEND_FUNCTION(curl_version); static const zend_function_entry ext_functions[] = { @@ -219,8 +219,8 @@ static const zend_function_entry ext_functions[] = { ZEND_FE(curl_share_init, arginfo_curl_share_init) ZEND_FE(curl_share_setopt, arginfo_curl_share_setopt) ZEND_FE(curl_share_strerror, arginfo_curl_share_strerror) + ZEND_FE(curl_share_init_persistent, arginfo_curl_share_init_persistent) ZEND_FE(curl_strerror, arginfo_curl_strerror) - ZEND_FE(curl_persistent_share_init, arginfo_curl_persistent_share_init) ZEND_FE(curl_version, arginfo_curl_version) ZEND_FE_END }; @@ -1144,11 +1144,11 @@ static zend_class_entry *register_class_CurlShareHandle(void) return class_entry; } -static zend_class_entry *register_class_CurlPersistentShareHandle(void) +static zend_class_entry *register_class_CurlSharePersistentHandle(void) { zend_class_entry ce, *class_entry; - INIT_CLASS_ENTRY(ce, "CurlPersistentShareHandle", NULL); + INIT_CLASS_ENTRY(ce, "CurlSharePersistentHandle", NULL); class_entry = zend_register_internal_class_with_flags(&ce, NULL, ZEND_ACC_FINAL|ZEND_ACC_NO_DYNAMIC_PROPERTIES|ZEND_ACC_NOT_SERIALIZABLE); zval property_options_default_value; diff --git a/ext/curl/curl_private.h b/ext/curl/curl_private.h index f0cb61a7c4a6..dc23b5910cfb 100644 --- a/ext/curl/curl_private.h +++ b/ext/curl/curl_private.h @@ -164,8 +164,8 @@ static inline php_curlsh *curl_share_from_obj(zend_object *obj) { void curl_multi_register_handlers(void); void curl_share_register_handlers(void); +void curl_share_persistent_register_handlers(void); void curl_share_free_persistent_curlsh(zval *data); -void curl_persistent_share_register_handlers(void); void curlfile_register_class(void); zend_result curl_cast_object(zend_object *obj, zval *result, int type); diff --git a/ext/curl/interface.c b/ext/curl/interface.c index 937e0ce03478..45ef64854669 100644 --- a/ext/curl/interface.c +++ b/ext/curl/interface.c @@ -244,7 +244,7 @@ PHP_GSHUTDOWN_FUNCTION(curl) zend_class_entry *curl_ce; zend_class_entry *curl_share_ce; -zend_class_entry *curl_persistent_share_ce; +zend_class_entry *curl_share_persistent_ce; static zend_object_handlers curl_object_handlers; static zend_object *curl_create_object(zend_class_entry *class_type); @@ -429,8 +429,8 @@ PHP_MINIT_FUNCTION(curl) curl_share_ce = register_class_CurlShareHandle(); curl_share_register_handlers(); - curl_persistent_share_ce = register_class_CurlPersistentShareHandle(); - curl_persistent_share_register_handlers(); + curl_share_persistent_ce = register_class_CurlSharePersistentHandle(); + curl_share_persistent_register_handlers(); curlfile_register_class(); @@ -2302,7 +2302,7 @@ static zend_result _php_curl_setopt(php_curl *ch, zend_long option, zval *zvalue break; } - if (Z_OBJCE_P(zvalue) != curl_share_ce && Z_OBJCE_P(zvalue) != curl_persistent_share_ce) { + if (Z_OBJCE_P(zvalue) != curl_share_ce && Z_OBJCE_P(zvalue) != curl_share_persistent_ce) { break; } diff --git a/ext/curl/php_curl.h b/ext/curl/php_curl.h index cae04217e0af..6084d5935c70 100644 --- a/ext/curl/php_curl.h +++ b/ext/curl/php_curl.h @@ -37,7 +37,7 @@ extern zend_module_entry curl_module_entry; PHP_CURL_API extern zend_class_entry *curl_ce; PHP_CURL_API extern zend_class_entry *curl_share_ce; -PHP_CURL_API extern zend_class_entry *curl_persistent_share_ce; +PHP_CURL_API extern zend_class_entry *curl_share_persistent_ce; PHP_CURL_API extern zend_class_entry *curl_multi_ce; PHP_CURL_API extern zend_class_entry *curl_CURLFile_class; PHP_CURL_API extern zend_class_entry *curl_CURLStringFile_class; diff --git a/ext/curl/share.c b/ext/curl/share.c index 01c96b2c39d1..bd242d57f227 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -141,7 +141,7 @@ PHP_FUNCTION(curl_share_strerror) * * Throws an exception if the share options are invalid. */ -PHP_FUNCTION(curl_persistent_share_init) +PHP_FUNCTION(curl_share_init_persistent) { zval *share_opts = NULL, *entry = NULL; zend_ulong persistent_id = 0; @@ -154,7 +154,7 @@ PHP_FUNCTION(curl_persistent_share_init) Z_PARAM_ARRAY_EX(share_opts, 0, 1) ZEND_PARSE_PARAMETERS_END(); - object_init_ex(return_value, curl_persistent_share_ce); + object_init_ex(return_value, curl_share_persistent_ce); sh = Z_CURL_SHARE_P(return_value); ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { @@ -177,7 +177,7 @@ PHP_FUNCTION(curl_persistent_share_init) } ZEND_HASH_FOREACH_END(); zend_array_sort(Z_ARRVAL_P(share_opts), php_array_data_compare_unstable_i, 1); - zend_update_property(curl_persistent_share_ce, Z_OBJ_P(return_value), "options", sizeof("options") - 1, share_opts); + zend_update_property(curl_share_persistent_ce, Z_OBJ_P(return_value), "options", sizeof("options") - 1, share_opts); if (persistent_id) { zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); @@ -280,22 +280,22 @@ void curl_share_register_handlers(void) { curl_share_handlers.compare = zend_objects_not_comparable; } -/* CurlPersistentShareHandle class */ +/* CurlSharePersistentHandle class */ -static zend_function *curl_persistent_share_get_constructor(zend_object *object) { - zend_throw_error(NULL, "Cannot directly construct CurlPersistentShareHandle, use curl_persistent_share_init() instead"); +static zend_object_handlers curl_share_persistent_handlers; + +static zend_function *curl_share_persistent_get_constructor(zend_object *object) { + zend_throw_error(NULL, "Cannot directly construct CurlSharePersistentHandle, use curl_share_init_persistent() instead"); return NULL; } -static zend_object_handlers curl_persistent_share_handlers; - -void curl_persistent_share_register_handlers(void) { - curl_persistent_share_ce->create_object = curl_share_create_object; - curl_persistent_share_ce->default_object_handlers = &curl_persistent_share_handlers; +void curl_share_persistent_register_handlers(void) { + curl_share_persistent_ce->create_object = curl_share_create_object; + curl_share_persistent_ce->default_object_handlers = &curl_share_persistent_handlers; - memcpy(&curl_persistent_share_handlers, &std_object_handlers, sizeof(zend_object_handlers)); - curl_persistent_share_handlers.offset = XtOffsetOf(php_curlsh, std); - curl_persistent_share_handlers.get_constructor = curl_persistent_share_get_constructor; - curl_persistent_share_handlers.clone_obj = NULL; - curl_persistent_share_handlers.compare = zend_objects_not_comparable; + memcpy(&curl_share_persistent_handlers, &std_object_handlers, sizeof(zend_object_handlers)); + curl_share_persistent_handlers.offset = XtOffsetOf(php_curlsh, std); + curl_share_persistent_handlers.get_constructor = curl_share_persistent_get_constructor; + curl_share_persistent_handlers.clone_obj = NULL; + curl_share_persistent_handlers.compare = zend_objects_not_comparable; } diff --git a/ext/curl/tests/curl_persistent_share_001.phpt b/ext/curl/tests/curl_persistent_share_001.phpt index 99aa363f178e..ceaec5cc17aa 100644 --- a/ext/curl/tests/curl_persistent_share_001.phpt +++ b/ext/curl/tests/curl_persistent_share_001.phpt @@ -9,7 +9,7 @@ include 'skipif-nocaddy.inc'; --FILE-- options); -$sh2 = curl_persistent_share_init([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT]); +$sh2 = curl_share_init_persistent([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT]); $ch2 = get_localhost_curl_handle($sh2); // Expect the connect time on the subsequent request to be zero, since it's reusing the connection. diff --git a/ext/curl/tests/curl_persistent_share_002.phpt b/ext/curl/tests/curl_persistent_share_002.phpt index daa52ad49d39..05bd569c50c8 100644 --- a/ext/curl/tests/curl_persistent_share_002.phpt +++ b/ext/curl/tests/curl_persistent_share_002.phpt @@ -9,7 +9,7 @@ include 'skipif-nocaddy.inc'; --FILE-- options != $sh2->options); diff --git a/ext/curl/tests/curl_persistent_share_003.phpt b/ext/curl/tests/curl_persistent_share_003.phpt index 81a371221b7d..99910bd80fdc 100644 --- a/ext/curl/tests/curl_persistent_share_003.phpt +++ b/ext/curl/tests/curl_persistent_share_003.phpt @@ -5,12 +5,12 @@ curl --FILE-- --EXPECTF-- Fatal error: Uncaught Exception: Could not construct persistent cURL share handle: Unknown share option in %scurl_persistent_share_003.php:3 Stack trace: -#0 %scurl_persistent_share_003.php(3): curl_persistent_share_init(Array) +#0 %scurl_persistent_share_003.php(3): curl_share_init_persistent(Array) #1 {main} thrown in %scurl_persistent_share_003.php on line 3 diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index 2c1a0b6c5278..a16cb5123bb7 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -5,12 +5,12 @@ curl --FILE-- --EXPECTF-- Fatal error: Uncaught Exception: CURL_LOCK_DATA_COOKIE is not allowed with persistent curl share handles in %scurl_persistent_share_004.php:3 Stack trace: -#0 %scurl_persistent_share_004.php(3): curl_persistent_share_init(Array) +#0 %scurl_persistent_share_004.php(3): curl_share_init_persistent(Array) #1 {main} thrown in %scurl_persistent_share_004.php on line 3 diff --git a/ext/curl/tests/curl_persistent_share_005.phpt b/ext/curl/tests/curl_persistent_share_005.phpt index ab2811f5e760..9a9c4d90d117 100644 --- a/ext/curl/tests/curl_persistent_share_005.phpt +++ b/ext/curl/tests/curl_persistent_share_005.phpt @@ -5,13 +5,13 @@ curl --FILE-- --EXPECTF-- -Fatal error: Uncaught TypeError: curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlPersistentShareHandle given in %scurl_persistent_share_005.php:4 +Fatal error: Uncaught TypeError: curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlSharePersistentHandle given in %scurl_persistent_share_005.php:4 Stack trace: -#0 %scurl_persistent_share_005.php(4): curl_share_setopt(Object(CurlPersistentShareHandle), %d, %d) +#0 %scurl_persistent_share_005.php(4): curl_share_setopt(Object(CurlSharePersistentHandle), %d, %d) #1 {main} thrown in %scurl_persistent_share_005.php on line 4 From ee836f3995e4bfc5dcd03b55682497242c2a12d4 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Wed, 27 Nov 2024 15:12:23 -0500 Subject: [PATCH 06/17] address first round of PR comments - use hardcoded allow list for $share_options - use zval_try_get_long --- ext/curl/share.c | 39 ++++++++++++++----- ext/curl/tests/curl_persistent_share_003.phpt | 2 +- ext/curl/tests/curl_persistent_share_004.phpt | 2 +- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index bd242d57f227..60106925c290 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -160,20 +160,39 @@ PHP_FUNCTION(curl_share_init_persistent) ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { ZVAL_DEREF(entry); - zend_ulong option = zval_get_long_ex(entry, true); - - if (option == CURL_LOCK_DATA_COOKIE) { - zend_throw_exception_ex( - NULL, - 0, - "CURL_LOCK_DATA_COOKIE is not allowed with persistent curl share handles" - ); + bool failed = false; + zend_ulong option = zval_try_get_long(entry, &failed); + if (failed) { + zend_argument_type_error(1, "must contain only longs, %s given", zend_zval_value_name(entry)); goto error; } - // Ensure that each additional option results in a unique persistent ID. - persistent_id += 1 << option; + switch (option) { + // Disallowed options + case CURL_LOCK_DATA_COOKIE: + zend_argument_value_error(1, "CURL_LOCK_DATA_COOKIE is not allowed"); + goto error; + + // Allowed options + case CURL_LOCK_DATA_DNS: + persistent_id |= 1 << 0; + break; + case CURL_LOCK_DATA_SSL_SESSION: + persistent_id |= 1 << 1; + break; + case CURL_LOCK_DATA_CONNECT: + persistent_id |= 1 << 2; + break; + case CURL_LOCK_DATA_PSL: + persistent_id |= 1 << 3; + break; + + // Unknown options + default: + zend_argument_value_error(1, "must contain only CURL_LOCK_DATA_* constants"); + goto error; + } } ZEND_HASH_FOREACH_END(); zend_array_sort(Z_ARRVAL_P(share_opts), php_array_data_compare_unstable_i, 1); diff --git a/ext/curl/tests/curl_persistent_share_003.phpt b/ext/curl/tests/curl_persistent_share_003.phpt index 99910bd80fdc..a64440c11883 100644 --- a/ext/curl/tests/curl_persistent_share_003.phpt +++ b/ext/curl/tests/curl_persistent_share_003.phpt @@ -9,7 +9,7 @@ $sh = curl_share_init_persistent([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT, 30 ?> --EXPECTF-- -Fatal error: Uncaught Exception: Could not construct persistent cURL share handle: Unknown share option in %scurl_persistent_share_003.php:3 +Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must contain only CURL_LOCK_DATA_* constants in %scurl_persistent_share_003.php:3 Stack trace: #0 %scurl_persistent_share_003.php(3): curl_share_init_persistent(Array) #1 {main} diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index a16cb5123bb7..af1d7924375e 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -9,7 +9,7 @@ $sh = curl_share_init_persistent([CURL_LOCK_DATA_COOKIE]); ?> --EXPECTF-- -Fatal error: Uncaught Exception: CURL_LOCK_DATA_COOKIE is not allowed with persistent curl share handles in %scurl_persistent_share_004.php:3 +Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) CURL_LOCK_DATA_COOKIE is not allowed in %scurl_persistent_share_004.php:3 Stack trace: #0 %scurl_persistent_share_004.php(3): curl_share_init_persistent(Array) #1 {main} From 69bb4c0f999e0df7c6d17657299207edffa7b816 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Thu, 2 Jan 2025 11:28:06 -0500 Subject: [PATCH 07/17] address second round of PR comments --- ext/curl/share.c | 62 ++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 60106925c290..5991d0c25155 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -146,58 +146,58 @@ PHP_FUNCTION(curl_share_init_persistent) zval *share_opts = NULL, *entry = NULL; zend_ulong persistent_id = 0; - php_curlsh *sh; + php_curlsh *sh = NULL; - CURLSHcode error; + CURLSHcode error = 0; ZEND_PARSE_PARAMETERS_START(1, 1) Z_PARAM_ARRAY_EX(share_opts, 0, 1) ZEND_PARSE_PARAMETERS_END(); - object_init_ex(return_value, curl_share_persistent_ce); - sh = Z_CURL_SHARE_P(return_value); - ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { ZVAL_DEREF(entry); - bool failed = false; + bool failed = false; zend_ulong option = zval_try_get_long(entry, &failed); if (failed) { - zend_argument_type_error(1, "must contain only longs, %s given", zend_zval_value_name(entry)); + zend_argument_type_error(1, "must contain only integer values, %s given", zend_zval_value_name(entry)); goto error; } switch (option) { - // Disallowed options - case CURL_LOCK_DATA_COOKIE: - zend_argument_value_error(1, "CURL_LOCK_DATA_COOKIE is not allowed"); - goto error; - - // Allowed options - case CURL_LOCK_DATA_DNS: - persistent_id |= 1 << 0; - break; - case CURL_LOCK_DATA_SSL_SESSION: - persistent_id |= 1 << 1; - break; - case CURL_LOCK_DATA_CONNECT: - persistent_id |= 1 << 2; - break; - case CURL_LOCK_DATA_PSL: - persistent_id |= 1 << 3; - break; - - // Unknown options - default: - zend_argument_value_error(1, "must contain only CURL_LOCK_DATA_* constants"); - goto error; + // Disallowed options + case CURL_LOCK_DATA_COOKIE: + zend_argument_value_error(1, "CURL_LOCK_DATA_COOKIE is not allowed"); + goto error; + + // Allowed options + case CURL_LOCK_DATA_DNS: + persistent_id |= 1 << 0; + break; + case CURL_LOCK_DATA_SSL_SESSION: + persistent_id |= 1 << 1; + break; + case CURL_LOCK_DATA_CONNECT: + persistent_id |= 1 << 2; + break; + case CURL_LOCK_DATA_PSL: + persistent_id |= 1 << 3; + break; + + // Unknown options + default: + zend_argument_value_error(1, "must contain only CURL_LOCK_DATA_* constants"); + goto error; } } ZEND_HASH_FOREACH_END(); zend_array_sort(Z_ARRVAL_P(share_opts), php_array_data_compare_unstable_i, 1); zend_update_property(curl_share_persistent_ce, Z_OBJ_P(return_value), "options", sizeof("options") - 1, share_opts); + object_init_ex(return_value, curl_share_persistent_ce); + sh = Z_CURL_SHARE_P(return_value); + if (persistent_id) { zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); @@ -238,7 +238,7 @@ PHP_FUNCTION(curl_share_init_persistent) return; error: - if (sh->share) { + if (sh && sh->share) { curl_share_cleanup(sh->share); } From 62dbc12797ecce1ead7a4d909b8b88fc6162ad4a Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Thu, 2 Jan 2025 14:59:26 -0500 Subject: [PATCH 08/17] refactor: manually construct `->options` property --- ext/curl/share.c | 34 +++++++++++++++++-- ext/curl/tests/curl_persistent_share_001.phpt | 4 +-- ext/standard/array.c | 2 +- ext/standard/php_array.h | 1 - 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 5991d0c25155..2aa30d13430e 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -151,7 +151,7 @@ PHP_FUNCTION(curl_share_init_persistent) CURLSHcode error = 0; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_ARRAY_EX(share_opts, 0, 1) + Z_PARAM_ARRAY(share_opts) ZEND_PARSE_PARAMETERS_END(); ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { @@ -192,10 +192,38 @@ PHP_FUNCTION(curl_share_init_persistent) } } ZEND_HASH_FOREACH_END(); - zend_array_sort(Z_ARRVAL_P(share_opts), php_array_data_compare_unstable_i, 1); - zend_update_property(curl_share_persistent_ce, Z_OBJ_P(return_value), "options", sizeof("options") - 1, share_opts); + // Construct a property field for the CurlSharePersistentHandle object with the enabled share options. + zval share_opts_prop; + array_init(&share_opts_prop); + + if (persistent_id & 1 << 0) { + add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_DNS); + } + + if (persistent_id & 1 << 1) { + add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_SSL_SESSION); + } + + if (persistent_id & 1 << 2) { + add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_CONNECT); + } + + if (persistent_id & 1 << 3) { + add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_PSL); + } object_init_ex(return_value, curl_share_persistent_ce); + + zend_update_property( + curl_share_persistent_ce, + Z_OBJ_P(return_value), + "options", + sizeof("options") - 1, + &share_opts_prop + ); + + zval_ptr_dtor(&share_opts_prop); + sh = Z_CURL_SHARE_P(return_value); if (persistent_id) { diff --git a/ext/curl/tests/curl_persistent_share_001.phpt b/ext/curl/tests/curl_persistent_share_001.phpt index ceaec5cc17aa..70da0d5880f7 100644 --- a/ext/curl/tests/curl_persistent_share_001.phpt +++ b/ext/curl/tests/curl_persistent_share_001.phpt @@ -19,10 +19,10 @@ function get_localhost_curl_handle(CurlSharePersistentHandle $sh): CurlHandle { return $ch; } -$sh1 = curl_share_init_persistent([CURL_LOCK_DATA_CONNECT, CURL_LOCK_DATA_DNS]); +$sh1 = curl_share_init_persistent([CURL_LOCK_DATA_CONNECT, CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_DNS]); $ch1 = get_localhost_curl_handle($sh1); -// Expect the two options we set above, but sorted. +// Expect the two options we set above, but sorted and de-duplicated. var_dump($sh1->options); $sh2 = curl_share_init_persistent([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT]); diff --git a/ext/standard/array.c b/ext/standard/array.c index db1ef1d09a65..a84fa50d61f0 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -283,7 +283,7 @@ static zend_always_inline int php_array_key_compare_string_locale_unstable_i(Buc } /* }}} */ -PHPAPI zend_always_inline int php_array_data_compare_unstable_i(Bucket *f, Bucket *s) /* {{{ */ +static zend_always_inline int php_array_data_compare_unstable_i(Bucket *f, Bucket *s) /* {{{ */ { int result = zend_compare(&f->val, &s->val); /* Special enums handling for array_unique. We don't want to add this logic to zend_compare as diff --git a/ext/standard/php_array.h b/ext/standard/php_array.h index 2c48ea8e5fb9..2a35af603808 100644 --- a/ext/standard/php_array.h +++ b/ext/standard/php_array.h @@ -28,7 +28,6 @@ PHP_MSHUTDOWN_FUNCTION(array); PHPAPI int php_array_merge(HashTable *dest, HashTable *src); PHPAPI int php_array_merge_recursive(HashTable *dest, HashTable *src); PHPAPI int php_array_replace_recursive(HashTable *dest, HashTable *src); -PHPAPI int php_array_data_compare_unstable_i(Bucket *f, Bucket *s); PHPAPI int php_multisort_compare(const void *a, const void *b); PHPAPI zend_long php_count_recursive(HashTable *ht); From ed9f339bc201b1249744af3606f99a4d43187b57 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Thu, 2 Jan 2025 15:44:23 -0500 Subject: [PATCH 09/17] update wording for `CURL_LOCK_DATA_COOKIE` error --- ext/curl/share.c | 2 +- ext/curl/tests/curl_persistent_share_004.phpt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 2aa30d13430e..cb7c80070045 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -168,7 +168,7 @@ PHP_FUNCTION(curl_share_init_persistent) switch (option) { // Disallowed options case CURL_LOCK_DATA_COOKIE: - zend_argument_value_error(1, "CURL_LOCK_DATA_COOKIE is not allowed"); + zend_argument_value_error(1, "must not contain CURL_LOCK_DATA_COOKIE because it is unsafe"); goto error; // Allowed options diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index af1d7924375e..06a9a7123e26 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -9,7 +9,7 @@ $sh = curl_share_init_persistent([CURL_LOCK_DATA_COOKIE]); ?> --EXPECTF-- -Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) CURL_LOCK_DATA_COOKIE is not allowed in %scurl_persistent_share_004.php:3 +Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because it is unsafe in %scurl_persistent_share_004.php:3 Stack trace: #0 %scurl_persistent_share_004.php(3): curl_share_init_persistent(Array) #1 {main} From c146ac7dd3ad7f59edeaa131de9134c5c3992fad Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Thu, 2 Jan 2025 17:30:29 -0500 Subject: [PATCH 10/17] address third round of PR comments --- ext/curl/share.c | 54 +++++++------------ ext/curl/tests/curl_persistent_share_004.phpt | 2 +- ext/curl/tests/curl_persistent_share_006.phpt | 16 ++++++ 3 files changed, 36 insertions(+), 36 deletions(-) create mode 100644 ext/curl/tests/curl_persistent_share_006.phpt diff --git a/ext/curl/share.c b/ext/curl/share.c index cb7c80070045..fdcf4557480a 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -154,6 +154,11 @@ PHP_FUNCTION(curl_share_init_persistent) Z_PARAM_ARRAY(share_opts) ZEND_PARSE_PARAMETERS_END(); + if (zend_hash_num_elements(Z_ARRVAL_P(share_opts)) == 0) { + zend_argument_value_error(1, "must not be empty"); + goto error; + } + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { ZVAL_DEREF(entry); @@ -168,7 +173,7 @@ PHP_FUNCTION(curl_share_init_persistent) switch (option) { // Disallowed options case CURL_LOCK_DATA_COOKIE: - zend_argument_value_error(1, "must not contain CURL_LOCK_DATA_COOKIE because it is unsafe"); + zend_argument_value_error(1, "must not contain CURL_LOCK_DATA_COOKIE because sharing cookies across PHP requests is unsafe"); goto error; // Allowed options @@ -196,44 +201,36 @@ PHP_FUNCTION(curl_share_init_persistent) zval share_opts_prop; array_init(&share_opts_prop); - if (persistent_id & 1 << 0) { + if (persistent_id & (1 << 0)) { add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_DNS); } - if (persistent_id & 1 << 1) { + if (persistent_id & (1 << 1)) { add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_SSL_SESSION); } - if (persistent_id & 1 << 2) { + if (persistent_id & (1 << 2)) { add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_CONNECT); } - if (persistent_id & 1 << 3) { + if (persistent_id & (1 << 3)) { add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_PSL); } object_init_ex(return_value, curl_share_persistent_ce); - zend_update_property( - curl_share_persistent_ce, - Z_OBJ_P(return_value), - "options", - sizeof("options") - 1, - &share_opts_prop - ); - + zend_update_property(curl_share_persistent_ce, Z_OBJ_P(return_value), ZEND_STRL("options"), &share_opts_prop); zval_ptr_dtor(&share_opts_prop); sh = Z_CURL_SHARE_P(return_value); - if (persistent_id) { - zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); + zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); - if (persisted) { - sh->share = Z_PTR_P(persisted); + // If we were able to find an existing persistent share handle, we can use it and return early. + if (persisted) { + sh->share = Z_PTR_P(persisted); - return; - } + return; } // We could not find an existing share handle, so we'll have to create one. @@ -246,27 +243,18 @@ PHP_FUNCTION(curl_share_init_persistent) error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(entry)); if (error != CURLSHE_OK) { - zend_throw_exception_ex( - NULL, - 0, - "Could not construct persistent cURL share handle: %s", - curl_share_strerror(error) - ); + zend_throw_exception_ex(NULL, 0, "Could not construct persistent cURL share handle: %s", curl_share_strerror(error)); goto error; } } ZEND_HASH_FOREACH_END(); - zend_hash_index_add_new_ptr( - &CURL_G(persistent_curlsh), - persistent_id, - sh->share - ); + zend_hash_index_add_new_ptr(&CURL_G(persistent_curlsh), persistent_id, sh->share); return; error: - if (sh && sh->share) { + if (sh) { curl_share_cleanup(sh->share); } @@ -282,10 +270,6 @@ void curl_share_free_persistent_curlsh(zval *data) { CURLSH *handle = Z_PTR_P(data); - if (!handle) { - return; - } - curl_share_cleanup(handle); } diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index 06a9a7123e26..f2d5592d9ac9 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -9,7 +9,7 @@ $sh = curl_share_init_persistent([CURL_LOCK_DATA_COOKIE]); ?> --EXPECTF-- -Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because it is unsafe in %scurl_persistent_share_004.php:3 +Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because sharing cookies across PHP requests is unsafe in %scurl_persistent_share_004.php:3 Stack trace: #0 %scurl_persistent_share_004.php(3): curl_share_init_persistent(Array) #1 {main} diff --git a/ext/curl/tests/curl_persistent_share_006.phpt b/ext/curl/tests/curl_persistent_share_006.phpt new file mode 100644 index 000000000000..e8ebf8f34a5c --- /dev/null +++ b/ext/curl/tests/curl_persistent_share_006.phpt @@ -0,0 +1,16 @@ +--TEST-- +Curl persistent share handles must be initialized with a non-empty $share_opts +--EXTENSIONS-- +curl +--FILE-- + +--EXPECTF-- +Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not be empty in %scurl_persistent_share_006.php:3 +Stack trace: +#0 %scurl_persistent_share_006.php(3): curl_share_init_persistent(Array) +#1 {main} + thrown in %scurl_persistent_share_006.php on line 3 From efda5c24f3424c9122e90dcd903161270cf7e8d2 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Thu, 2 Jan 2025 19:27:40 -0500 Subject: [PATCH 11/17] address fourth round of PR comments --- ext/curl/share.c | 33 +++++++++---------- ext/curl/tests/curl_persistent_share_003.phpt | 12 +++---- ext/curl/tests/curl_persistent_share_004.phpt | 12 +++---- ext/curl/tests/curl_persistent_share_005.phpt | 15 +++++---- ext/curl/tests/curl_persistent_share_006.phpt | 13 ++++---- 5 files changed, 44 insertions(+), 41 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index fdcf4557480a..1b3f93eb0f1f 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -21,7 +21,6 @@ #endif #include "php.h" -#include "ext/standard/php_array.h" #include "Zend/zend_exceptions.h" #include "curl_private.h" @@ -143,30 +142,30 @@ PHP_FUNCTION(curl_share_strerror) */ PHP_FUNCTION(curl_share_init_persistent) { - zval *share_opts = NULL, *entry = NULL; + HashTable *share_opts = NULL; + zval *share_opts_entry = NULL; + zend_ulong persistent_id = 0; php_curlsh *sh = NULL; - CURLSHcode error = 0; - ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_ARRAY(share_opts) + Z_PARAM_ARRAY_HT(share_opts) ZEND_PARSE_PARAMETERS_END(); - if (zend_hash_num_elements(Z_ARRVAL_P(share_opts)) == 0) { - zend_argument_value_error(1, "must not be empty"); + if (zend_hash_num_elements(share_opts) == 0) { + zend_argument_must_not_be_empty_error(1); goto error; } - ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { - ZVAL_DEREF(entry); + ZEND_HASH_FOREACH_VAL(share_opts, share_opts_entry) { + ZVAL_DEREF(share_opts_entry); - bool failed = false; - zend_ulong option = zval_try_get_long(entry, &failed); + bool failed = false; + zend_ulong option = zval_try_get_long(share_opts_entry, &failed); if (failed) { - zend_argument_type_error(1, "must contain only integer values, %s given", zend_zval_value_name(entry)); + zend_argument_type_error(1, "must contain only int values, %s given", zend_zval_value_name(share_opts_entry)); goto error; } @@ -237,13 +236,13 @@ PHP_FUNCTION(curl_share_init_persistent) sh->share = curl_share_init(); // Apply $share_options to the handle. - ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(share_opts), entry) { - ZVAL_DEREF(entry); + ZEND_HASH_FOREACH_VAL(share_opts, share_opts_entry) { + ZVAL_DEREF(share_opts_entry); - error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(entry)); + CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(share_opts_entry)); - if (error != CURLSHE_OK) { - zend_throw_exception_ex(NULL, 0, "Could not construct persistent cURL share handle: %s", curl_share_strerror(error)); + if (curlsh_error != CURLSHE_OK) { + zend_throw_exception_ex(NULL, 0, "Could not construct persistent cURL share handle: %s", curl_share_strerror(curlsh_error)); goto error; } diff --git a/ext/curl/tests/curl_persistent_share_003.phpt b/ext/curl/tests/curl_persistent_share_003.phpt index a64440c11883..8d92a56e3714 100644 --- a/ext/curl/tests/curl_persistent_share_003.phpt +++ b/ext/curl/tests/curl_persistent_share_003.phpt @@ -5,12 +5,12 @@ curl --FILE-- getMessage() . PHP_EOL; +} ?> --EXPECTF-- -Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must contain only CURL_LOCK_DATA_* constants in %scurl_persistent_share_003.php:3 -Stack trace: -#0 %scurl_persistent_share_003.php(3): curl_share_init_persistent(Array) -#1 {main} - thrown in %scurl_persistent_share_003.php on line 3 +curl_share_init_persistent(): Argument #1 ($share_options) must contain only CURL_LOCK_DATA_* constants diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index f2d5592d9ac9..06827370d76f 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -5,12 +5,12 @@ curl --FILE-- getMessage() . PHP_EOL; +} ?> --EXPECTF-- -Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because sharing cookies across PHP requests is unsafe in %scurl_persistent_share_004.php:3 -Stack trace: -#0 %scurl_persistent_share_004.php(3): curl_share_init_persistent(Array) -#1 {main} - thrown in %scurl_persistent_share_004.php on line 3 +curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because sharing cookies across PHP requests is unsafe diff --git a/ext/curl/tests/curl_persistent_share_005.phpt b/ext/curl/tests/curl_persistent_share_005.phpt index 9a9c4d90d117..9d529aa4d00a 100644 --- a/ext/curl/tests/curl_persistent_share_005.phpt +++ b/ext/curl/tests/curl_persistent_share_005.phpt @@ -6,12 +6,15 @@ curl getMessage() . PHP_EOL; +} + + ?> --EXPECTF-- -Fatal error: Uncaught TypeError: curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlSharePersistentHandle given in %scurl_persistent_share_005.php:4 -Stack trace: -#0 %scurl_persistent_share_005.php(4): curl_share_setopt(Object(CurlSharePersistentHandle), %d, %d) -#1 {main} - thrown in %scurl_persistent_share_005.php on line 4 +curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlSharePersistentHandle given diff --git a/ext/curl/tests/curl_persistent_share_006.phpt b/ext/curl/tests/curl_persistent_share_006.phpt index e8ebf8f34a5c..a882e9b93b00 100644 --- a/ext/curl/tests/curl_persistent_share_006.phpt +++ b/ext/curl/tests/curl_persistent_share_006.phpt @@ -5,12 +5,13 @@ curl --FILE-- getMessage() . PHP_EOL; +} + ?> --EXPECTF-- -Fatal error: Uncaught ValueError: curl_share_init_persistent(): Argument #1 ($share_options) must not be empty in %scurl_persistent_share_006.php:3 -Stack trace: -#0 %scurl_persistent_share_006.php(3): curl_share_init_persistent(Array) -#1 {main} - thrown in %scurl_persistent_share_006.php on line 3 +curl_share_init_persistent(): Argument #1 ($share_options) must not be empty From 4388ceafc7cbb893f2dfe1f700472e388b8e5c2f Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Fri, 3 Jan 2025 14:26:14 -0500 Subject: [PATCH 12/17] address fifth round of PR comments, fix typo --- ext/curl/share.c | 7 ++++--- ext/curl/tests/curl_persistent_share_002.phpt | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 1b3f93eb0f1f..1af86cda7d9c 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -235,11 +235,12 @@ PHP_FUNCTION(curl_share_init_persistent) // We could not find an existing share handle, so we'll have to create one. sh->share = curl_share_init(); - // Apply $share_options to the handle. - ZEND_HASH_FOREACH_VAL(share_opts, share_opts_entry) { + // Apply the options property to the handle. We avoid using $share_opts as zval_get_long may not necessarily return + // the same value as it did in the initial ZEND_HASH_FOREACH_VAL above. + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), share_opts_entry) { ZVAL_DEREF(share_opts_entry); - CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, zval_get_long(share_opts_entry)); + CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, Z_LVAL_P(share_opts_entry)); if (curlsh_error != CURLSHE_OK) { zend_throw_exception_ex(NULL, 0, "Could not construct persistent cURL share handle: %s", curl_share_strerror(curlsh_error)); diff --git a/ext/curl/tests/curl_persistent_share_002.phpt b/ext/curl/tests/curl_persistent_share_002.phpt index 05bd569c50c8..fbd6631b9ba1 100644 --- a/ext/curl/tests/curl_persistent_share_002.phpt +++ b/ext/curl/tests/curl_persistent_share_002.phpt @@ -29,7 +29,7 @@ $ch2 = get_localhost_curl_handle($sh2); var_dump($sh1->options != $sh2->options); -// Expect the connect time on the subsequent request to be non-zero, since it's reusing the connection. +// Expect the connect time on the subsequent request to be non-zero, since it's *not* reusing the connection. var_dump(curl_exec($ch1)); var_dump(curl_exec($ch2)); var_dump("second connect_time: " . (curl_getinfo($ch2)["connect_time"])); From 7db065ab89d9b074174f457a01578c04901e1c44 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Fri, 3 Jan 2025 14:41:16 -0500 Subject: [PATCH 13/17] remove unnecessary `ZVAL_DEREF` --- ext/curl/share.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 1af86cda7d9c..691fbd1b979d 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -238,8 +238,6 @@ PHP_FUNCTION(curl_share_init_persistent) // Apply the options property to the handle. We avoid using $share_opts as zval_get_long may not necessarily return // the same value as it did in the initial ZEND_HASH_FOREACH_VAL above. ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), share_opts_entry) { - ZVAL_DEREF(share_opts_entry); - CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, Z_LVAL_P(share_opts_entry)); if (curlsh_error != CURLSHE_OK) { From d45ad56da29002c13243a656d3ff6c2879d88576 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Fri, 3 Jan 2025 15:30:05 -0500 Subject: [PATCH 14/17] use optimized FOREACH --- ext/curl/share.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 691fbd1b979d..5762525ea8bf 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -237,7 +237,7 @@ PHP_FUNCTION(curl_share_init_persistent) // Apply the options property to the handle. We avoid using $share_opts as zval_get_long may not necessarily return // the same value as it did in the initial ZEND_HASH_FOREACH_VAL above. - ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), share_opts_entry) { + ZEND_HASH_PACKED_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), share_opts_entry) { CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, Z_LVAL_P(share_opts_entry)); if (curlsh_error != CURLSHE_OK) { From 1bb04e45d324e781f10fa7a86abb49000940c7d9 Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Sun, 5 Jan 2025 13:19:07 -0500 Subject: [PATCH 15/17] don't call `zval_ptr_dtor` until after we're finished with `share_opts_prop` also, only declare `zval *entry` where we need it. --- ext/curl/share.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/ext/curl/share.c b/ext/curl/share.c index 5762525ea8bf..8f3bb7543a20 100644 --- a/ext/curl/share.c +++ b/ext/curl/share.c @@ -142,9 +142,14 @@ PHP_FUNCTION(curl_share_strerror) */ PHP_FUNCTION(curl_share_init_persistent) { + // Options specified by the user. HashTable *share_opts = NULL; - zval *share_opts_entry = NULL; + // De-duplicated and sorted options. + zval share_opts_prop; + ZVAL_UNDEF(&share_opts_prop); + + // An ID representing the unique set of options specified by the user. zend_ulong persistent_id = 0; php_curlsh *sh = NULL; @@ -158,14 +163,14 @@ PHP_FUNCTION(curl_share_init_persistent) goto error; } - ZEND_HASH_FOREACH_VAL(share_opts, share_opts_entry) { - ZVAL_DEREF(share_opts_entry); + ZEND_HASH_FOREACH_VAL(share_opts, zval *entry) { + ZVAL_DEREF(entry); bool failed = false; - zend_ulong option = zval_try_get_long(share_opts_entry, &failed); + zend_ulong option = zval_try_get_long(entry, &failed); if (failed) { - zend_argument_type_error(1, "must contain only int values, %s given", zend_zval_value_name(share_opts_entry)); + zend_argument_type_error(1, "must contain only int values, %s given", zend_zval_value_name(entry)); goto error; } @@ -196,8 +201,10 @@ PHP_FUNCTION(curl_share_init_persistent) } } ZEND_HASH_FOREACH_END(); - // Construct a property field for the CurlSharePersistentHandle object with the enabled share options. - zval share_opts_prop; + // We're now decently confident that we'll be returning a CurlSharePersistentHandle object, so we construct it here. + object_init_ex(return_value, curl_share_persistent_ce); + + // Next we initialize a property field for the CurlSharePersistentHandle object with the enabled share options. array_init(&share_opts_prop); if (persistent_id & (1 << 0)) { @@ -216,20 +223,17 @@ PHP_FUNCTION(curl_share_init_persistent) add_next_index_long(&share_opts_prop, CURL_LOCK_DATA_PSL); } - object_init_ex(return_value, curl_share_persistent_ce); - zend_update_property(curl_share_persistent_ce, Z_OBJ_P(return_value), ZEND_STRL("options"), &share_opts_prop); - zval_ptr_dtor(&share_opts_prop); sh = Z_CURL_SHARE_P(return_value); + // If we are able to find an existing persistent share handle, we can use it and return early. zval *persisted = zend_hash_index_find(&CURL_G(persistent_curlsh), persistent_id); - // If we were able to find an existing persistent share handle, we can use it and return early. if (persisted) { sh->share = Z_PTR_P(persisted); - return; + goto cleanup; } // We could not find an existing share handle, so we'll have to create one. @@ -237,8 +241,8 @@ PHP_FUNCTION(curl_share_init_persistent) // Apply the options property to the handle. We avoid using $share_opts as zval_get_long may not necessarily return // the same value as it did in the initial ZEND_HASH_FOREACH_VAL above. - ZEND_HASH_PACKED_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), share_opts_entry) { - CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, Z_LVAL_P(share_opts_entry)); + ZEND_HASH_PACKED_FOREACH_VAL(Z_ARRVAL_P(&share_opts_prop), zval *entry) { + CURLSHcode curlsh_error = curl_share_setopt(sh->share, CURLSHOPT_SHARE, Z_LVAL_P(entry)); if (curlsh_error != CURLSHE_OK) { zend_throw_exception_ex(NULL, 0, "Could not construct persistent cURL share handle: %s", curl_share_strerror(curlsh_error)); @@ -249,6 +253,9 @@ PHP_FUNCTION(curl_share_init_persistent) zend_hash_index_add_new_ptr(&CURL_G(persistent_curlsh), persistent_id, sh->share); + cleanup: + zval_ptr_dtor(&share_opts_prop); + return; error: @@ -256,6 +263,8 @@ PHP_FUNCTION(curl_share_init_persistent) curl_share_cleanup(sh->share); } + zval_ptr_dtor(&share_opts_prop); + RETURN_THROWS(); } From 32cf6f6144a77ea906a4ab282ade90da261cbded Mon Sep 17 00:00:00 2001 From: Eric Norris Date: Sun, 5 Jan 2025 23:23:43 -0500 Subject: [PATCH 16/17] fix tabs in tests, use expect instead of expectf --- ext/curl/tests/curl_persistent_share_003.phpt | 4 ++-- ext/curl/tests/curl_persistent_share_004.phpt | 4 ++-- ext/curl/tests/curl_persistent_share_005.phpt | 6 ++---- ext/curl/tests/curl_persistent_share_006.phpt | 5 ++--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/ext/curl/tests/curl_persistent_share_003.phpt b/ext/curl/tests/curl_persistent_share_003.phpt index 8d92a56e3714..60a30de4d464 100644 --- a/ext/curl/tests/curl_persistent_share_003.phpt +++ b/ext/curl/tests/curl_persistent_share_003.phpt @@ -8,9 +8,9 @@ curl try { $sh = curl_share_init_persistent([CURL_LOCK_DATA_DNS, CURL_LOCK_DATA_CONNECT, 30]); } catch (\ValueError $e) { - echo $e->getMessage() . PHP_EOL; + echo $e->getMessage() . PHP_EOL; } ?> ---EXPECTF-- +--EXPECT-- curl_share_init_persistent(): Argument #1 ($share_options) must contain only CURL_LOCK_DATA_* constants diff --git a/ext/curl/tests/curl_persistent_share_004.phpt b/ext/curl/tests/curl_persistent_share_004.phpt index 06827370d76f..fe0e7ec2cf0e 100644 --- a/ext/curl/tests/curl_persistent_share_004.phpt +++ b/ext/curl/tests/curl_persistent_share_004.phpt @@ -8,9 +8,9 @@ curl try { $sh = curl_share_init_persistent([CURL_LOCK_DATA_COOKIE]); } catch (\ValueError $e) { - echo $e->getMessage() . PHP_EOL; + echo $e->getMessage() . PHP_EOL; } ?> ---EXPECTF-- +--EXPECT-- curl_share_init_persistent(): Argument #1 ($share_options) must not contain CURL_LOCK_DATA_COOKIE because sharing cookies across PHP requests is unsafe diff --git a/ext/curl/tests/curl_persistent_share_005.phpt b/ext/curl/tests/curl_persistent_share_005.phpt index 9d529aa4d00a..8737ba6c39a4 100644 --- a/ext/curl/tests/curl_persistent_share_005.phpt +++ b/ext/curl/tests/curl_persistent_share_005.phpt @@ -10,11 +10,9 @@ $sh = curl_share_init_persistent([CURL_LOCK_DATA_DNS]); try { curl_share_setopt($sh, CURLOPT_SHARE, CURL_LOCK_DATA_CONNECT); } catch (\TypeError $e) { - echo $e->getMessage() . PHP_EOL; + echo $e->getMessage() . PHP_EOL; } - - ?> ---EXPECTF-- +--EXPECT-- curl_share_setopt(): Argument #1 ($share_handle) must be of type CurlShareHandle, CurlSharePersistentHandle given diff --git a/ext/curl/tests/curl_persistent_share_006.phpt b/ext/curl/tests/curl_persistent_share_006.phpt index a882e9b93b00..ffca764757fb 100644 --- a/ext/curl/tests/curl_persistent_share_006.phpt +++ b/ext/curl/tests/curl_persistent_share_006.phpt @@ -8,10 +8,9 @@ curl try { $sh = curl_share_init_persistent([]); } catch (\ValueError $e) { - echo $e->getMessage() . PHP_EOL; + echo $e->getMessage() . PHP_EOL; } - ?> ---EXPECTF-- +--EXPECT-- curl_share_init_persistent(): Argument #1 ($share_options) must not be empty From 3fed2026fdf9b00b0a19f8e7c9bd49b368b602b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Mon, 6 Jan 2025 21:00:35 +0100 Subject: [PATCH 17/17] NEWS/UPGRADING --- NEWS | 1 + UPGRADING | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/NEWS b/NEWS index 864a6b2e189d..e0e0d1d98ae5 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,7 @@ PHP NEWS - Curl: . Added curl_multi_get_handles(). (timwolla) + . Added curl_share_init_persistent(). (enorris) - Date: . Fix undefined behaviour problems regarding integer overflow in extreme edge diff --git a/UPGRADING b/UPGRADING index 63307556848d..275c34a345c2 100644 --- a/UPGRADING +++ b/UPGRADING @@ -68,6 +68,11 @@ PHP 8.5 UPGRADE NOTES . Added support for Closures in constant expressions. RFC: https://wiki.php.net/rfc/closures_in_const_expr +- Curl: + . Added support for share handles that are persisted across multiple PHP + requests, safely allowing for more effective connection reuse. + RFC: https://wiki.php.net/rfc/curl_share_persistence_improvement + - DOM: . Added Dom\Element::$outerHTML. @@ -148,6 +153,9 @@ PHP 8.5 UPGRADE NOTES . curl_multi_get_handles() allows retrieving all CurlHandles current attached to a CurlMultiHandle. This includes both handles added using curl_multi_add_handle() and handles accepted by CURLMOPT_PUSHFUNCTION. + . curl_share_init_persistent() allows creating a share handle that is + persisted across multiple PHP requests. + RFC: https://wiki.php.net/rfc/curl_share_persistence_improvement - DOM: . Added Dom\Element::insertAdjacentHTML(). @@ -166,6 +174,11 @@ PHP 8.5 UPGRADE NOTES 7. New Classes and Interfaces ======================================== +- Curl: + . CurlSharePersistentHandle representing a share handle that is persisted + across multiple PHP requests. + RFC: https://wiki.php.net/rfc/curl_share_persistence_improvement + ======================================== 8. Removed Extensions and SAPIs ========================================