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

[Chris Hegarty]

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.


On 07/09/18 17:47, Daniel Fuchs wrote:
> Hi Vyom,
> 
> Please see inline... I've elided some parts...
> 
> On 07/09/2018 10:11, vyom tewari wrote:
>> On Tuesday 04 September 2018 08:13 PM, Daniel Fuchs wrote:
>>> So what we see here is actually a race between an open
>>> connection being retrieved from the pool, and the server
>>> deciding to close that connection.
>>> The connection/client is retrieved from the pool, then
>>> unfortunately closed by the server after having been selected
>>> for the next operation.
>>>
>> I checked  the code and i did not found any problem. Both "get" and 
>> "removePooledConnection" are "synchronized" so there should not be any 
>> problem(concurrency issue) here.
> 
> I'm referring to this pattern (in forceClose):
> 
>              conn.cleanup(null, false);
>              conn = null;
> 
>              if (cleanPool) {
>                  pcb.removePooledConnection(this);
>              }
> 
> The connection is cleaned up, then nulled, then only removed
> from the pool. I'm questioning whether the first thing to do would
> be to remove the connection from the pool, to reduce the time window
> in which the client could be retrieved from the pool by another
> thread while forceClose is in process.
> 
> I am not sure what side effect this could have - as I haven't
> studied the code that retrieves the client from the pool,
> but I'm wondering whether that's worth exploring.
> 
>> As you said server is closing the same connection which is retrieved 
>> from pool which  is correct but this should not throw unintentional NPE.
> 
> Agreed.
> 
>> In LdapClient, we set the "Ldapclient.conn" to explicitly null 
>> asynchronously after we remove the connection from pool and which is 
>> creating the NPE.
>>
>> LdapClient does not set `Ldapclient.conn` to null if connection is not 
>> pooled.
> 
> Really? That's not what I see... Am I missing something?
> 
>  >> [...]
>>> If so will leaving the 'conn' field assigned ensure that
>>> the retry happens, or will new InitialDirContext() fail
>>> with some other exception because the connection has
>>> been closed? Or does the code simply assume that asynchronous
>>> closing of the connection by the server can only happen while
>>> it's sitting idle in the pool?
>>>
>>   I don't know if retrying is feasible in 'IntialDirContext' but if we 
>> leave 'LdapClient.conn' assigned then we will get 
>> "javax.naming.CommunicationException" which tells that, underline 
>> connection is closed. I am not 100% sure if this is right approach but 
>> if we set 'LdapClient.conn' to null asynchronously then we will hit NPE.
> 
> OK - that's good to know. I agree that CommunicationException is
> better than NPE. If so then not nulling the connection
> is at least an improvement.
> But this would deserve a comment in the code, I think.
> More below...
> 
>>> I wonder if some other mechanism could be used to reduce
>>> and hopefully eliminate the time window in which the race
>>> could appear. For instance taking the connection out of
>>> the pool before closing it, instead of closing it before
>>> taking it out of the pool, etc...
>>>
>> Changing order will not help, i think closing the  same connection is 
>> not a problem  but setting `LdapClient.conn'  to null asynchronously 
>> is causing NPE.
> 
> Yes - I agree that setting conn to null is problematic.
> I wonder why it's set to null. Maybe in an attempt to
> make it eligible for GC earlier?
> 
> If that's not the issue, then I would suggest making it
> final (AFAICS it's only set to non-null once) and cleaning
> up the code that tests for conn == null.
> 
> If GC is the issue, then the best you can do is probably
> only set it to null when the client is not pooled.
> 
> And if we could reduce the time window in which a thread
> could get a stale client from the pool that might be good
> too?
> 
> Is there anyway we could somehow add a test that reproduce
> the NPE?
> 
> best regards,
> 
> -- daniel
> 
> [...]
> 
>>>>> 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.
> [...]