Skip to content
This repository has been archived by the owner on Apr 6, 2019. It is now read-only.

Fix memory corruption in case of pending callbacks on destruction of client object #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions includes/cpp_redis/core/client.hpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// The MIT License (MIT)
// The MIT License (MIT)
//
// Copyright (c) 2015-2017 Simon Ninon <[email protected]>
//
Expand Down Expand Up @@ -161,6 +162,13 @@ class client {
//!
void cancel_reconnect(void);

//!
//! Wait until all callbacks are completed.
//!
//! \return current instance
//!
client& wait_for_completion(void);

public:
//!
//! reply callback called whenever a reply is received
Expand Down
29 changes: 20 additions & 9 deletions sources/core/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ client::~client(void) {
m_client.disconnect(true);
}

// [email protected]
// If there are pending callbacks we have to wait until they are completed
// otherwise this will cause a memory corruption if they return after this point.
wait_for_completion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if no callbacks are pending, then notify_all() will not be called from connection_receive_handler and m_sync_condvar.wait will hang. So I don't think this works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take that back. However, there is another problem: If there are uncommitted commands, then wait for completion will not return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skeetor , have a look at this please https://github.com/steple/cpp_redis/commit/b69991470c4ea35b29279b79b8b836c0321ad315

This will pass all tests. I'll try to add a test case that tests for the original bug of the pending callback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does this mean that I can withdraw my PR and you will submit yours?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will, but my PR is missing a test case for the bug that you had found. I'll need to add that before I can submit it. If you have an idea on how to test it, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unable to reproduce what I think is the bug you described. Can you provide an example or a more detailed description?


__CPP_REDIS_LOG(debug, "cpp_redis::client destroyed");
}

Expand Down Expand Up @@ -125,6 +130,7 @@ client::disconnect(bool wait_for_removal) {

//! make sure we clear buffer of unsent commands
clear_callbacks();
wait_for_completion();

__CPP_REDIS_LOG(info, "cpp_redis::client disconnected");
}
Expand Down Expand Up @@ -201,11 +207,7 @@ client::sync_commit(void) {
try_commit();
}

std::unique_lock<std::mutex> lock_callback(m_callbacks_mutex);
__CPP_REDIS_LOG(debug, "cpp_redis::client waiting for callbacks to complete");
m_sync_condvar.wait(lock_callback, [=] { return m_callbacks_running == 0 && m_commands.empty(); });
__CPP_REDIS_LOG(debug, "cpp_redis::client finished waiting for callback completion");
return *this;
return wait_for_completion();
}

void
Expand All @@ -215,11 +217,11 @@ client::try_commit(void) {
m_client.commit();
__CPP_REDIS_LOG(info, "cpp_redis::client sent pipelined commands");
}
catch (const cpp_redis::redis_error&) {
catch (const cpp_redis::redis_error& e) {
__CPP_REDIS_LOG(error, "cpp_redis::client could not send pipelined commands");
//! ensure commands are flushed
clear_callbacks();
throw;
throw e;
}
}

Expand Down Expand Up @@ -250,8 +252,17 @@ client::connection_receive_handler(network::redis_connection&, reply& reply) {
}
}

client&
client::wait_for_completion(void) {
std::unique_lock<std::mutex> lock_callback(m_callbacks_mutex);
__CPP_REDIS_LOG(debug, "cpp_redis::client waiting for callbacks to complete");
m_sync_condvar.wait(lock_callback, [=] { return m_callbacks_running == 0 && m_commands.empty(); });
__CPP_REDIS_LOG(debug, "cpp_redis::client finished waiting for callback completion");
return *this;
}

void
client::clear_callbacks(void) {
client::clear_callbacks() {
if (m_commands.empty()) {
return;
}
Expand Down Expand Up @@ -288,7 +299,7 @@ client::resend_failed_commands(void) {
//! dequeue commands and move them to a local variable
std::queue<command_request> commands = std::move(m_commands);

while (commands.size() > 0) {
while (m_commands.size() > 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change would cause a regression since m_commands would be empty after moving it into the local variable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If m_commands is empty after the move, then the loop is never entered. And this will always be the case, or am I wrong?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct but it would revert the fix introduced in 2a22c32

//! Reissue the pending command and its callback.
unprotected_send(commands.front().command, commands.front().callback);

Expand Down