[C++] Fix potential crash caused by AckGroupTracker's timer #8519

BewareMyPower · 2020-11-11T05:39:32Z

Motivation

The AckGroupingTrackerEnabled's timer callback only captures this, which is a weak reference to the AckGroupingTrackerEnabled instance. If the instance went out of the scope and destroyed, this would point to an invalid block.

Even if the destructor of AckGroupingTrackerEnabled cancels the timer, the callback may not be triggered immediately. There's still a possibility that when the callback is triggered, the error code is 0 but accessing to this is invalid. For example, there's a crash caused by the callback in production environment that is hard to reproduce:

#6 <signal handler called>
#7 0x00007fb4e67c5cb8 in ?? ()
#8 0x00007fb604981adb in operator() (ec=..., __closure=0x7fb52b0fb230)
   at /usr/local/src/apache-pulsar-microfocus/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc:148
#9 operator() (this=0x7fb52b0fb230) at /usr/local/include/boost/asio/detail/bind_handler.hpp:47

Modifications

Use std::shared_ptr instead of std::unique_ptr for AckGroupingTrackerEnabled, then capture the shared pointer in timer callback's lambda expression to extend the lifetime of this.
Add start() method to AckGroupingTracker to avoid std::bad_weak_ptr because shared_from_this() in a constructor returns a null pointer.
Use std::weak_ptr to reference HandlerBase in case that the handler may be invalid when the timer callback is triggered.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

BewareMyPower · 2020-11-11T10:57:32Z

/pulsarbot run-failure-checks

codelipenghui · 2020-11-12T01:46:29Z

/pulsarbot cherry-pick to branch-2.6

### Motivation The `AckGroupingTrackerEnabled`'s timer callback only captures `this`, which is a weak reference to the `AckGroupingTrackerEnabled ` instance. If the instance went out of the scope and destroyed, `this` would point to an invalid block. Even if the destructor of `AckGroupingTrackerEnabled` cancels the timer, the callback may not be triggered immediately. There's still a possibility that when the callback is triggered, the error code is 0 but accessing to `this` is invalid. For example, there's a crash caused by the callback in production environment that is hard to reproduce: ``` #6 <signal handler called> #7 0x00007fb4e67c5cb8 in ?? () #8 0x00007fb604981adb in operator() (ec=..., __closure=0x7fb52b0fb230) at /usr/local/src/apache-pulsar-microfocus/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc:148 #9 operator() (this=0x7fb52b0fb230) at /usr/local/include/boost/asio/detail/bind_handler.hpp:47 ``` ### Modifications - Use `std::shared_ptr` instead of `std::unique_ptr` for `AckGroupingTrackerEnabled`, then capture the shared pointer in timer callback's lambda expression to extend the lifetime of `this`. - Add `start()` method to `AckGroupingTracker` to avoid `std::bad_weak_ptr` because `shared_from_this()` in a constructor returns a null pointer. - Use `std::weak_ptr` to reference `HandlerBase` in case that the handler may be invalid when the timer callback is triggered.

### Motivation The `AckGroupingTrackerEnabled`'s timer callback only captures `this`, which is a weak reference to the `AckGroupingTrackerEnabled ` instance. If the instance went out of the scope and destroyed, `this` would point to an invalid block. Even if the destructor of `AckGroupingTrackerEnabled` cancels the timer, the callback may not be triggered immediately. There's still a possibility that when the callback is triggered, the error code is 0 but accessing to `this` is invalid. For example, there's a crash caused by the callback in production environment that is hard to reproduce: ``` #6 <signal handler called> #7 0x00007fb4e67c5cb8 in ?? () #8 0x00007fb604981adb in operator() (ec=..., __closure=0x7fb52b0fb230) at /usr/local/src/apache-pulsar-microfocus/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc:148 #9 operator() (this=0x7fb52b0fb230) at /usr/local/include/boost/asio/detail/bind_handler.hpp:47 ``` ### Modifications - Use `std::shared_ptr` instead of `std::unique_ptr` for `AckGroupingTrackerEnabled`, then capture the shared pointer in timer callback's lambda expression to extend the lifetime of `this`. - Add `start()` method to `AckGroupingTracker` to avoid `std::bad_weak_ptr` because `shared_from_this()` in a constructor returns a null pointer. - Use `std::weak_ptr` to reference `HandlerBase` in case that the handler may be invalid when the timer callback is triggered. (cherry picked from commit cfa65d0)

codelipenghui · 2020-11-12T06:36:43Z

cherry-picked to branch-2.6

) ### Motivation The `AckGroupingTrackerEnabled`'s timer callback only captures `this`, which is a weak reference to the `AckGroupingTrackerEnabled ` instance. If the instance went out of the scope and destroyed, `this` would point to an invalid block. Even if the destructor of `AckGroupingTrackerEnabled` cancels the timer, the callback may not be triggered immediately. There's still a possibility that when the callback is triggered, the error code is 0 but accessing to `this` is invalid. For example, there's a crash caused by the callback in production environment that is hard to reproduce: ``` #6 <signal handler called> #7 0x00007fb4e67c5cb8 in ?? () #8 0x00007fb604981adb in operator() (ec=..., __closure=0x7fb52b0fb230) at /usr/local/src/apache-pulsar-microfocus/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc:148 #9 operator() (this=0x7fb52b0fb230) at /usr/local/include/boost/asio/detail/bind_handler.hpp:47 ``` ### Modifications - Use `std::shared_ptr` instead of `std::unique_ptr` for `AckGroupingTrackerEnabled`, then capture the shared pointer in timer callback's lambda expression to extend the lifetime of `this`. - Add `start()` method to `AckGroupingTracker` to avoid `std::bad_weak_ptr` because `shared_from_this()` in a constructor returns a null pointer. - Use `std::weak_ptr` to reference `HandlerBase` in case that the handler may be invalid when the timer callback is triggered. (cherry picked from commit cfa65d0) (cherry picked from commit 98591c4)

) ### Motivation The `AckGroupingTrackerEnabled`'s timer callback only captures `this`, which is a weak reference to the `AckGroupingTrackerEnabled ` instance. If the instance went out of the scope and destroyed, `this` would point to an invalid block. Even if the destructor of `AckGroupingTrackerEnabled` cancels the timer, the callback may not be triggered immediately. There's still a possibility that when the callback is triggered, the error code is 0 but accessing to `this` is invalid. For example, there's a crash caused by the callback in production environment that is hard to reproduce: ``` #6 <signal handler called> apache#7 0x00007fb4e67c5cb8 in ?? () apache#8 0x00007fb604981adb in operator() (ec=..., __closure=0x7fb52b0fb230) at /usr/local/src/apache-pulsar-microfocus/pulsar-client-cpp/lib/AckGroupingTrackerEnabled.cc:148 apache#9 operator() (this=0x7fb52b0fb230) at /usr/local/include/boost/asio/detail/bind_handler.hpp:47 ``` ### Modifications - Use `std::shared_ptr` instead of `std::unique_ptr` for `AckGroupingTrackerEnabled`, then capture the shared pointer in timer callback's lambda expression to extend the lifetime of `this`. - Add `start()` method to `AckGroupingTracker` to avoid `std::bad_weak_ptr` because `shared_from_this()` in a constructor returns a null pointer. - Use `std::weak_ptr` to reference `HandlerBase` in case that the handler may be invalid when the timer callback is triggered.

…mer(issue like #apache#8519) ### Motivation - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up `msgId`, not <= `msgId` in `UnAckedMessageTrackerEnabled::removeMessagesTill` - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like apache#8519) ### Modifications - When do AcknowledgeCumulative from application, earse <= `msgId` in UnAckedMessageTrackerEnabled, avoid redeliver unnecessary unacknowledged messages to Broker - Use std::shared_ptr instead of std::unique_ptr for UnAckedMessageTrackerEnabled - add `start()`, `close()` method to `UnAckedMessageTrackerEnabled` solve same issue see apache#8519 - add `isEmpty()`, `size()` method to `UnAckedMessageTrackerEnabled` for checking of test case - when close `UnAckedMessageTrackerEnabled` and `AckGroupingTrackerEnabled`, reset shared_ptr `timer_` - add unit test for `UnAckedMessageTrackerEnabled`

…mer(issue like #apache#8519) - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up `msgId`, not <= `msgId` in `UnAckedMessageTrackerEnabled::removeMessagesTill` - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like apache#8519) - When do AcknowledgeCumulative from application, earse <= `msgId` in UnAckedMessageTrackerEnabled, avoid redeliver unnecessary unacknowledged messages to Broker - Use std::shared_ptr instead of std::unique_ptr for UnAckedMessageTrackerEnabled - add `start()`, `close()` method to `UnAckedMessageTrackerEnabled` solve same issue see apache#8519 - add `isEmpty()`, `size()` method to `UnAckedMessageTrackerEnabled` for checking of test case - when close `UnAckedMessageTrackerEnabled` and `AckGroupingTrackerEnabled`, reset shared_ptr `timer_` - add unit test for `UnAckedMessageTrackerEnabled`

…mer(issue like #apache#8519) - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up `msgId`, not <= `msgId` in `UnAckedMessageTrackerEnabled::removeMessagesTill` - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like apache#8519) - When do AcknowledgeCumulative from application, earse <= `msgId` in UnAckedMessageTrackerEnabled, avoid redeliver unnecessary unacknowledged messages to Broker - Use std::shared_ptr instead of std::unique_ptr for UnAckedMessageTrackerEnabled - add `start()`, `close()` method to `UnAckedMessageTrackerEnabled` solve same issue see apache#8519 - add `isEmpty()`, `size()` method to `UnAckedMessageTrackerEnabled` for checking of test case - when close `UnAckedMessageTrackerEnabled` and `AckGroupingTrackerEnabled`, reset shared_ptr `timer_` - add unit test for `UnAckedMessageTrackerEnabled` [C++] Fix potential crash caused by UnAckedMessageTrackerEnabled's timer(issue like #apache#8519) - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up `msgId`, not <= `msgId` in `UnAckedMessageTrackerEnabled::removeMessagesTill` - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like apache#8519) - When do AcknowledgeCumulative from application, earse <= `msgId` in UnAckedMessageTrackerEnabled, avoid redeliver unnecessary unacknowledged messages to Broker - Use std::shared_ptr instead of std::unique_ptr for UnAckedMessageTrackerEnabled - add `start()`, `close()` method to `UnAckedMessageTrackerEnabled` solve same issue see apache#8519 - add `isEmpty()`, `size()` method to `UnAckedMessageTrackerEnabled` for checking of test case - when close `UnAckedMessageTrackerEnabled` and `AckGroupingTrackerEnabled`, reset shared_ptr `timer_` - add unit test for `UnAckedMessageTrackerEnabled`

Use shared pointer for AckGroupingTrackerEnabled

cef7531

BewareMyPower changed the title ~~[C++] Fix potential crash caused by AckGroupTracker's timer~~ [WIP][C++] Fix potential crash caused by AckGroupTracker's timer Nov 11, 2020

Use weak_ptr to reference HandlerBase

27bcdb8

BewareMyPower changed the title ~~[WIP][C++] Fix potential crash caused by AckGroupTracker's timer~~ [C++] Fix potential crash caused by AckGroupTracker's timer Nov 11, 2020

Fix test compile error

4ab9bae

BewareMyPower changed the title ~~[C++] Fix potential crash caused by AckGroupTracker's timer~~ [WIP][C++] Fix potential crash caused by AckGroupTracker's timer Nov 11, 2020

Fix bad_weak_ptr caused by shared_from_this()

d1fe15b

BewareMyPower changed the title ~~[WIP][C++] Fix potential crash caused by AckGroupTracker's timer~~ [C++] Fix potential crash caused by AckGroupTracker's timer Nov 11, 2020

codelipenghui assigned BewareMyPower Nov 11, 2020

codelipenghui added the release/2.6.3 label Nov 11, 2020

codelipenghui added this to the 2.7.0 milestone Nov 11, 2020

codelipenghui requested review from merlimat, jiazhai and sijie November 11, 2020 11:47

jiazhai approved these changes Nov 12, 2020

View reviewed changes

codelipenghui merged commit cfa65d0 into apache:master Nov 12, 2020

codelipenghui added the cherry-picked/branch-2.6 label Nov 21, 2020

saosir mentioned this pull request Nov 30, 2020

[C++] fix cpp client do AcknowledgeCumulative not clean up previous message #8606

Merged

BewareMyPower deleted the bewaremypower/fix-cpp-ack-group branch December 2, 2020 07:21

BewareMyPower mentioned this pull request Dec 11, 2020

[C++] AckGroupingTrackerEnabled may cause segmentation fault #8914

Closed

sijie mentioned this pull request Dec 11, 2020

ISSUE-8914: [C++] AckGroupingTrackerEnabled may cause segmentation fault streamnative/pulsar-archived#1868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++] Fix potential crash caused by AckGroupTracker's timer #8519

[C++] Fix potential crash caused by AckGroupTracker's timer #8519

BewareMyPower commented Nov 11, 2020 •

edited

Loading

BewareMyPower commented Nov 11, 2020

codelipenghui commented Nov 12, 2020

codelipenghui commented Nov 12, 2020

[C++] Fix potential crash caused by AckGroupTracker's timer #8519

[C++] Fix potential crash caused by AckGroupTracker's timer #8519

Conversation

BewareMyPower commented Nov 11, 2020 • edited Loading

Motivation

Modifications

Verifying this change

BewareMyPower commented Nov 11, 2020

codelipenghui commented Nov 12, 2020

codelipenghui commented Nov 12, 2020

BewareMyPower commented Nov 11, 2020 •

edited

Loading