From 02bfb7ab449c2e78fc59e78c8aea7bd73a7073bc Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Sun, 5 Jan 2025 18:54:24 +0000 Subject: [PATCH 1/3] ext/ldap: Use FCC for rebind_proc callback --- ext/ldap/ldap.c | 52 ++++++++------- .../tests/ldap_set_rebind_proc_basic.phpt | 23 ++++--- .../tests/ldap_set_rebind_proc_error.phpt | 44 ------------- .../ldap_set_rebind_proc_trampoline.phpt | 63 +++++++++++++++++++ 4 files changed, 107 insertions(+), 75 deletions(-) delete mode 100644 ext/ldap/tests/ldap_set_rebind_proc_error.phpt create mode 100644 ext/ldap/tests/ldap_set_rebind_proc_trampoline.phpt diff --git a/ext/ldap/ldap.c b/ext/ldap/ldap.c index f13cc71248998..89ef936e3e486 100644 --- a/ext/ldap/ldap.c +++ b/ext/ldap/ldap.c @@ -73,7 +73,7 @@ void ldap_memvfree(void **v) typedef struct { LDAP *link; #if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC) - zval rebindproc; + zend_fcall_info_cache rebind_proc_fcc; #endif zend_object std; } ldap_linkdata; @@ -131,7 +131,9 @@ static void ldap_link_free(ldap_linkdata *ld) ld->link = NULL; #if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC) - zval_ptr_dtor(&ld->rebindproc); + if (ZEND_FCC_INITIALIZED(ld->rebind_proc_fcc)) { + zend_fcc_dtor(&ld->rebind_proc_fcc); + } #endif LDAPG(num_links)--; @@ -3711,7 +3713,7 @@ int _ldap_rebind_proc(LDAP *ldap, const char *url, ber_tag_t req, ber_int_t msgi } /* link exists and callback set? */ - if (Z_ISUNDEF(ld->rebindproc)) { + if (!ZEND_FCC_INITIALIZED(ld->rebind_proc_fcc)) { php_error_docref(NULL, E_WARNING, "No callback set"); return LDAP_OTHER; } @@ -3719,11 +3721,12 @@ int _ldap_rebind_proc(LDAP *ldap, const char *url, ber_tag_t req, ber_int_t msgi /* callback */ ZVAL_COPY_VALUE(&cb_args[0], cb_link); ZVAL_STRING(&cb_args[1], url); - if (call_user_function(EG(function_table), NULL, &ld->rebindproc, &cb_retval, 2, cb_args) == SUCCESS && !Z_ISUNDEF(cb_retval)) { + zend_call_known_fcc(&ld->rebind_proc_fcc, &cb_retval, 2, cb_args, NULL); + if (EXPECTED(!Z_ISUNDEF(cb_retval))) { + // TODO Use zval_try_get_long() retval = zval_get_long(&cb_retval); zval_ptr_dtor(&cb_retval); } else { - php_error_docref(NULL, E_WARNING, "rebind_proc PHP callback failed"); retval = LDAP_OTHER; } zval_ptr_dtor(&cb_args[1]); @@ -3735,35 +3738,38 @@ int _ldap_rebind_proc(LDAP *ldap, const char *url, ber_tag_t req, ber_int_t msgi PHP_FUNCTION(ldap_set_rebind_proc) { zval *link; - zend_fcall_info fci; - zend_fcall_info_cache fcc; + zend_fcall_info dummy_fci; + zend_fcall_info_cache fcc = empty_fcall_info_cache; ldap_linkdata *ld; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "Of!", &link, ldap_link_ce, &fci, &fcc) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS(), "OF!", &link, ldap_link_ce, &dummy_fci, &fcc) == FAILURE) { RETURN_THROWS(); } ld = Z_LDAP_LINK_P(link); - VERIFY_LDAP_LINK_CONNECTED(ld); - - if (!ZEND_FCI_INITIALIZED(fci)) { - /* unregister rebind procedure */ - if (!Z_ISUNDEF(ld->rebindproc)) { - zval_ptr_dtor(&ld->rebindproc); - ZVAL_UNDEF(&ld->rebindproc); - ldap_set_rebind_proc(ld->link, NULL, NULL); - } - RETURN_TRUE; + /* Inline VERIFY_LDAP_LINK_CONNECTED(ld); as we need to free trampoline */ + if (!ld->link) { + zend_release_fcall_info_cache(&fcc); + zend_throw_error(NULL, "LDAP connection has already been closed"); + RETURN_THROWS(); } - /* register rebind procedure */ - if (Z_ISUNDEF(ld->rebindproc)) { + /* Free old FCC */ + if (!ZEND_FCC_INITIALIZED(ld->rebind_proc_fcc)) { + zend_fcc_dtor(&ld->rebind_proc_fcc); + memcpy(&ld->rebind_proc_fcc, &empty_fcall_info_cache, sizeof(zend_fcall_info_cache)); + } + if (ZEND_FCC_INITIALIZED(fcc)) { + /* register rebind procedure */ ldap_set_rebind_proc(ld->link, _ldap_rebind_proc, (void *) link); + zend_fcc_dup(&ld->rebind_proc_fcc, &fcc); + /* Clear potential trampoline */ + zend_release_fcall_info_cache(&fcc); } else { - zval_ptr_dtor(&ld->rebindproc); - } + /* unregister rebind procedure */ + ldap_set_rebind_proc(ld->link, NULL, NULL); + } - ZVAL_COPY(&ld->rebindproc, &fci.function_name); RETURN_TRUE; } /* }}} */ diff --git a/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt b/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt index a328912a5e168..9f32775423559 100644 --- a/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt +++ b/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt @@ -14,20 +14,27 @@ require_once('skipifbindfailure.inc'); diff --git a/ext/ldap/tests/ldap_set_rebind_proc_error.phpt b/ext/ldap/tests/ldap_set_rebind_proc_error.phpt deleted file mode 100644 index 9a4fdd5cbb2ec..0000000000000 --- a/ext/ldap/tests/ldap_set_rebind_proc_error.phpt +++ /dev/null @@ -1,44 +0,0 @@ ---TEST-- -ldap_set_rebind_proc() - Testing ldap_set_rebind_proc() that should fail ---CREDITS-- -Patrick Allaert -# Belgian PHP Testfest 2009 ---EXTENSIONS-- -ldap ---SKIPIF-- - ---FILE-- -getMessage(), "\n"; -} -?> ---EXPECT-- -ldap_set_rebind_proc(): Argument #2 ($callback) must be a valid callback or null, function "rebind_proc_inexistent" not found or invalid function name diff --git a/ext/ldap/tests/ldap_set_rebind_proc_trampoline.phpt b/ext/ldap/tests/ldap_set_rebind_proc_trampoline.phpt new file mode 100644 index 0000000000000..ebc14f7a85b2f --- /dev/null +++ b/ext/ldap/tests/ldap_set_rebind_proc_trampoline.phpt @@ -0,0 +1,63 @@ +--TEST-- +ldap_set_rebind_proc() with a trampoline +--EXTENSIONS-- +ldap +--SKIPIF-- + +--FILE-- +$protocol_version); + if (!ldap_bind($ldap, $this->$user, $this->$password)) { + // Failure + print "Cannot bind"; + return 1; + } + // Success + return 0; + } +} +$o = new TrampolineTest($user, $passwd, $protocol_version); +$callback = [$o, 'trampoline']; +$callbackThrow = [$o, 'trampolineThrow']; +$callbackWrongType = [$o, 'trampolineWrongType']; + +$link = ldap_connect_and_bind($uri, $user, $passwd, $protocol_version); +var_dump(ldap_set_rebind_proc($link, $callback)); +var_dump(ldap_set_rebind_proc($link, null)); +var_dump(ldap_set_rebind_proc($link, $callbackThrow)); +var_dump(ldap_set_rebind_proc($link, null)); +var_dump(ldap_set_rebind_proc($link, $callbackWrongType)); + +var_dump(ldap_unbind($link)); +try { + var_dump(ldap_set_rebind_proc($link, $callback)); +} catch (Throwable $e) { + echo $e::class, ': ', $e->getMessage(), PHP_EOL; +} + +?> +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +Error: LDAP connection has already been closed From 5963eb3d334531f0a214ba9512d41f251016ffa4 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 8 Jan 2025 19:04:27 +0000 Subject: [PATCH 2/3] Review comments --- ext/ldap/ldap.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/ext/ldap/ldap.c b/ext/ldap/ldap.c index 89ef936e3e486..24292b1d61368 100644 --- a/ext/ldap/ldap.c +++ b/ext/ldap/ldap.c @@ -3757,14 +3757,11 @@ PHP_FUNCTION(ldap_set_rebind_proc) /* Free old FCC */ if (!ZEND_FCC_INITIALIZED(ld->rebind_proc_fcc)) { zend_fcc_dtor(&ld->rebind_proc_fcc); - memcpy(&ld->rebind_proc_fcc, &empty_fcall_info_cache, sizeof(zend_fcall_info_cache)); } if (ZEND_FCC_INITIALIZED(fcc)) { /* register rebind procedure */ ldap_set_rebind_proc(ld->link, _ldap_rebind_proc, (void *) link); zend_fcc_dup(&ld->rebind_proc_fcc, &fcc); - /* Clear potential trampoline */ - zend_release_fcall_info_cache(&fcc); } else { /* unregister rebind procedure */ ldap_set_rebind_proc(ld->link, NULL, NULL); From bbed1002d944bc34b52681202e1e3029cc325ac2 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 8 Jan 2025 20:52:08 +0000 Subject: [PATCH 3/3] [skip ci] Add comments to tests --- ext/ldap/tests/ldap_set_rebind_proc_basic.phpt | 2 ++ ext/ldap/tests/ldap_set_rebind_proc_trampoline.phpt | 2 ++ 2 files changed, 4 insertions(+) diff --git a/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt b/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt index 9f32775423559..95c9298309eef 100644 --- a/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt +++ b/ext/ldap/tests/ldap_set_rebind_proc_basic.phpt @@ -12,6 +12,8 @@ require_once('skipifbindfailure.inc'); ?> --FILE-- --FILE--