ConcurrentModificationException in ProvSSLSessionContext.removeAllExpiredSessions()

[aberenguel]

The scenario is a TLS server with some expired sessions. In such instant, the executed code is

SSLContext sslContext = ... // some pre-existing sslContext backed by BC and with some expired sessions
SSLSessionContext sslSessionContext = sslContext.getServerSessionContext();
Enumeration<byte[]> sessionIdsEnumeration = sslSessionContext.getIds(); // <== throws ConcurrentModificationException

The stacktrace:

java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.remove(LinkedHashMap.java:732)
	at org.bouncycastle.jsse.provider.ProvSSLSessionContext.removeAllExpiredSessions(ProvSSLSessionContext.java:281)
	at org.bouncycastle.jsse.provider.ProvSSLSessionContext.getIds(ProvSSLSessionContext.java:132)

After some debug inspections, I saw the problem is in ProvSSLSessionContext.removeAllExpiredSessions(), more specifically in ProvSSLSessionContext.invalidateIfCreatedBefore(SessionEntry, long). In order to visualize the bug, let's see the code with added comments:

    private void removeAllExpiredSessions()
    {
        processQueue();

        long creationTimeLimit = getCreationTimeLimit(System.currentTimeMillis());

        Iterator<SessionEntry> iter = sessionsByID.values().iterator();
        while (iter.hasNext())
        {
            SessionEntry sessionEntry = iter.next();
            if (invalidateIfCreatedBefore(sessionEntry, creationTimeLimit)) // <== modifies the attribute sessionsByID (see below)
            {
                iter.remove(); // <== tries to modify the attribute this.sessionsByID, which already has been modified
                removeSessionByPeer(sessionEntry);
            }
        }
    }

    private boolean invalidateIfCreatedBefore(SessionEntry sessionEntry, long creationTimeLimit)
    {
        ProvSSLSession session = sessionEntry.get();
        if (session == null)
        {
            return true;
        }
        if (session.getCreationTime() < creationTimeLimit)
        {
            session.invalidate(); // <== here, the attribute sessionsByID is modified by invoking removeSession(byte[] sessionID)
        }
        return !session.isValid();
    }

My solution is to change the removeAllExpiredSessions() method so that the sessionsByID can be changed in the loop.

    private void removeAllExpiredSessions()
    {
        processQueue();

        long creationTimeLimit = getCreationTimeLimit(System.currentTimeMillis());

        Iterator<Entry<SessionID, SessionEntry>> iter = new ArrayList<>(sessionsByID.entrySet()).iterator();
        while (iter.hasNext())
        {
            Entry<SessionID, SessionEntry> entry = iter.next();
            if (invalidateIfCreatedBefore(entry.getValue(), creationTimeLimit))
            {
                sessionsByID.remove(entry.getKey());
                removeSessionByPeer(entry.getValue());
            }
        }
    }

[ahmcosta]

I'm facing the same issue.

[peterdettman]

Thanks for the report. This is now fixed in our git repo and will appear here soon, plus we should get a new beta up shortly.

I preferred a fix that avoids the ProvSSLSessionBase.invalidate call to removeSession instead of copying the entrySet.

[peterdettman]

Should be fixed by https://github.com/bcgit/bc-java/commit/8dda7ca6a6de19d9c42d72cb4aa73e5af0e87bdd .

A new beta with the fix (1.70b01) is available here: https://downloads.bouncycastle.org/betas/ . Confirmation would be appreciated.

[peterdettman]

@aberenguel Were you able to verify this was fixed by 1.70 release?