RFR: 8313678 - SymbolTable can leak Symbols during cleanup [v5]

[David Holmes]

On Thu, 10 Aug 2023 12:28:02 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 :)
>
> Oli Gillespie 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 eight additional commits since the last revision:
> 
>  - Add comment for equals() behaviour
>  - Merge remote-tracking branch 'origin/master' into 8313678
>    
>    Hopefully fix GHA for RISC cross-compilation
>  - Undo nullptr 'improvement'
>    
>    It's implemented wrong, and not sure it's worth doing anyway.
>  - Remove .equals is_dead arg
>    
>    Better to use the new is_dead function.
>  - Fix jcheck
>    
>    Whitespace
>  - Fix jcheck
>    
>    Whitespace
>  - Review comments - add dedicated is_dead function
>  - 8313678 - SymbolTable can leak Symbols during cleanup
>    
>    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.

src/hotspot/share/classfile/symbolTable.cpp line 378:

> 376:   }
> 377:   // Note: When equals() returns "true", the symbol's refcount is incremented.
> 378:   // This is need to ensure that symbol is kept alive before equals() returns to caller,

nit: needed
nit: the caller

Thanks for adding the comment.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15137#discussion_r1290086245