Asterisk
/
asterisk
mirror of https://github.com/asterisk/asterisk
1
0
Fork 0
Code Issues Releases Wiki Activity

channelstorage_cpp_map_name_id.cc: Refactor iterators for thread-safety.

Browse Source 
The fact that deleting an object from a map invalidates any iterator
that happens to currently point to that object was overlooked in the initial
implementation.  Unfortunately, there's no way to detect that an iterator
has been invalidated so the result was an occasional SEGV triggered by modules
like app_chanspy that opens an iterator and can keep it open for a long period
of time.  The new implementation doesn't keep the underlying C++ iterator
open across calls to ast_channel_iterator_next() and uses a read lock
on the map to ensure that, even for the few microseconds we use the
iterator, another thread can't delete a channel from under it.  Even with
this change, the iterators are still WAY faster than the ao2_legacy
storage driver.

Full details about the new implementation are located in the comments for
iterator_next() in channelstorage_cpp_map_name_id.cc.

Resolves: #1309
pull/1404/head
George Joseph 2 months ago
parent 581e4a23a8
commit 830dd4656c
2 changed files with 196 additions and 134 deletions
Show Stats Download Patch File Download Diff File

70
main/channelstorage.c
Unescape View File

@ -277,76 +277,6 @@ static void *test_storage_thread(void *data)
elapsed = ast_tvdiff_us(end, start);
ast_test_status_update(test, "%*s: %8ld\n", collen, "by context/exten", elapsed);
#if 0
start = ast_tvnow();
for (i = 0; i < CHANNEL_COUNT; i++) {
sprintf(search1, "TestChannel-%ld-%04d-something", rand, i);
mock_channel = CHANNELSTORAGE_API(storage_instance, get_by_name_or_uniqueid, search1);
ast_test_validate_cleanup(test, mock_channel, res, done);
CHANNELSTORAGE_API(storage_instance, wrlock);
sprintf(mock_channel->context, "TestXXContext-%ld-%04d", rand, i);
sprintf(search1, "TestContext-%ld-%04d", rand, i);
rc = CHANNELSTORAGE_API(storage_instance, update, mock_channel,
AST_CHANNELSTORAGE_UPDATE_CONTEXT, search1, mock_channel->context, 0);
ast_test_validate_cleanup(test, rc == 0, res, done);
sprintf(mock_channel->exten, "TestXXExten-%ld-%04d", rand, i);
sprintf(search2, "TestExten-%ld-%04d", rand, i);
rc = CHANNELSTORAGE_API(storage_instance, update, mock_channel,
AST_CHANNELSTORAGE_UPDATE_EXTEN, search2, mock_channel->exten, 0);
CHANNELSTORAGE_API(storage_instance, unlock);
ast_test_validate_cleanup(test, rc == 0, res, done);
ast_channel_unref(mock_channel);
}
end = ast_tvnow();
elapsed = ast_tvdiff_us(end, start);
ast_test_status_update(test, "%*s: %8ld\n", collen, "update", elapsed);
start = ast_tvnow();
for (i = 0; i < CHANNEL_COUNT; i++) {
sprintf(search1, "TestXXContext-%ld-%04d", rand, i);
sprintf(search2, "TestXXExten-%ld-%04d", rand, i);
mock_channel = CHANNELSTORAGE_API(storage_instance, get_by_exten, search2, search1);
ast_test_validate_cleanup(test, mock_channel, res, done);
ast_channel_unref(mock_channel);
}
end = ast_tvnow();
elapsed = ast_tvdiff_us(end, start);
ast_test_status_update(test, "%*s: %8ld\n", collen, "by context/exten2", elapsed);
start = ast_tvnow();
for (i = 0; i < CHANNEL_COUNT; i++) {
sprintf(search1, "TestChannel-%ld-%04d-something", rand, i);
mock_channel = CHANNELSTORAGE_API(storage_instance, get_by_name_or_uniqueid, search1);
ast_test_validate_cleanup(test, mock_channel, res, done);
sprintf(search2, "TestXXChannel-%ld-%04d", rand, i);
rc = CHANNELSTORAGE_API(storage_instance, update, mock_channel,
AST_CHANNELSTORAGE_UPDATE_NAME, search1, search2, 1);
ast_channel_unref(mock_channel);
ast_test_validate_cleanup(test, rc == 0, res, done);
}
end = ast_tvnow();
elapsed = ast_tvdiff_us(end, start);
ast_test_status_update(test, "%*s: %8ld\n", collen, "change name", elapsed);
start = ast_tvnow();
for (i = 0; i < CHANNEL_COUNT; i++) {
sprintf(search1, "TestXXChannel-%ld-%04d", rand, i);
mock_channel = CHANNELSTORAGE_API(storage_instance, get_by_name_or_uniqueid, search1);
ast_test_validate_cleanup_custom(test, mock_channel, res, done,"Channel %s not found\n", search1);
ast_channel_unref(mock_channel);
}
end = ast_tvnow();
elapsed = ast_tvdiff_us(end, start);
ast_test_status_update(test, "%*s: %8ld\n", collen, "by name exact2", elapsed);
#endif
i = 0;
start = ast_tvnow();
iter = CHANNELSTORAGE_API(storage_instance, iterator_all_new);

260
main/channelstorage_cpp_map_name_id.cc
Unescape View File

@ -19,7 +19,6 @@
#include <memory>
#include <string>
#include <map>
#include <unordered_map>
#include <cassert>
#include <utility>
@ -40,16 +39,25 @@ struct mni_channelstorage_driver_pvt {
static void rdlock(struct ast_channelstorage_instance *driver)
{
if (!driver || !driver->lock_handle) {
return;
}
ast_rwlock_rdlock((ast_rwlock_t*)driver->lock_handle);
}
static void wrlock(struct ast_channelstorage_instance *driver)
{
if (!driver || !driver->lock_handle) {
return;
}
ast_rwlock_wrlock((ast_rwlock_t*)driver->lock_handle);
}
static void unlock(struct ast_channelstorage_instance *driver)
{
if (!driver || !driver->lock_handle) {
return;
}
ast_rwlock_unlock((ast_rwlock_t*)driver->lock_handle);
}
@ -167,39 +175,44 @@ enum cpp_map_iterator_type {
};
struct mni_channel_iterator {
ChannelMap::const_iterator it;
ChannelMap::const_iterator it_end;
enum cpp_map_iterator_type it_type;
char *channel_name;
size_t channel_name_len;
std::string l_name;
size_t l_name_len;
char *context;
char *exten;
std::string last_channel;
int counter;
mni_channel_iterator(ChannelMap::const_iterator it,
ChannelMap::const_iterator it_end, char *name, size_t name_len)
: it(it), it_end(it_end), it_type(ITERATOR_BY_NAME), channel_name(name), channel_name_len(name_len),
context(NULL), exten(NULL)
mni_channel_iterator() :
it_type(ITERATOR_ALL), l_name(""), l_name_len(0),
context(NULL), exten(NULL),
last_channel(""), counter(0)
{
}
mni_channel_iterator(ChannelMap::const_iterator it,
ChannelMap::const_iterator it_end, char *context, char *exten)
: it(it), it_end(it_end), it_type(ITERATOR_BY_EXTEN), channel_name(NULL), channel_name_len(0),
context(context), exten(exten)
mni_channel_iterator(const char *l_name) :
it_type(ITERATOR_BY_NAME), l_name(l_name), l_name_len(strlen(l_name)),
context(NULL), exten(NULL),
last_channel(""), counter(0)
{
}
mni_channel_iterator(ChannelMap::const_iterator it, ChannelMap::const_iterator it_end)
: it(it), it_end(it_end), it_type(ITERATOR_ALL), channel_name(NULL), channel_name_len(0),
context(NULL), exten(NULL)
mni_channel_iterator(const char *context, const char *exten) :
it_type(ITERATOR_BY_EXTEN), l_name(""), l_name_len(0),
context(ast_strdup(context)), exten(ast_strdup(exten)),
last_channel(""), counter(0)
{
}
~mni_channel_iterator()
{
ast_free(channel_name);
ast_free(context);
ast_free(exten);
context = NULL;
exten = NULL;
l_name.clear();
last_channel.clear();
counter = 0;
}
};
@ -207,69 +220,192 @@ static struct ast_channel_iterator *iterator_destroy(struct ast_channelstorage_i
struct ast_channel_iterator *ai)
{
struct mni_channel_iterator *i = (struct mni_channel_iterator *)ai;
if (!driver || !i) {
return NULL;
}
delete i;
return NULL;
}
/*!
* \internal
* \brief Create a new iterator for all channels
*
* No I/O is done at this time. It's simply allocating the iterator
* structure and initializing it.
*
* \return struct mni_channel_iterator *
*/
static struct ast_channel_iterator *iterator_all_new(struct ast_channelstorage_instance *driver)
{
struct mni_channel_iterator *i = new mni_channel_iterator(
getdb(driver).begin(), getdb(driver).end());
struct mni_channel_iterator *i = new mni_channel_iterator();
if (!i) {
return NULL;
}
if (i->it == getdb(driver).end()) {
delete i;
return NULL;
}
return (struct ast_channel_iterator *)i;
}
/*!
* \internal
* \brief Retrieve the next channel in the iterator.
*
* This function retrieves the next channel in the iterator, based on the
* type of iterator it is. If there are no more channels, it returns NULL.
*
* In a single-threaded environment, we'd simply use the std::map
* begin(), end(), lower_bound() and upper_bound() functions and use
* standard iterator operations to move through the map. This doesn't
* work well in a multi-threaded environment where deletes can happen
* in another thread because if you delete the object an iterator points
* to, it becomes invalid and there's no way to test that. If you try
* to access or operate on that iterator (like incrementing it), the
* result will be a SEGV or other undefined behavior.
*
* app_chanspy is particularly prone to triggering this issue because
* it opens an iterator and keeps it open for a long period of time
* looking for channels to spy on.
*
* The solution is to use a C++ iterator to find the next (or first)
* channel then save that channel's key in our iterator structure to
* use as the starting point the next time iterator_next() is called.
* We also put a read lock on the driver to prevent a driver from
* deleting a channel in the short time we use it. We NEVER keep
* C++ iterators across multiple calls to iterator_next().
*
* This sounds inefficient but in practice, it works very well
* because the C++ map is implemented as a red-black tree. This
* makes calling lower_bound() very efficient. Besides, even with
* this approach, the iterators are still at least an order of
* magnitude, and sometimes two orders, faster than the ao2_legacy
* driver. To check the results for yourself, build in development
* mode and run "test execute category /main/channelstorage/"
* from the CLI.
*
* \return struct ast_channel * or NULL
*/
static struct ast_channel *iterator_next(struct ast_channelstorage_instance *driver,
struct ast_channel_iterator *ai)
{
struct mni_channel_iterator *i = (struct mni_channel_iterator *)ai;
struct ast_channel *chan = NULL;
ChannelMap::const_iterator it;
if (i->it == i->it_end) {
if (!driver || !i) {
return NULL;
}
if (i->it_type == ITERATOR_ALL) {
chan = ao2_bump(i->it->second);
++i->it;
return chan;
i->counter++;
rdlock(driver);
if (i->counter == 1) {
/*
* When this is the first call to iterator_next(),
* lower_bound(i->l_name) will return the first
* channel in the map if i->l_name is empty
* (ITERATOR_ALL and ITERATOR_BY_EXTEN) or the
* first channel whose name starts with i->l_name
* (ITERATOR_BY_NAME). This is exactly what we want.
*/
it = getdb(driver).lower_bound(i->l_name);
} else {
/*
* When this is not the first call to iterator_next(),
* we want to return the next channel after the last
* channel returned. We can do this by using the
* last_channel key stored in the iterator to get
* an iterator to directly to it, then advancing it.
* It's possible that last_channel was actually the
* last channel in the map and was deleted between the
* last call to iterator_next() and now so we need to
* check that it's still around before we try to advance it.
*/
it = getdb(driver).lower_bound(i->last_channel);
if (it == getdb(driver).end()) {
unlock(driver);
return NULL;
}
std::advance(it, 1);
}
if (i->it_type == ITERATOR_BY_NAME) {
chan = ao2_bump(i->it->second);
++i->it;
return chan;
/*
* Whether this is the first call to iterator_next() or
* a subsequent call, if we reached the end of the map,
* return NULL.
*/
if (it == getdb(driver).end()) {
unlock(driver);
return NULL;
}
/* ITERATOR_BY_EXTEN */
while (i->it != i->it_end) {
int ret = channelstorage_exten_cb(i->it->second, i->context, i->exten, 0);
if (ret & CMP_MATCH) {
chan = ao2_bump(i->it->second);
++i->it;
return chan;
if (i->it_type == ITERATOR_ALL) {
/*
* The simplest case. Save the channel key to last_channel
* and bump and return the channel.
*/
i->last_channel = it->first;
chan = ao2_bump(it->second);
} else if (i->it_type == ITERATOR_BY_NAME) {
/*
* If this was a search by name, we need to check that
* the channel key still matches the name being searched for.
* If it does, save the channel key to last_channel and bump
* and return the channel.
* If it doesn't match, we're done because the map is sorted
* by channel name so any further channels in the map won't
* match either.
*/
if (it->first.substr(0, i->l_name_len) == i->l_name) {
i->last_channel = it->first;
chan = ao2_bump(it->second);
}
} else if (i->it_type == ITERATOR_BY_EXTEN) {
/*
* Searching by context and extension is a bit more complex.
* Every time iterator_next() is called, we need to search for
* matching context and extension from the last_channel forward
* to the end of the map. It's f'ugly and we have to hold
* the read lock while we traverse but it works, it's safe,
* and it's STILL better than the ao2_legacy driver albeit not
* by much.
*/
while (it != getdb(driver).end()) {
int ret = channelstorage_exten_cb(it->second, i->context, i->exten, 0);
if (ret & CMP_MATCH) {
i->last_channel = it->first;
chan = ao2_bump(it->second);
break;
}
std::advance(it, 1);
}
++i->it;
} else {
ast_log(LOG_ERROR, "Unknown iterator type %d\n", i->it_type);
}
unlock(driver);
return NULL;
return chan;
}
static struct ast_channel_iterator *iterator_by_name_new(struct ast_channelstorage_instance *driver,
/*!
* \internal
* \brief Create a new iterator for retrieving all channels matching
* a specific name prefix. A full channel name can be supplied but calling
* get_by_name_exact() is more efficient for that.
*
* No I/O is done at this time. It's simply allocating the iterator
* structure and initializing it.
*
* \return struct mni_channel_iterator *
*/
static struct ast_channel_iterator *iterator_by_name_new(
struct ast_channelstorage_instance *driver,
const char *name, size_t name_len)
{
char *l_name = NULL;
char *u_name = NULL;
struct mni_channel_iterator *i;
size_t new_name_len = 0;
if (ast_strlen_zero(name)) {
return NULL;
@ -280,37 +416,33 @@ static struct ast_channel_iterator *iterator_by_name_new(struct ast_channelstora
name_len = strlen(name);
}
l_name[name_len] = '\0';
new_name_len = strlen(l_name);
u_name = (char *)ast_alloca(new_name_len + 2);
sprintf(u_name, "%s%c", l_name, '\xFF');
i = new mni_channel_iterator(getdb(driver).lower_bound(l_name),
getdb(driver).upper_bound(u_name));
i = new mni_channel_iterator(l_name);
if (!i) {
return NULL;
}
if (i->it == getdb(driver).end()) {
delete i;
return NULL;
}
return (struct ast_channel_iterator *)i;
}
/*!
* \internal
* \brief Create a new iterator for retrieving all channels
* matching a specific context and optionally exten.
*
* No I/O is done at this time. It's simply allocating the iterator
* structure and initializing it.
*
* \return struct mni_channel_iterator *
*/
static struct ast_channel_iterator *iterator_by_exten_new(struct ast_channelstorage_instance *driver,
const char *exten, const char *context)
{
struct mni_channel_iterator *i =
new mni_channel_iterator(getdb(driver).begin(),
getdb(driver).end(),
ast_str_to_lower(ast_strdup(context)), ast_str_to_lower(ast_strdup(exten)));
if (!i) {
return NULL;
}
struct mni_channel_iterator *i = new mni_channel_iterator(
ast_str_to_lower(ast_strdupa(context)),
ast_str_to_lower(ast_strdupa(exten)));
if (i->it == getdb(driver).end()) {
delete i;
if (!i) {
return NULL;
}

Write Preview
Loading…
Cancel
Save