MySQL Bugs: #101473: A protential nullptr dereference of a smart pointer reported by static analyzer

Bug #101473	A protential nullptr dereference of a smart pointer reported by static analyzer
Submitted:	5 Nov 2020 7:55	Modified:	2 Dec 2020 18:25
Reporter:	Ella Ma	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Logging	Severity:	S3 (Non-critical)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	null

Description:
A nullptr dereference problem through a smart pointer caused by an unchecked, potentially nullable smart pointer.

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

How to repeat:
In function `Iterable_buffer::iterator Iterable_buffer::iterator::operator++(int)` of file `mysql-server/libbinlogevents/src/compression/iterator.cpp` on line 143.

```mysql-server/libbinlogevents/src/compression/iterator.cpp
143 Iterable_buffer::iterator Iterable_buffer::iterator::operator++(int) {
144   Iterable_buffer::iterator to_return = (*this);
145   ++(*this);
146   return to_return;
147 }
```

The copy from `*this` to `to_return` on line 144 will call the assignment operator on line 99.

```mysql-server/libbinlogevents/src/compression/iterator.cpp
 99 Iterable_buffer::iterator &Iterable_buffer::iterator::operator=(
100     const Iterable_buffer::iterator &rhs) {
101   m_target = rhs.m_target;
102   if (rhs.m_reader != nullptr) {  // <== ASSUME m_reader TO BE NULL
103     m_reader = std::make_unique<binary_log::Event_reader>(
104         m_target->m_decompressed_buffer, m_target->m_decompressed_buffer_size);
105     m_reader->go_to(rhs.m_reader->position());
106   }
107   return (*this);
108 }
```

Then on line 102, the condition `rhs.m_reader != nullptr` will check the `m_reader` pointer of `*this`. **Let's assume `this->m_reader` to be nullptr here.** Then the self addition to `*this` on line 145 will call the prefix addition operator on line 122.

```mysql-server/libbinlogevents/src/compression/iterator.cpp
122 Iterable_buffer::iterator &Iterable_buffer::iterator::operator++() {
123   // advance the previous buffer length
124   if (has_next_buffer()) {
  ...
```

On line 124, function `has_next_buffer` is invoked.

```mysql-server/libbinlogevents/src/compression/iterator.cpp
169 bool Iterable_buffer::iterator::has_next_buffer() const {
170   if (m_reader->has_error() ||  // <== DEREFERENCE HERE
171       !m_reader->can_read(LOG_EVENT_MINIMAL_HEADER_LEN))
172     return false;
173 
174   return true;
175 }
```

Then, in the function `has_next_buffer` on line 169, the pointer `this->m_reader`, which was assumed to be nullptr on line 102, is dereferenced on line 170.

See the attached bug report for more details.

Suggested fix:
Add a check to pointer m_reader before using it. If m_reader is null, return false.

```
diff --git a/libbinlogevents/src/compression/iterator.cpp b/libbinlogevents/src/compression/iterator.cpp
index d795bd1da3e..cf58b349681 100644
--- a/libbinlogevents/src/compression/iterator.cpp
+++ b/libbinlogevents/src/compression/iterator.cpp
@@ -167,6 +167,8 @@ bool Iterable_buffer::iterator::operator!=(
 }
 
 bool Iterable_buffer::iterator::has_next_buffer() const {
+  if (!m_reader) return false;
+
   if (m_reader->has_error() ||
       !m_reader->can_read(LOG_EVENT_MINIMAL_HEADER_LEN))
     return false;
```

Bug report generated by the static analyzer

Attachment: report-e265fd.html (text/html), 48.16 KiB.

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.

Hi Verification Team,

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

This problem can also be detected in version 8.0.22, 8.0.21 and 8.0.20. The file mentioned in the report is first introduced after version 8.0.19 and never changed then. Therefore, all versions after 8.0.19 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?

Hi Mrs. Ma,

We have checked the code in our latest release and we consider that you are correct.

Verified as reported.

Just setting the correct version.

Posted by developer:
 
Internals, closing with no changelog entry.

Thank you, Margaret.