You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/10 18:38:52 UTC
[GitHub] [pulsar] oversearch opened a new pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
oversearch opened a new pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630
### 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.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
Change is covered by existing tests, or is otherwise difficult to test. All unit tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630#issuecomment-897139220
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630#issuecomment-897250298
Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630#issuecomment-897821212
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] oversearch commented on pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
oversearch commented on pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630#issuecomment-897906422
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 `test_encryption` Python test that I think is related to the older version of Python I'm running. I rebased on master and pushed again, and it seems to have passed this time. Fingers crossed on the rest of the checks...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui closed pull request #11630: [C++] Fixing use-after-free and constructor bugs in UnAckedMessageTrackerEnabled
Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #11630:
URL: https://github.com/apache/pulsar/pull/11630
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org