[C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled #11630
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
merlimat
approved these changes
Aug 10, 2021
merlimat
added
component/c++
type/bug
|
@oversearch There seems to be a segfault during the execution of Python tests: https://github.com/apache/pulsar/pull/11630/checks?check_run_id=3295994482 |
|
Thanks for your contribution. For this PR, do we need to update docs? (The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks) |
|
Ah yes, sorry about leaving out the docs section. I had assumed leaving it out would imply "no" but I'll make that explicit. I'm also looking into the python test segfault. I realized the python tests weren't building properly in my environment, and I'm working on getting that sorted out.... I might need to upgrade the Python version in my dev environment (currently 3.6). |
…. This was caught by ASAN in my company's build system.
…ckerEnabled This is very similar to a previous fix I submitted in commit 87ebe80. It's the same basic problem, but this class isn't part of the HandlerBase hierarchy, so it needs an independent fix. Essentially, when we create Boost ASIO timer objects from a connection pointer, they maintain a bare reference to the corresponding io_service object inside the connection object. When the destructor runs, we need to destroy the timer *before* the connection object. Keeping the correct order of these member variables is crucial to ensure we don't hit a use-after-free scenario. This was crashing some of our service code in rare circumstances, and is easily caught with Valgrind or ASAN. I also noticed a rather serious bug in one of the UnAckedMessageTrackerEnabled constructors: I believe the intent here was to use the c++11 "delegating constructors" feature, but I think it's written using the Java style, which doesn't work in C++ (see https://stackoverflow.com/questions/13961037/delegate-constructor-c). The semantics of the existing code would just create a new, separate UnAckedMessageTrackerEnabled on the stack in the constructor scope, then immediately destroy it! I corrected the syntax to ensure this works correctly, and fixed up the other constructor to properly use C++ initializer list syntax. Finally, I removed some dangerous c-style casts (which should *never* be used) in favor of C++ static_cast.
oversearch
force-pushed
the
fix/cpp_bugs
branch
from
871cb51
to
2daa64c
Compare
|
I wasn't able to reproduce the stack dump in the tests in my local environment. Everything passes except for an obscure error about a bytes/string type mismatch in the |
codelipenghui
pushed a commit
that referenced
this pull request
Sep 9, 2021
…ckerEnabled (#11630) * [C++] Fix an off-by-one error in my original change from commit 83f0345. This was caught by ASAN in my company's build system. * [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled This is very similar to a previous fix I submitted in commit 87ebe80. It's the same basic problem, but this class isn't part of the HandlerBase hierarchy, so it needs an independent fix. Essentially, when we create Boost ASIO timer objects from a connection pointer, they maintain a bare reference to the corresponding io_service object inside the connection object. When the destructor runs, we need to destroy the timer *before* the connection object. Keeping the correct order of these member variables is crucial to ensure we don't hit a use-after-free scenario. This was crashing some of our service code in rare circumstances, and is easily caught with Valgrind or ASAN. I also noticed a rather serious bug in one of the UnAckedMessageTrackerEnabled constructors: I believe the intent here was to use the c++11 "delegating constructors" feature, but I think it's written using the Java style, which doesn't work in C++ (see https://stackoverflow.com/questions/13961037/delegate-constructor-c). The semantics of the existing code would just create a new, separate UnAckedMessageTrackerEnabled on the stack in the constructor scope, then immediately destroy it! I corrected the syntax to ensure this works correctly, and fixed up the other constructor to properly use C++ initializer list syntax. Finally, I removed some dangerous c-style casts (which should *never* be used) in favor of C++ static_cast. * [C++] Applied clang-format to previous change * [C++] Apparently clang-format didn't like this one either.... (cherry picked from commit 0748ecb)
bharanic-dev
pushed a commit
to bharanic-dev/pulsar
that referenced
this pull request
Mar 18, 2022
…ckerEnabled (apache#11630) * [C++] Fix an off-by-one error in my original change from commit 83f0345. This was caught by ASAN in my company's build system. * [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled This is very similar to a previous fix I submitted in commit 87ebe80. It's the same basic problem, but this class isn't part of the HandlerBase hierarchy, so it needs an independent fix. Essentially, when we create Boost ASIO timer objects from a connection pointer, they maintain a bare reference to the corresponding io_service object inside the connection object. When the destructor runs, we need to destroy the timer *before* the connection object. Keeping the correct order of these member variables is crucial to ensure we don't hit a use-after-free scenario. This was crashing some of our service code in rare circumstances, and is easily caught with Valgrind or ASAN. I also noticed a rather serious bug in one of the UnAckedMessageTrackerEnabled constructors: I believe the intent here was to use the c++11 "delegating constructors" feature, but I think it's written using the Java style, which doesn't work in C++ (see https://stackoverflow.com/questions/13961037/delegate-constructor-c). The semantics of the existing code would just create a new, separate UnAckedMessageTrackerEnabled on the stack in the constructor scope, then immediately destroy it! I corrected the syntax to ensure this works correctly, and fixed up the other constructor to properly use C++ initializer list syntax. Finally, I removed some dangerous c-style casts (which should *never* be used) in favor of C++ static_cast. * [C++] Applied clang-format to previous change * [C++] Apparently clang-format didn't like this one either....
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This is very similar to a previous fix I submitted in commit 87ebe80. It's the same basic problem, but this class isn't part of the HandlerBase hierarchy, so it needs an independent fix. Essentially, when we create Boost ASIO timer objects from a connection pointer, they maintain a bare reference to the corresponding io_service object inside the connection object. When the destructor runs, we need to destroy the timer before the connection object. Keeping the correct order of these member variables is crucial to ensure we don't hit a use-after-free scenario. This was crashing some of our service code in rare circumstances, and is easily caught with Valgrind or ASAN.
While I was at it, I also noticed a rather serious bug in one of the UnAckedMessageTrackerEnabled constructors: I believe the intent here was to use the c++11 "delegating constructors" feature, but I think it's written using the Java style, which doesn't work in C++ (see https://stackoverflow.com/questions/13961037/delegate-constructor-c). The semantics of the existing code would just create a new, separate UnAckedMessageTrackerEnabled on the stack in the constructor scope, then immediately destroy it! I corrected the syntax to ensure this works correctly, and fixed up the other constructor to properly use C++ initializer list syntax. Finally, I removed some dangerous c-style casts (which should never be used) in favor of C++ static_cast.
Modifications
Fixed the variable ordering and constructor syntax, as well as my old off-by-one bug. I checked for other possible instances of this bug, but it appears all of the other deadline timers are declared after the objects containing their io_service.
Docs
No documentation changes are necessary - it's just a bug fix.
Verifying this change
Change is covered by existing tests, or is otherwise difficult to test. All unit tests pass.