Skip to content

Commit 4616170

Browse files
authored
Fix data race (UniversalRobots#498)
I realized that we access data from the primary consumer in a hazardous way: Data is stored in shared pointers, while writing and reading those shared pointers is mutex-protected inside the consumer. However, the public access functions expose those shared pointers directly. This PR instead exposes copies of the buffers upon request.
1 parent b4e4816 commit 4616170

1 file changed

Lines changed: 35 additions & 16 deletions

File tree

include/ur_client_library/primary/primary_consumer.h

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535

3636
#include <functional>
3737
#include <mutex>
38-
#include <condition_variable>
3938

4039
namespace urcl
4140
{
@@ -89,7 +88,7 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
8988
virtual bool consume(VersionMessage& pkg) override
9089
{
9190
std::scoped_lock lock(version_information_mutex_);
92-
version_information_ = std::make_shared<VersionInformation>();
91+
version_information_ = VersionInformation();
9392
version_information_->major = pkg.major_version_;
9493
version_information_->minor = pkg.minor_version_;
9594
version_information_->bugfix = pkg.svn_version_;
@@ -108,7 +107,7 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
108107
{
109108
URCL_LOG_DEBUG("%s", pkg.toString().c_str());
110109
std::scoped_lock lock(kinematics_info_mutex_);
111-
kinematics_info_ = std::make_shared<KinematicsInfo>(pkg);
110+
kinematics_info_ = std::make_unique<KinematicsInfo>(pkg);
112111
return true;
113112
}
114113

@@ -167,22 +166,22 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
167166
{
168167
URCL_LOG_DEBUG("Robot mode is now %s", robotModeString(static_cast<RobotMode>(pkg.robot_mode_)).c_str());
169168
std::scoped_lock lock(robot_mode_mutex_);
170-
robot_mode_ = std::make_shared<RobotModeData>(pkg);
169+
robot_mode_ = std::make_unique<RobotModeData>(pkg);
171170
return true;
172171
}
173172

174173
virtual bool consume(SafetyModeMessage& pkg) override
175174
{
176175
URCL_LOG_DEBUG("Robot safety mode is now %s", safetyModeString(pkg.safety_mode_type_).c_str());
177176
std::scoped_lock lock(safety_mode_mutex_);
178-
safety_mode_ = std::make_shared<SafetyModeMessage>(pkg);
177+
safety_mode_ = std::make_unique<SafetyModeMessage>(pkg);
179178
return true;
180179
}
181180

182181
virtual bool consume(ConfigurationData& pkg) override
183182
{
184183
std::scoped_lock lock(configuration_data_mutex_);
185-
configuration_data_ = std::make_shared<ConfigurationData>(pkg);
184+
configuration_data_ = std::make_unique<ConfigurationData>(pkg);
186185
return true;
187186
}
188187

@@ -205,7 +204,11 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
205204
std::shared_ptr<KinematicsInfo> getKinematicsInfo()
206205
{
207206
std::scoped_lock lock(kinematics_info_mutex_);
208-
return kinematics_info_;
207+
if (kinematics_info_ == nullptr)
208+
{
209+
return nullptr;
210+
}
211+
return std::make_shared<KinematicsInfo>(*kinematics_info_);
209212
}
210213

211214
/*!
@@ -217,7 +220,11 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
217220
std::shared_ptr<RobotModeData> getRobotModeData()
218221
{
219222
std::scoped_lock lock(robot_mode_mutex_);
220-
return robot_mode_;
223+
if (robot_mode_ == nullptr)
224+
{
225+
return nullptr;
226+
}
227+
return std::make_shared<RobotModeData>(*robot_mode_);
221228
}
222229

223230
/*!
@@ -229,7 +236,11 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
229236
std::shared_ptr<SafetyModeMessage> getSafetyModeMessage()
230237
{
231238
std::scoped_lock lock(safety_mode_mutex_);
232-
return safety_mode_;
239+
if (safety_mode_ == nullptr)
240+
{
241+
return nullptr;
242+
}
243+
return std::make_shared<SafetyModeMessage>(*safety_mode_);
233244
}
234245

235246
/*!
@@ -242,7 +253,11 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
242253
std::shared_ptr<VersionInformation> getVersionInformation()
243254
{
244255
std::scoped_lock lock(version_information_mutex_);
245-
return version_information_;
256+
if (!version_information_.has_value())
257+
{
258+
return nullptr;
259+
}
260+
return std::make_shared<VersionInformation>(*version_information_);
246261
}
247262

248263
/*!
@@ -255,21 +270,25 @@ class PrimaryConsumer : public AbstractPrimaryConsumer
255270
std::shared_ptr<ConfigurationData> getConfigurationData()
256271
{
257272
std::scoped_lock lock(configuration_data_mutex_);
258-
return configuration_data_;
273+
if (configuration_data_ == nullptr)
274+
{
275+
return nullptr;
276+
}
277+
return std::make_shared<ConfigurationData>(*configuration_data_);
259278
}
260279

261280
private:
262281
std::function<void(ErrorCode&)> error_code_message_callback_;
263-
std::shared_ptr<KinematicsInfo> kinematics_info_;
264282
std::mutex kinematics_info_mutex_;
283+
std::unique_ptr<KinematicsInfo> kinematics_info_;
265284
std::mutex robot_mode_mutex_;
266-
std::shared_ptr<RobotModeData> robot_mode_;
285+
std::unique_ptr<RobotModeData> robot_mode_;
267286
std::mutex version_information_mutex_;
268-
std::shared_ptr<VersionInformation> version_information_;
269-
std::shared_ptr<ConfigurationData> configuration_data_;
287+
std::optional<VersionInformation> version_information_;
270288
std::mutex configuration_data_mutex_;
289+
std::unique_ptr<ConfigurationData> configuration_data_;
271290
std::mutex safety_mode_mutex_;
272-
std::shared_ptr<SafetyModeMessage> safety_mode_;
291+
std::unique_ptr<SafetyModeMessage> safety_mode_;
273292
};
274293

275294
} // namespace primary_interface

0 commit comments

Comments
 (0)