-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/ldap: Use FCC for rebind_proc callback #17369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,19 +3713,20 @@ 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; | ||
} | ||
|
||
/* 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this copy, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes I forget that I design APIs in a reasonable way |
||
} | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, are you sure this is actually necessary? |
||
} 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; | ||
} | ||
/* }}} */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,20 +14,27 @@ require_once('skipifbindfailure.inc'); | |
<?php | ||
require "connect.inc"; | ||
|
||
function rebind_proc ($ds, $ldap_url) { | ||
global $user; | ||
global $passwd; | ||
global $protocol_version; | ||
function rebind_proc ($ldap, $referral) { | ||
global $user; | ||
global $passwd; | ||
global $protocol_version; | ||
|
||
// required by most modern LDAP servers, use LDAPv3 | ||
ldap_set_option($a, LDAP_OPT_PROTOCOL_VERSION, $protocol_version); | ||
// required by most modern LDAP servers, use LDAPv3 | ||
ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, $protocol_version); | ||
|
||
if (!ldap_bind($a, $user, $passwd)) { | ||
if (!ldap_bind($ldap, $user, $passwd)) { | ||
// Failure | ||
print "Cannot bind"; | ||
} | ||
return 1; | ||
} | ||
// Success | ||
return 0; | ||
} | ||
|
||
$link = ldap_connect_and_bind($uri, $user, $passwd, $protocol_version); | ||
ldap_set_option($link, LDAP_OPT_PROTOCOL_VERSION, 3); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version option seems pointless? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took this from the user notes of https://www.php.net/manual/en/function.ldap-set-rebind-proc.php There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay sure, can't harm I guess. |
||
ldap_set_option($link, LDAP_OPT_REFERRALS, true); | ||
|
||
var_dump(ldap_set_rebind_proc($link, "rebind_proc")); | ||
var_dump(ldap_set_rebind_proc($link, null)); | ||
?> | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
--TEST-- | ||
ldap_set_rebind_proc() with a trampoline | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the test seems kinda pointless, but it would also be a shame to get rid of this effort... |
||
--EXTENSIONS-- | ||
ldap | ||
--SKIPIF-- | ||
<?php | ||
if (!function_exists('ldap_set_rebind_proc')) die("skip ldap_set_rebind_proc() not available"); | ||
require_once('skipifbindfailure.inc'); | ||
?> | ||
--FILE-- | ||
<?php | ||
require "connect.inc"; | ||
|
||
class TrampolineTest { | ||
public function __construct(private $user, private $password, private $protocol_version) {} | ||
public function __call(string $name, array $arguments) { | ||
echo 'Trampoline for ', $name, PHP_EOL; | ||
var_dump(count($arguments)); | ||
if ($name === 'trampolineThrow') { | ||
throw new Exception('boo'); | ||
} | ||
if ($name === 'trampolineWrongType') { | ||
return ['not an int']; | ||
} | ||
// required by most modern LDAP servers, use LDAPv3 | ||
ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, $this->$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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the nightly issue: the condition seems inverted.