Re: (ITS#6828) TLS fails to start when LDAP_OPT_CONNECT_ASYNC is used

[hyc@symas.com]

hyc@symas.com wrote:
> ipuleston@SonicWALL.com wrote:
>> The patch that I submitted above has a memory leak due not freeing "ber"
> when returning error LDAP_X_CONNECTING from function
> ldap_send_initial_request(). I have uploaded a new version of the patch with
> this fixed to ftp.openldap.org/incoming as ian-puleston-110412.patch.2 (note
> the .2 on the end - it was a 2nd attempt after I forgot to specify ASCII mode).
>
> Unfortunately the patch was made against 2.4.23 and not against HEAD. It does
> not apply cleanly to current git. Patches must always be submitted against the
> development code, not against released code.

I've committed an alternate patch to git master.
>
> I'm pretty sure that this chunk can never do anything useful.
>
> diff -u libldap.orig/request.c libldap/request.c
> --- libldap.orig/request.c	2010-06-10 10:39:48.000000000 -0700
> +++ libldap/request.c	2011-04-12 12:11:20.769166100 -0700
> @@ -446,7 +462,21 @@
>    		lc->lconn_server = ldap_url_dup( srv );
>    	}
>
> -	lc->lconn_status = async ? LDAP_CONNST_CONNECTING : LDAP_CONNST_CONNECTED;
> +	if ( async ) {
> +#ifdef HAVE_TLS
> +		void *ssl = NULL;
> +		ber_sockbuf_ctrl( lc->lconn_sb, LBER_SB_OPT_GET_SSL, (void *)&ssl );
> +		if ( ssl ) {
> +			/* the connect finished and tls was started */
> +			lc->lconn_status = LDAP_CONNST_CONNECTED;
> +		} else
> +#endif
> +		{
> +			lc->lconn_status = LDAP_CONNST_CONNECTING;
> +		}
> +	} else {
> +		lc->lconn_status = LDAP_CONNST_CONNECTED;
> +	}
>    #ifdef LDAP_R_COMPILE
>    	ldap_pvt_thread_mutex_lock(&ld->ld_conn_mutex );
>    #endif
>
> The connection has just been created, asynchronously, so there's no way that
> the TLS layer was already started when it got here. Also, even if this was
> valid, it obviously would only apply for OPT_X_TLS_HARD || ldaps, but you
> haven't checked that condition here. Ultimately it appears that this test is a
> no-op.
>
> Also, I suggest that you only check for CONNST_CONNECTING in the callers, and
> do the TLS check in the check function, like this:
>
> +int
> +ldap_int_check_async_open( LDAP *ld, LDAPConn *lc, LDAPURLDesc *srv,
> ber_socket_t sd )
> +{
> +       struct timeval tv = { 0 };
> +       int rc;
> +
> +       rc = ldap_int_poll( ld, sd,&tv );
> +       switch ( rc ) {
> +       case 0:
> +               /* now ready to start tls */
> +               lc->lconn_status = LDAP_CONNST_CONNECTED;
> +               break;
> +
> +       default:
> +               rc = -1;
> +		return rc;
> +       case -2:
> +               /* connect not completed yet */
> +		ld->ld_errno = LDAP_X_CONNECTING;
> +               return rc;
> +       }
> +
> +#ifdef HAVE_TLS
> +       if ( ld->ld_options.ldo_tls_mode == LDAP_OPT_X_TLS_HARD ||
> +               !strcmp( srv->lud_scheme, "ldaps" )) {
> +
> +               ++lc->lconn_refcnt;     /* avoid premature free */
> +
> +               rc = ldap_int_tls_start( ld, lc, srv );
> +
> +               --lc->lconn_refcnt;
> +       }
> +#endif
> +       return rc;
> +}
>


-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/