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

[Ioi Lam]

On Wed, 11 Sep 2024 05:47:33 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Thanks for taking a look @xmas92 .  As far as I can see Arena's don't actually try to free memory and shrink, so in that sense they always "leak". `Afree` has one fast path that if the thing being freed was the last thing allocated then it reclaims it by resetting the hwm - otherwise it is a no-op other than zapping the freed region. In any case we aren't leaking a symbol per-se just leaking part of the memory used by the `symbolTable`. Given no other code ever checks the result of `Afree` I am tempted to just delete the logging statement. Otherwise the fix presently in the PR will suffice.
>
> 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 just leaking part of the memory used by the symbolTable.
> 
> Is there a difference?

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.

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

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