Moved the ldap connection timeout call (was: Re: svn commit: r160707 )

[Brad Nicholes <BN...@novell.com>]

  I just checked in a patch to move the ldap_set_option() call from trying to set the connection timeout globally to ldap connection specific.  I don't have a good way to test that it fixes the problem, but if somebody can verify the fix, I will propose it for backport.

Brad

>>> chip@force-elite.com Tuesday, April 26, 2005 10:11:34 AM >>>
bnicholes@apache.org wrote:
> Author: bnicholes
> Date: Sat Apr  9 12:00:18 2005
> New Revision: 160707
> 
> URL: http://svn.apache.org/viewcvs?view=rev&rev=160707 
> Log:
> Added a new LDAPConnectionTimeout directive to util_ldap so that the socket connection timeout value is configurable.
> 
> Reviewed by: bnicholes, trawick, jim
....
> @@ -1379,6 +1404,7 @@
>  
>      void *data;
>      const char *userdata_key = "util_ldap_init";
> +    struct timeval timeOut = {10,0};    /* 10 second connection timeout */
>  
>      /* util_ldap_post_config() will be called twice. Don't bother
>       * going through all of the initialization on the first call
> @@ -1603,6 +1629,20 @@
>                           "LDAP: SSL support unavailable" );
>      }
>      
> +#ifdef LDAP_OPT_NETWORK_TIMEOUT
> +    if (st->connectionTimeout > 0) {
> +        timeOut.tv_sec = st->connectionTimeout;
> +    }
> +
> +    if (st->connectionTimeout >= 0) {
> +        rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void *)&timeOut);
> +        if (APR_SUCCESS != rc) {
> +            ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> +                             "LDAP: Could not set the connection timeout" );
> +        }
> +    }
> +#endif
> +
>      return(OK);
>  }
>  
....

It looks like this change is causing crashes with PHP:
http://issues.apache.org/bugzilla/show_bug.cgi?id=34618 
http://issues.apache.org/bugzilla/show_bug.cgi?id=34620 

I think the call to ldap_set_option is the cause.

Quoting from http://docs.sun.com/source/816-5616-10/function.htm#24534 

"""If NULL, you are setting the default options that will apply to any
new LDAP connection handles that are subsequently created.  """

Doesn't it seem bad to set this globally?  We are trampling on other
citizens inside the process.

Why isn't this call to ldap_set_option done per-LDAP context?

-Paul

Re: Moved the ldap connection timeout call (was: Re: svn commit: r160707 )

[Rici Lake <ri...@ricilake.net>]

On 26-Apr-05, at 6:58 PM, Brad Nicholes wrote:

>   I just checked in a patch to move the ldap_set_option() call from 
> trying to set the connection timeout globally to ldap connection 
> specific.  I don't have a good way to test that it fixes the problem, 
> but if somebody can verify the fix, I will propose it for backport.

You know that there is a bug in the OpenLDAP SDK up to about 2.2.21 
which causes OpenLDAP to crash when the global connection timeout is 
set (ITS #3487). It ends up doing a double free because it's copying 
pointers instead of contents in the global config structure. I've had 
to explicitly set LDAPConnectionTimeout -1 in order to prevent the 
default timeout being applied, on a couple of installations where I 
couldn't upgrade the SDK.