On Wed, 15 Oct 2025 17:34:55 GMT, Weibing Xiao <[email protected]> wrote:
>> [webrev.zip](https://github.com/user-attachments/files/22605072/webrev.zip) >> NPE thrown from SASL GSSAPI impl when TLS is used with QOP auth-int against >> Active Directory. >> >> When the exception is triggered, LDAP Connection will do "clean-up" >> operation and output stream get flushed and closed the context while >> GssKrb5Client is still wrapping the message, and tried to send the abandoned >> info to the client at line >> https://github.com/openjdk/jdk/blob/master/src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java#L140. >> That's the reason to throw NPE. >> >> The change is going to close socket and output stream in LdapClient.java. It >> would allow SASL client code to send the abandoned request to client; then >> dispose GSS context. This will avoid NPE to thrown at line 140 of >> GssKrb5Base.java. >> >> No test file is attached for this MR since it needs Sasl LDAP server with >> security setup. Attached the updated webrev for the reference. > > Weibing Xiao has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 26 additional > commits since the last revision: > > - update the code > - Merge branch 'master' of github.com:openjdk/jdk into JDK-8362268 > - Merge branch 'master' into JDK-8362268 > - Merge branch 'master' of github.com:weibxiao/jdk > - Merge branch 'openjdk:master' into master > - Merge branch 'openjdk:master' into master > - Merge branch 'master' of github.com:openjdk/jdk > - Merge branch 'openjdk:master' into master > - Merge branch 'master' of github.com:openjdk/jdk > - Merge branch 'master' of github.com:openjdk/jdk > - ... and 16 more: https://git.openjdk.org/jdk/compare/614738ee...4dd20668 I am not quite sure I understand how this fix works. If all tests are passing then it may be OK. I hope it isn't going to re-introduce a resource leak though. Synchronization/locking must be fixed however, and I have suggseted some changes below that will ensure it integrates correctly with the locking strategy in the Connection class. It would be good to get @AlekseiEfimov review. src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java line 464: > 462: // Not being pooled; continue with closing > 463: conn.cleanup(reqCtls, false); > 464: closeOpenedResource(); Please replace these two lines with: `conn.cleanupAndClose(reqCtls);` Then add a method in Connection: void cleanupAndClose(Control[] reqCtls) { lock.lock(); try { cleanup(reqCtls, false); // 8313657 socket is not closed until GC is run // it caused the bug 8362268, hence moved here if (outStream != null) { outStream.close(); } if (!sock.isClosed()) { sock.close(); } } catch (IOException ignored) { // we're closing, ignore IO. } finally { lock.unlock(); } } src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java line 497: > 495: } catch (IOException ioEx) { > 496: //ignore the error; > 497: } Please revert this change. Use conn.cleanupAnadClose(...) instead. src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java line 512: > 510: } > 511: conn.cleanup(null, false); > 512: closeOpenedResource(); same here. replace these two lines with: `conn.cleanupAndClose(null);` ------------- Changes requested by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/26566#pullrequestreview-3350556027 PR Review Comment: https://git.openjdk.org/jdk/pull/26566#discussion_r2440169793 PR Review Comment: https://git.openjdk.org/jdk/pull/26566#discussion_r2440180557 PR Review Comment: https://git.openjdk.org/jdk/pull/26566#discussion_r2440177815
