RFR: 8339159: api/java_rmi/Naming/Rebind.html crashes with SEGV from UTF8::quoted_ascii_length call

[David Holmes]

On Wed, 11 Sep 2024 06:12:47 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> I agree that the logging seems unnecessary. 
>> 
>> In the case that the logging has to stay, I tend to dislike that we use the Symbol after calling `Afree`, as well as having a dependency to `ZapResourceArea` here. It feels like the wrong design. I would rather we refactor a bit, and call `as_quoted_ascii` once before we destroy.
>> 
>> Something along these lines. But maybe the `#ifndef PRODUCT` things can be tidied up.
>> ```c++
>> static inline void log_trace_symboltable_helper(const char* sym, const char* msg) {
>> #ifndef PRODUCT
>>   log_trace(symboltable)("%s [%s]", msg, sym);
>> #endif // PRODUCT
>> }
>> 
>> static void free_node(void* context, void* memory, Value & value) {
>>   [...]
>>       MutexLocker ml(SymbolArena_lock, Mutex::_no_safepoint_check_flag); // Protect arena
>>       // Deleting permanent symbol should not occur very often (insert race condition),
>>       // so log it.
>>       size_t alloc_size = SymbolTableHash::get_dynamic_node_size(value.byte_size());
>> 
>> #ifndef PRODUCT
>>       LogTarget(Trace, symboltable) lt;
>>       ResourceMark rm;
>>       const char* sym = nullptr;
>>       if (lt.is_enabled()) {
>>         sym = value.as_quoted_ascii();
>>         log_trace_symboltable_helper(sym, "Freeing permanent symbol");
>>       }
>> #endif
>>       if (!SymbolTable::arena()->Afree(memory, alloc_size)) {
>>         NOT_PRODUCT(log_trace_symboltable_helper(sym, "Leaked permanent symbol");)
>>       }
>>   [...]
>> }
>> 
>> 
>> The rest of my message is just semantics, and ultimately irrelevant.
>> 
>>> As far as I can see Arena's don't actually try to free memory and shrink, so in that sense they always "leak".
>> 
>> Arenas frees all its memory when they are destroyed. It is just that this arena in never destroyed. (It is implicitly destroyed when the process is terminated and the OS reclaims its memory).
>> 
>> I use leak here as something which is no longer needed that used a resource but cannot be reused. (Similar to how a Java memory leak, where some object is no longer used, but is still reachable, and thus the GC cannot reclaim the memory used by that object). 
>> 
>> Permanent symbols allocated in this arena are needed  for the lifetime of the program, except when a race creates multiples of the same symbol, and only one is kept. If Afree fails it means that memory can never be used for something useful, i.e. it is leaked.
>> 
>> I am still curious if just using malloc/free would work just as well here.
>> 
>>> In any case we aren't leaking a symbol per-se ju...
>
> The current patch always prints `<unavailable>` in debug builds (where ZapResourceArea is true by default), so it's rather useless.
> 
> Since the value of the symbol has just been printed a few lines before we free it, shouldn't we just do this?
> 
> 
> if (!SymbolTable::arena()->Afree(memory, alloc_size)) {
>    log_trace(symboltable)("Leaked permanent symbol");
> }
> 
> 
> If you worry about concurrent frees that may make it difficult to see which symbol is leaked (I don't know if this can happen or not), you can print the address of the symbol in both logs.

I was thinking I was allowing this to log in product builds and completely overlooked the fact the helper does nothing in product builds. :( As @iklam notes we print the symbol before the free anyway, so we don't need to print after. Further this code executes within the `SymbolArena_lock` so concurrent frees are not an issue. 

Thanks for your suggestions @xmas92 but I think I will go with @iklam 's simpler approach. There are obviously a lot of ways to side-step this particular issue.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20865#discussion_r1753219530