Skip to content

Commit

Permalink
lcm: Escape regex characters in DrakeLcm channel names (RobotLocomoti…
Browse files Browse the repository at this point in the history
…on#12534)

Drake no longer accidentally supports regexes for channel names.
  • Loading branch information
jwnimmer-tri authored Jan 7, 2020
1 parent b14c725 commit 16ef859
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 5 deletions.
2 changes: 2 additions & 0 deletions lcm/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ drake_cc_library(
deps = [
":interface",
"//common:essential",
"//common:scope_exit",
"@glib",
"@lcm",
],
)
Expand Down
11 changes: 9 additions & 2 deletions lcm/drake_lcm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@
#include <utility>
#include <vector>

#include <glib.h>

#include "drake/common/drake_assert.h"
#include "drake/common/drake_copyable.h"
#include "drake/common/drake_throw.h"
#include "drake/common/scope_exit.h"

namespace drake {
namespace lcm {
Expand Down Expand Up @@ -90,15 +93,19 @@ class DrakeSubscription final : public DrakeSubscriptionInterface {
HandlerFunction handler) {
DRAKE_DEMAND(native_instance != nullptr);

// The argument to subscribeFunction is regex (not a string literal), so
// we'll need to escape the channel name before calling subscribeFunction.
char* const channel_regex = g_regex_escape_string(channel.c_str(), -1);
ScopeExit guard([channel_regex](){ g_free(channel_regex); });

// Create the result.
auto result = std::make_shared<DrakeSubscription>();
result->native_instance_ = native_instance;
result->user_callback_ = std::move(handler);
result->weak_self_reference_ = result;
result->strong_self_reference_ = result;
// TODO(#12523) This should escape `channel` against regex parsing.
result->native_subscription_ = native_instance->subscribeFunction(
channel, &DrakeSubscription::NativeCallback, result.get());
channel_regex, &DrakeSubscription::NativeCallback, result.get());
result->native_subscription_->setQueueCapacity(1);

// Sanity checks. (The use_count will be 2 because both 'result' and
Expand Down
3 changes: 0 additions & 3 deletions lcm/drake_lcm_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ class DrakeLcmInterface {
*
* NOTE: Unlike upstream LCM, DrakeLcm does not support regexes for the
* `channel` argument.
* TODO(#12523) Some implementations may provide this capability, but users
* should not rely on this functionality which will be removed in the
* future.
*
* @param channel The channel to subscribe to.
* Must not be the empty string.
Expand Down
27 changes: 27 additions & 0 deletions lcm/test/drake_lcm_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,33 @@ TEST_F(DrakeLcmTest, AcceptanceTest) {
});
}

// Ensure that regex characters in the channel named are treated as literals,
// not regex directives.
TEST_F(DrakeLcmTest, SubscribeNotRegexTest) {
const std::string channel1 = "DrakeLcmTest_SubscribeNotRegexTest";
const std::string channel2 = "DrakeLcmTest_.*";

Subscriber<lcmt_drake_signal> channel1_subscriber(dut_.get(), channel1);
Subscriber<lcmt_drake_signal> channel2_subscriber(dut_.get(), channel2);

// Publishing one channel is only ever received by its own subscriber.
LoopUntilDone(&channel1_subscriber.message(), 20 /* retries */, [&]() {
Publish(dut_.get(), channel1, message_);
dut_->HandleSubscriptions(50 /* millis */);
});
EXPECT_GE(channel1_subscriber.count(), 0);
EXPECT_EQ(channel2_subscriber.count(), 0);
channel1_subscriber.clear();
channel2_subscriber.clear();

LoopUntilDone(&channel2_subscriber.message(), 20 /* retries */, [&]() {
Publish(dut_.get(), channel2, message_);
dut_->HandleSubscriptions(50 /* millis */);
});
EXPECT_EQ(channel1_subscriber.count(), 0);
EXPECT_GE(channel2_subscriber.count(), 0);
}

// Tests DrakeLcm's unsubscribe.
TEST_F(DrakeLcmTest, UnsubscribeTest) {
const std::string channel_name = "DrakeLcmTest.UnsubscribeTest";
Expand Down

0 comments on commit 16ef859

Please sign in to comment.