RFR: 8362268 : NPE thrown from SASL GSSAPI impl when TLS is used with QOP auth-int against Active Directory [v5]

[Daniel Fuchs]

On Wed, 15 Oct 2025 17:34:55 GMT, Weibing Xiao <wxiao at openjdk.org> 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