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

[vyom tewari]

On Friday 24 August 2018 08:52 PM, Chris Hegarty wrote:
> Hi Vyom,
>
> On 24/08/18 11:35, vyom tewari wrote:
>> Hi All,
>>
>> Please review this simple fix below
>>
>> webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html
>>
>> bugid: https://bugs.openjdk.java.net/browse/JDK-8205330
>>
>> This fix will resolve the race in LdapClient  where we are explicitly 
>> making "null" to LdapClient.conn.
>
> Sorry, I don't know this code all that well, but I think
> that more explanation will be needed before this code
> can be reviewed.
>
Hi Chris, let me try to explain issue little bit.

The issue is a if ldap connection has already been established and then 
the LDAP directory server sends an (unsolicited) "Notice of 
Disconnection", the client's processing of this LDAP message can race 
with an application thread calling new InitialDirContext() to 
authenticate user credentials (i.e.bind) and throw NPE.

I did some analysis and found out that when server send an (unsolicited) 
"Notice of Disconnection",  LdapClient.forceClose() will be called in 
LdapClient.processUnsolicited() which is called asynchronously by the 
reader thread in Connection, this means 'LdapClient.conn' may set to 
null anytime after it received  "Notice of Disconnection".

> The LdapClient and the Connection seem to be loosely
> coupled. I think part of this is to allow the LdapClient
> to be GC'ed and finalized separately to the Connection
> ( that can be reused ). Not setting `conn` to null could
> have a negative impact on this loose coupling. I also
> note that the synchronization is implemented poorly in
> the LdapClient, `conn` is operated on both from within
> synchronized blocks and from unsynchronized blocks (
> which I think is the reason you see the unexpected
> null ).
>
I agree, not setting  'conn' to null will definitely have some impact.  
I check the LdapClient and it looks to me it is intentional(i may be 
wrong) that 'conn' is operated on both from within synchronize blocks 
and from unsynchronize block.

LdapClient, java doc says that connection(conn) take care of it's own sync.

##################################
    access from outside LdapClient must sync;
  *   conn - no sync; Connection takes care of its own sync
  *   unsolicited - sync Vector; multiple operations sync'ed

##################################

Please have a look and do let me know your thought on the above.

Thanks,
Vyom
> -Chris.