MySQL Bugs: #99078: A memory leak might happen when doing lf_hash

Bug #99078	A memory leak might happen when doing lf_hash_insert
Submitted:	26 Mar 2020 10:41	Modified:	9 Apr 2020 18:53
Reporter:	Xiong Wang	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Performance Schema	Severity:	S2 (Serious)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
From the source, a memory leak might happen in function lf_hash_insert.
int lf_hash_insert(LF_HASH *hash, LF_PINS *pins, const void *data) {
  int csize, bucket, hashnr;
  LF_SLIST *node;
  std::atomic<LF_SLIST *> *el;

  node = (LF_SLIST *)lf_alloc_new(pins);
  if (unlikely(!node)) {
    return -1;
  }
==================> Until here, node has been created.
  uchar *extra_data =
      (uchar *)(node + 1);  // Stored immediately after the node.
  if (hash->initialize) {
    (*hash->initialize)(extra_data, (const uchar *)data);
  } else {
    memcpy(extra_data, data, hash->element_size);
  }
  node->key = hash_key(hash, (uchar *)(node + 1), &node->keylen);
  hashnr = calc_hash(hash, node->key, node->keylen);
  bucket = hashnr % hash->size;
  el = static_cast<std::atomic<LF_SLIST *> *>(
      lf_dynarray_lvalue(&hash->array, bucket));
  if (unlikely(!el)) {
===================> Error happens, but the created 'node'  isn't freed
    return -1;
  }
  if (el->load() == nullptr &&
      unlikely(initialize_bucket(hash, el, bucket, pins))) {
===================> Ditto
    return -1;
  }
  node->hashnr = my_reverse_bits(hashnr) | 1; /* normal node */
  if (linsert(el, hash->charset, node, pins, hash->flags)) {
    lf_pinbox_free(pins, node); =============> Here, the node is freed.
    return 1;
  }

How to repeat:
Look through source code

Suggested fix:
When error happens, lf_pinbox_free should be called at the same time.

Hello Mr. Wang,

Thank you for your bug report.

I have carefully analysed that function and entire code where this function is utilised.

I must admit that you are correct in your analysis. Not only that the pointer in question is allocated from heap (and not from stack), but also there are many places in the source code files where you have a conditional statement like this:

If (lf_hash_insert(.......))

although this function can return both -1 or 1 in case of error. I have not encountered (so far) a single place where those two error codes are distinguished.

Verified as reported, but with a higher severity.

Posted by developer:
 
Fixed in 8.0.21.

A potential memory leak in lf_hash_insert() was fixed.

Thank you, Paul.