Bug #101468 A protential nullptr dereference of a smart pointer reported by static analyzer
Submitted: 5 Nov 2020 4:21 Modified: 16 Nov 2020 13:28
Reporter: Ella Ma Email Updates:
Status: Won't fix Impact on me:
None 
Category:MySQL Server: Security: Encryption Severity:S3 (Non-critical)
Version:8.0 OS:Any
Assigned to: CPU Architecture:Any
Tags: null

[5 Nov 2020 4:21] Ella Ma
Description:
A nullptr dereference problem through a smart pointer caused by a failed memory allocation and an un-checked smart pointer dereference.

The code scanned is fetched from Github on branch `8.0` of commit ee4455a33b1.

How to repeat:
```mysql-server/plugin/keyring/buffer.cc
40 bool Buffer::get_next_key(IKey **key) {
41   *key = nullptr;
42
43   std::unique_ptr<Key> key_ptr(new Key());  // <== ASSIGNED NULL HERE

   Sub function calls for the new operator:
   - new Key() calls keyring::Keyring_alloc::operator new(size_t)
   - keyring::Keyring_alloc::operator new(size_t) calls kering::kering_malloc(size_t)
   - kering::kering_malloc(size_t) calls my_malloc
   - nullptr is returned from the above calls.

44   size_t number_of_bytes_read_from_buffer = 0;
45   if (data == nullptr) {
46     DBUG_ASSERT(size == 0);
47     return true;
48   }
49   if (key_ptr->load_from_buffer(  // <== DEREFERENCE HERE
50           data + position, &number_of_bytes_read_from_buffer, size - position))
51     return true;
52
53   position += number_of_bytes_read_from_buffer;
54   *key = key_ptr.release();
55   return false;
56 }
```

In function `Buffer::get_next_key` of file `mysql-server/plugin/keyring/buffer.cc`, when `my_malloc` failed to allocate memory and returns `nullptr`, the local unique pointer `key_ptr` on line 43 will be assigned to `nullptr`. Then the pointer is immediately dereferenced on line 49 without being checked.

See the attached bug report for more details.

---

Besides, during the manual review of the code related to the bug report, I also found another problem.

It seems that the function `my_malloc` will eventually use `malloc` to allocate the memory in the function `my_raw_malloc`, using an std::unique_ptr<T, std::default_delete<T>> to wrap the memory may trigger bad deallocation during the destruction of `key_ptr`.

Suggested fix:
Add a check to the variable `key_ptr` online 49 before dereferencing it.

```
diff --git a/plugin/keyring/buffer.cc b/plugin/keyring/buffer.cc
index 330ea5345c8..af21ceabf5c 100644
--- a/plugin/keyring/buffer.cc
+++ b/plugin/keyring/buffer.cc
@@ -46,7 +46,7 @@ bool Buffer::get_next_key(IKey **key) {
     DBUG_ASSERT(size == 0);
     return true;
   }
-  if (key_ptr->load_from_buffer(
+  if (key_ptr && key_ptr->load_from_buffer(
           data + position, &number_of_bytes_read_from_buffer, size - position))
     return true;
```
[5 Nov 2020 4:23] Ella Ma
Bug report generated by the static analyzer

Attachment: report-bc6cfd.html (text/html), 38.90 KiB.

[5 Nov 2020 13:02] MySQL Verification Team
Hi Mr. Ma,

Thank you for your bug report.

However, we don't take bug reports for the unpublished releases.

Please, do let us know if the errors, that you correctly observed, have crept into the official release. Every release is very, very thoroughly tested before being published, so the bug reported might not be there, in the final release.

Meanwhile, we shall inform our Development of your observations.
[6 Nov 2020 3:54] Ella Ma
Hi Verification Team,

> However, we don't take bug reports for the unpublished releases.

This problem can also be detected since version 8.0.16. And all versions since 8.0.16 will have the problem mentioned above.

> Every release is very, very thoroughly tested before being published, so the bug reported might not be there, in the final release.

Since the problem is detected by a static analyzer, chances are that it can not be triggered by dynamic test executions. But it is still worth noticing.

Best regards.

P.S. I am a female. Did I wrongly set my profile?
[6 Nov 2020 13:15] MySQL Verification Team
HI Mrs. Ma,

We have double checked your report and concluded that this is truly a bug

Verified as reported.

Thank you for your contribution.
[11 Nov 2020 12:36] MySQL Verification Team
Hi Mr. Ma,

This is a very low priority bug, so it is unknown when would it be fixed.

However, if you would contribute a valid patch, that could speed things up.

Thanks in advance.
[16 Nov 2020 13:28] Erlend Dahl
Comment by developer:

[11 Nov 2020 4:33] Georgi Kodinov

The MySQL codebase is not built to sustain a failed malloc() call. And fixing
these cases one by one is not really doing anybody any good, but is instead
slowing down the code due to the extra runtime checks.

I thank you for your report and your attempt at fixing the issue. But I do
not think this is the right approach to that. IMHO what needs to happen is
that the codebase is to be moved towards being exception safe and a memory
allocation exception is to be used. But, as you can imagine, this is not a
trivial project (mostly due to the side of the codebase) and needs extra
consideration and planning.
[16 Nov 2020 13:52] MySQL Verification Team
Thank you, Erlend.