RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection

[Daniel Fuchs]

Nice find Chris.

But I have to wonder why the NPE wasn't thrown at the
very beginning of `authenticate` namely one of these
two lines:

         int readTimeout = conn.readTimeout;
         conn.readTimeout = conn.connectTimeout;

which happen to be called even before ensureOpen() is
called a few lines later!

cheers,

-- daniel

On 10/09/2018 13:00, Chris Hegarty wrote:
> Vyom,
> 
> The NPE is originating from the finally block of the LdapClient
> `authenticate` method. In the finally block there is an attempt
> to restore the "read" timeout, since it was changed earlier in
> `authenticate`, to reflect the connect timeout value, since the
> subsequent operation(s) are considered part of the connect phase.
> 
> An unsolicited message may close the connection during
> authenticate, which is does and LdapClient.ldapBind throws
> "javax.naming.CommunicationException: Request: 1 cancelled". The
> finally block is then executed and the previous exception is
> effectively throw away when the NPE is encountered.
> 
> If we agree that such behaviour is reasonable ( and I think it
> most likely is ), then the finally block needs to be more
> defensive - check that conn is not null. It makes no sense to
> attempt to reset the read timeout if conn is null.
> 
> With the following change, then the CommunicationException will
> be thrown from `authenticate`. I think this is the behaviour we
> want, no?
> 
> 
> --- a/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java
> +++ b/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java
> @@ -297,7 +297,8 @@
>               conn.setV3(isLdapv3);
>               return res;
>           } finally {
> -            conn.readTimeout = readTimeout;
> +            if (conn != null)
> +                conn.readTimeout = readTimeout;
>           }
>       }
> 
> 
> -Chris.