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

[Axel Boldt-Christmas]

On Mon, 9 Sep 2024 04:52:42 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Failing to Afree here leaks the memory for the lifetime of the arena (unusable for new allocations), while if it succeeds the memory can be reused during the arenas lifetime.  This arena seems to be created when we start the VM and is never reclaimed, so we effectively leak this memory.
>> 
>> I do not quite understand why we have a `SymbolTable::arena()`. We only seem to use the arena for the permanent symbols. But why not just allocate with malloc, and in the racy case, where we free a permanent (embedded symbol) node just call free.
>> 
>> Maybe there is a fear that permanent small malloc allocations would cause fragmentation. Or there is some other history here.
>
> 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?

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

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