RFR: 8313678 - SymbolTable can leak Symbols during cleanup

[Coleen Phillimore]

On Thu, 3 Aug 2023 11:21:27 GMT, Oli Gillespie <ogillespie at openjdk.org> wrote:

> Fix leak in SymbolTable during cleanup.
> 
> 1. symbol1 inserted in bucket, refcount 1
> 2. Decrement refcount for this symbol, refcount is now 0
> 3. symbol2 inserted in same bucket
> 4. during insertion, concurrentHashTable notices there is a dead entry in this bucket
>  4a. This triggers delete_in_bucket, which triggers SymbolTableLookup.equals for symbol2
>  4b. SymbolTableLookup.equals for symbol2 spuriously increments symbol2's refcount
> 5. symbol2 is newly inserted with a refcount of 2, thus can never be cleaned up -> leak
> 
> The cleanup routine of concurrentHashTable uses the lookup function for its secondary effect of checking whether any particular value is dead or not. It does not actually care about the main purpose of the lookup function, i.e. checking if a particular value is the desired value.
> 
> The lookup function for SymbolTable, however, has the side effect of incrementing the Symbol refcount when the lookup succeeds. This is presumably with the assumption that a successful lookup will result in a newly held reference. concurrentHashTable's delete_in_bucket can break this assumption by completing a successful lookup without retaining the result (nor ever decrementing the refcount).
> 
> Thus, with a particular sequence of events (as shown in the new test case), a newly inserted Symbol can have a refcount of 2 instead of 1. Even if all legitimate references to this Symbol are removed, it will remain live, thus leaking.
> 
> The fix allows the caller of .equals to specify if they intend to use the value after the lookup, or not, allowing SymbolTable to only increment the refcount when appropriate.
> 
> I'd be happy to hear if anyone has any alternative suggestions for the fix, it seems a bit awkward/dangerous in general that SymbolTableLookup's equals increments the refcount. Also, any suggestions for implementing the test case without relying on the String.hashCode implementation would be great.
> 
> Thanks @shipilev for help debugging and fixing this :)

This is a really good analysis and find.  I'm wondering if it would be cleaner for the LookupFunction class to have an is_dead() function to return whether the entry is dead, rather than calling equals.  The call to equals is surprising in delete_in_bucket.

The reason that the Lookup class increments refcount is that it has the is_dead return, so needs to keep the entry alive at that point.  Also, Symbol refcounts going to zero in a surprising manner is a source of frequent bugs.  Moving that to the Get class (SymbolTableGet) might be too late - ie something may have discovered that the refcount went to zero before incrementing it.

SymbolTable::try_increment_refcount() will fail if the refcount is already zero and we don't want lookup to find that entry.

Can you see if adding a is_dead() function looks nicer here?  The remove_entry() case doesn't matter because we're removing the entry anyway, I believe.

-------------

PR Review: https://git.openjdk.org/jdk/pull/15137#pullrequestreview-1567941355