Am 12.08.2015 um 13:17 schrieb Andreas Heigl:
Hi Rainer.
Am 12.08.2015 um 13:00 schrieb Rainer Jung <rainer.j...@kippdata.de>:
Hi Côme,
Am 11.08.2015 um 16:58 schrieb Côme BERNIGAUD:
On 2015-08-11 00:36, Rainer Jung wrote:
The current problems should be mostly around the above four compiler
warnings. I can test any patches you want me to test.
Can you test including the attached file in ext/ldap/ldap.c, and not
defining HAVE_3ARG_SETREBINDPROC and LDAP_CONTROL_PAGEDRESULTS ?
thanks for the patch proposal. There's a few problems with it and I will come
back to you with an alternative proposal soon. For the moment:
- the definition of ldap_control_find in terms of at the top of ldap.c
currently is protected by
#if !defined(HAVE_LDAP_CONTROL_FIND)
It should be
#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)
That's not related to your patch.
- in
void ldap_memvfree( void **value )
{
ldap_value_free(value);
}
it should probably be
ldap_value_free((char **)value);
otherwise we get compiler warnings.
- instead of ldap_open() we can use ldap_init() (recommended).
- using ldap_open() or ldap_init() with a url does not work, we have to use
host and port.
- the use of ldap_sasl_bind_s() in PHP_FUNCTION(ldap_bind) does not work, without setting
"LDAP_OPT_PROTOCOL_VERSION" to LDAP_VERSION3 using ldap_set_option(). Maybe it
works for OpenLDAP, but Netscape and Solaris document it must be set and indeed return
LDAP_NOT_SUPPORTED immediately otherwise. But since in PHP_FUNCTION(ldap_bind) no SASL
functionality is actually being used, I propose to use ldap_simple_bind_s() instead of
ldap_sasl_bind_s(). It is not deprecated and using it actually simplifies the code a bit.
As I said, I'll come up with a patch proposal soon.
The patch should also help for other LDAP implementations than OpenLDAP or
Solaris.
The main question here is whether we want PHP to support other implementations.
The documentation clearly states that OpenLDAP shall be used and even Oracle
states on an official site (I don't actually have the URL at hand but it has
been in an email to this thread) that PHPs LDAP extension has to be built
against OpenLDAP.
That's you (project) call. You are doing the work, you decide. I
wouldn't rely on Oracle for a judgement ;)
Also note that the code and m4 macros have quite a few places where they
react on other LDAP SDKs, like Windows, Oracle (not sun) and I think
also Netware. But those are not really strong arguments for multi-SDK
support. You can purge those places and the argument is gone. I really
think it depends on your goals.
But you should consider 5.6 a stable release. Maybe I'm the only guy on
the planet haven't build the ldap extension against non-OpenLDAP, but I
doubt it. 7.0 is in front of the doors and compatibility is much less an
issue there, maybe combined with a NEWS entry strengthening, that you
now really demand OpenLDAP and throw an error, if you can't find it
during configure.
Come has done a great job to clean up the LDAP code to start implementing long
awaited features and I'm not really sure whether we should give that up for
maintaining compatibility with some edge-case usages. So I'm very interested in
your patch to see how we can rech both goals!
Agreed, and much of the cleanup wouldn't have to be undone to make the
result still compatible with more SDKs. For instance all of the new use
of the "_ext" variants of functions is nice and OK also for other LDAP
implementations.
I'll attach my current patch. See below for some comments on the patch.
In the meantime you can still use the "old" ext/ldap for the newer PHP-branch as it
should compile without an issue. The "newer" ext/ldap just removed stuff ;)
Sure, that's the workaround I would use if you decide to require OpenLDAP.
Concerning the patch:
- I switched from
#ifndef HAVE_LDAP_CONTROL_FIND
to
#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)
because the contents of the ifndef-block are only used in the
LDAP_CONTROL_PAGEDRESULTS case and the block needs ldap_find_control,
which isn't available in the general case.
- I added a definition of ldap_memvfree() in terms of ldap_value_free()
similar to Come's proposal, but surrounded it by #if
!defined(LDAP_API_FEATURE_X_OPENLDAP). This define is set in OpenLDAP
since about 1998.
- I replaced all
#if defined(HAVE_3ARG_SETREBINDPROC)
with
#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
Reasoning: most LDAP impls have a 3 arg ldap_set_rebind_proc(), but the
callback function argument has no standardized signature. The different
impls use different arguments in the callback and using them would need
different code. You can't simply abstract away the differences. So for
now the existing code only makes definite sense for OpenLDAP. For other
SDKs it might need to vary.
- I protected the use of ldap_initialize() by
#ifdef LDAP_API_FEATURE_X_OPENLDAP
and switch back to ldap_init(host, port) with a slightly different type
of success check for other impls. The original idea of Come to define
ldap_initialize(&ldap, url) in terms of ldap_init(host, port) does not
work, because ldap_init() doesn't take URLs.
- I protected the use of ldap_sasl_bind_s(..., ..., LDAP_SASL_SIMPLE,
...) by
#ifdef LDAP_API_FEATURE_X_OPENLDAP
and switch back to ldap_simple_bind_s() for the other impls. This is
because the sasl call for instance for the Solaris impl fails, if you do
not explicitly set the protocol version on ld->link to LDAP_VERSION3
using ldap_set_option(). Other impls have different behavior. Since we
here only want to do a simple bind, I used the old style call in the non
OpenLDAP case.
All of these changes should also be OK for other LDAP impls than
OpenLDAP and Solaris. I don't see reasons, why they currently shouldn't
be able to use the patched code without further adjustments.
Regards,
Rainer
Index: ext/ldap/ldap.c
--- ext/ldap/ldap.c 2015-08-06 09:55:57.000000000 +0200
+++ ext/ldap/ldap.c 2015-08-13 14:12:07.390709000 +0200
@@ -70,17 +70,24 @@
#define PHP_LDAP_ESCAPE_FILTER 0x01
#define PHP_LDAP_ESCAPE_DN 0x02
-#ifndef HAVE_LDAP_CONTROL_FIND
+#if defined(LDAP_CONTROL_PAGEDRESULTS) && !defined(HAVE_LDAP_CONTROL_FIND)
LDAPControl *ldap_control_find( const char *oid, LDAPControl **ctrls, LDAPControl ***nextctrlp)
{
- assert(nextctrlp == NULL);
- return ldap_find_control(oid, ctrls);
+ assert(nextctrlp == NULL);
+ return ldap_find_control(oid, ctrls);
+}
+#endif
+
+#if !defined(LDAP_API_FEATURE_X_OPENLDAP)
+void ldap_memvfree(void **v)
+{
+ ldap_value_free((char **)v);
}
#endif
typedef struct {
LDAP *link;
-#if defined(HAVE_3ARG_SETREBINDPROC)
+#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
zval *rebindproc;
#endif
} ldap_linkdata;
@@ -104,10 +111,8 @@
{
ldap_linkdata *ld = (ldap_linkdata *)rsrc->ptr;
- /* ldap_unbind_s() is deprecated;
- * the distinction between ldap_unbind() and ldap_unbind_s() is moot */
ldap_unbind_ext(ld->link, NULL, NULL);
-#ifdef HAVE_3ARG_SETREBINDPROC
+#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
if (ld->rebindproc != NULL) {
zval_dtor(ld->rebindproc);
@@ -349,11 +354,8 @@
ld = ecalloc(1, sizeof(ldap_linkdata));
- /* OpenLDAP provides a specific call to detect valid LDAP URIs;
- * ldap_init()/ldap_open() is deprecated, use ldap_initialize() instead.
- */
{
- int rc;
+ int rc = LDAP_SUCCESS;
char *url = host;
if (!ldap_is_ldap_url(url)) {
int urllen = hostlen + sizeof( "ldap://:65535"; );
@@ -367,7 +369,21 @@
snprintf( url, urllen, "ldap://%s:%ld";, host ? host : "", port );
}
+#ifdef LDAP_API_FEATURE_X_OPENLDAP
+ /* ldap_init() is deprecated, use ldap_initialize() instead.
+ */
rc = ldap_initialize(&ldap, url);
+#else /* ! LDAP_API_FEATURE_X_OPENLDAP */
+ /* ldap_init does not support URLs.
+ * We must try the original host and port information.
+ */
+ ldap = ldap_init(host, port);
+ if (ldap == NULL) {
+ efree(ld);
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Could not create session handle");
+ RETURN_FALSE;
+ }
+#endif /* ! LDAP_API_FEATURE_X_OPENLDAP */
if (url != host) {
efree(url);
}
@@ -465,14 +481,19 @@
}
{
+#ifdef LDAP_API_FEATURE_X_OPENLDAP
+ /* ldap_simple_bind_s() is deprecated, use ldap_sasl_bind_s() instead.
+ */
struct berval cred;
- /* ldap_bind_s() is deprecated; use ldap_sasl_bind_s() instead */
cred.bv_val = ldap_bind_pw;
cred.bv_len = ldap_bind_pw ? ldap_bind_pwlen : 0;
rc = ldap_sasl_bind_s(ld->link, ldap_bind_dn, LDAP_SASL_SIMPLE, &cred,
NULL, NULL, /* no controls right now */
NULL); /* we don't care about the server's credentials */
+#else /* ! LDAP_API_FEATURE_X_OPENLDAP */
+ rc = ldap_simple_bind_s(ld->link, ldap_bind_dn, ldap_bind_pw);
+#endif /* ! LDAP_API_FEATURE_X_OPENLDAP */
}
if ( rc != LDAP_SUCCESS) {
php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to bind to server: %s", ldap_err2string(rc));
@@ -1304,7 +1325,6 @@
add_index_string(return_value, i, ldap_value[i], 1);
}
- /* ldap_value_free() is deprecated */
ldap_memvfree((void **)ldap_value);
}
/* }}} */
@@ -2485,7 +2505,7 @@
#endif
#endif /* (LDAP_API_VERSION > 2000) || HAVE_NSLDAP || HAVE_ORALDAP */
-#if defined(HAVE_3ARG_SETREBINDPROC)
+#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
/* {{{ _ldap_rebind_proc()
*/
int _ldap_rebind_proc(LDAP *ldap, const char *url, ber_tag_t req, ber_int_t msgid, void *params)
@@ -3138,7 +3158,7 @@
#endif
#endif
-#if defined(HAVE_3ARG_SETREBINDPROC)
+#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
ZEND_BEGIN_ARG_INFO_EX(arginfo_ldap_set_rebind_proc, 0, 0, 2)
ZEND_ARG_INFO(0, link)
ZEND_ARG_INFO(0, callback)
@@ -3226,7 +3246,7 @@
#endif
#endif
-#if defined(HAVE_3ARG_SETREBINDPROC)
+#if defined(LDAP_API_FEATURE_X_OPENLDAP) && defined(HAVE_3ARG_SETREBINDPROC)
PHP_FE(ldap_set_rebind_proc, arginfo_ldap_set_rebind_proc)
#endif
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
