ext/ldap: Use FCC for rebind_proc callback #17369
Conversation
There was a problem hiding this comment.
Apparently, to test this we would need two LDAP servers (unless you can refer to the same server again?)
I learned what this method does via https://www.ibm.com/docs/da/zos/2.4.0?topic=routines-ldap-set-rebind-proc
I'm also a bit uneasy about changing code that we didn't test... It looks more or less right, but experience also tells me that users report regressions waaaay later than when they're initially introduced
| } | ||
|
|
||
| $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.
The version option seems pointless?
There was a problem hiding this comment.
I took this from the user notes of https://www.php.net/manual/en/function.ldap-set-rebind-proc.php
| @@ -0,0 +1,63 @@ | |||
| --TEST-- | |||
| ldap_set_rebind_proc() with a trampoline | |||
There was a problem hiding this comment.
Right, the test seems kinda pointless, but it would also be a shame to get rid of this effort...
ext/ldap/ldap.c
Outdated
| /* 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.
You don't need this copy, zend_fcc_dtor already does this for you.
There was a problem hiding this comment.
Sometimes I forget that I design APIs in a reasonable way
ext/ldap/ldap.c
Outdated
| 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.
Wait, are you sure this is actually necessary?
There was a problem hiding this comment.
I don't see obvious mistakes.
Please add a comment to the test you added such that people won't be confused why nothing is being called.
Also a bit uneasy that this can't be tested, but I hope that there's enough time between now and the official release that issues will get noticed if there are any.
|
Please check the failures in nightly. https://github.com/php/php-src/actions/runs/12682222932/job/35347262817 |
| /* register rebind procedure */ | ||
| if (Z_ISUNDEF(ld->rebindproc)) { | ||
| /* Free old FCC */ | ||
| if (!ZEND_FCC_INITIALIZED(ld->rebind_proc_fcc)) { |
There was a problem hiding this comment.
This is probably the nightly issue: the condition seems inverted.
The tests seem to be somewhat pointless, as they don't actually trigger the callback.
I don't know enough about LDAP (nor know how to set up a server) to improve them.