Skip to content

Commit 5c44278

Browse files
suifengersan123随风而散
suifengersan123
and
随风而散
authored
fix bug UAF (#5720)
* fix bug UAF Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> * fix linting problems Signed-off-by: suifengersan123 <[email protected]> * add test Signed-off-by: suifengersan123 <[email protected]> * fix errors Signed-off-by: suifengersan123 <[email protected]> --------- Signed-off-by: suifengersan123 <[email protected]> Co-authored-by: 随风而散 <[email protected]>
1 parent 91d926a commit 5c44278

File tree

3 files changed

+137
-8
lines changed

3 files changed

+137
-8
lines changed

‎nav2_dwb_controller/dwb_plugins/include/dwb_plugins/kinematic_parameters.hpp‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
#include <memory>
4040
#include <string>
4141
#include <vector>
42-
42+
#include <stdexcept>
4343
#include "nav2_ros_common/lifecycle_node.hpp"
4444
#include "rclcpp/rclcpp.hpp"
4545

@@ -145,7 +145,14 @@ class KinematicsHandler
145145
void activate();
146146
void deactivate();
147147

148-
inline KinematicParameters getKinematics() {return *kinematics_.load();}
148+
inline KinematicParameters getKinematics()
149+
{
150+
KinematicParameters * ptr = kinematics_.load();
151+
if (ptr == nullptr) {
152+
throw std::runtime_error("Can't call KinematicsHandler::getKinematics().");
153+
}
154+
return *ptr;
155+
}
149156

150157
void setSpeedLimit(const double & speed_limit, const bool & percentage);
151158

‎nav2_dwb_controller/dwb_plugins/src/kinematic_parameters.cpp‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
*/
3434

3535
#include "dwb_plugins/kinematic_parameters.hpp"
36-
36+
#include <atomic>
3737
#include <memory>
3838
#include <string>
3939
#include <vector>
@@ -55,7 +55,10 @@ KinematicsHandler::KinematicsHandler()
5555

5656
KinematicsHandler::~KinematicsHandler()
5757
{
58-
delete kinematics_.load();
58+
KinematicParameters * ptr = kinematics_.load();
59+
if (ptr != nullptr) {
60+
delete ptr;
61+
}
5962
}
6063

6164
void KinematicsHandler::initialize(
@@ -151,7 +154,11 @@ void KinematicsHandler::deactivate()
151154
void KinematicsHandler::setSpeedLimit(
152155
const double & speed_limit, const bool & percentage)
153156
{
154-
KinematicParameters kinematics(*kinematics_.load());
157+
KinematicParameters * ptr = kinematics_.load();
158+
if (ptr == nullptr) {
159+
return; // Nothing to update
160+
}
161+
KinematicParameters kinematics(*ptr);
155162

156163
if (speed_limit == nav2_costmap_2d::NO_SPEED_LIMIT) {
157164
// Restore default value
@@ -232,7 +239,11 @@ void
232239
KinematicsHandler::updateParametersCallback(const std::vector<rclcpp::Parameter> & parameters)
233240
{
234241
rcl_interfaces::msg::SetParametersResult result;
235-
KinematicParameters kinematics(*kinematics_.load());
242+
KinematicParameters * ptr = kinematics_.load();
243+
if (ptr == nullptr) {
244+
return; // Nothing to update
245+
}
246+
KinematicParameters kinematics(*ptr);
236247

237248
for (const auto & parameter : parameters) {
238249
const auto & param_type = parameter.get_type();
@@ -284,8 +295,11 @@ KinematicsHandler::updateParametersCallback(const std::vector<rclcpp::Parameter>
284295

285296
void KinematicsHandler::update_kinematics(KinematicParameters kinematics)
286297
{
287-
delete kinematics_.load();
288-
kinematics_.store(new KinematicParameters(kinematics));
298+
KinematicParameters * new_kinematics = new KinematicParameters(kinematics);
299+
KinematicParameters * old_kinematics = kinematics_.exchange(new_kinematics);
300+
if (old_kinematics != nullptr) {
301+
delete old_kinematics;
302+
}
289303
}
290304

291305
} // namespace dwb_plugins

‎nav2_dwb_controller/dwb_plugins/test/kinematic_parameters_test.cpp‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,114 @@ TEST(KinematicParameters, SetAllParameters) {
128128
EXPECT_EQ(kp.getAccX(), 56.78);
129129
}
130130

131+
// Test null pointer handling in setSpeedLimit
132+
TEST(KinematicParameters, SetSpeedLimitNullPointerHandling) {
133+
class TestableKinematicsHandler : public dwb_plugins::KinematicsHandler
134+
{
135+
public:
136+
void simulateNullKinematics()
137+
{
138+
// Temporarily set kinematics_ to null to test null pointer handling
139+
dwb_plugins::KinematicParameters * old_ptr = kinematics_.exchange(nullptr);
140+
delete old_ptr;
141+
}
142+
143+
void restoreKinematics()
144+
{
145+
// Restore a valid kinematics pointer
146+
kinematics_.store(new dwb_plugins::KinematicParameters);
147+
}
148+
};
149+
150+
std::string nodeName = "test_node_null";
151+
auto node = std::make_shared<nav2::LifecycleNode>(nodeName);
152+
TestableKinematicsHandler kh;
153+
kh.initialize(node, nodeName);
154+
kh.activate();
155+
156+
// Simulate null pointer scenario
157+
kh.simulateNullKinematics();
158+
159+
// This should not crash - it should handle the null pointer gracefully
160+
EXPECT_NO_THROW(kh.setSpeedLimit(10.0, true));
161+
EXPECT_NO_THROW(kh.setSpeedLimit(5.0, false));
162+
163+
// Restore for cleanup
164+
kh.restoreKinematics();
165+
}
166+
167+
// Test null pointer handling in updateParametersCallback
168+
TEST(KinematicParameters, UpdateParametersNullPointerHandling) {
169+
class TestableKinematicsHandler : public dwb_plugins::KinematicsHandler
170+
{
171+
public:
172+
void simulateNullKinematics()
173+
{
174+
// Temporarily set kinematics_ to null to test null pointer handling
175+
dwb_plugins::KinematicParameters * old_ptr = kinematics_.exchange(nullptr);
176+
delete old_ptr;
177+
}
178+
179+
void restoreKinematics()
180+
{
181+
// Restore a valid kinematics pointer
182+
kinematics_.store(new dwb_plugins::KinematicParameters);
183+
}
184+
185+
// Expose protected method for testing
186+
void testUpdateParametersCallback(std::vector<rclcpp::Parameter> params)
187+
{
188+
updateParametersCallback(params);
189+
}
190+
};
191+
192+
std::string nodeName = "test_node_update_null";
193+
auto node = std::make_shared<nav2::LifecycleNode>(nodeName);
194+
TestableKinematicsHandler kh;
195+
kh.initialize(node, nodeName);
196+
kh.activate();
197+
198+
// Simulate null pointer scenario
199+
kh.simulateNullKinematics();
200+
201+
// Create test parameters
202+
std::vector<rclcpp::Parameter> test_params = {
203+
rclcpp::Parameter(nodeName + ".max_vel_x", 10.0),
204+
rclcpp::Parameter(nodeName + ".max_vel_y", 20.0)
205+
};
206+
207+
// This should not crash - it should handle the null pointer gracefully
208+
EXPECT_NO_THROW(kh.testUpdateParametersCallback(test_params));
209+
210+
// Restore for cleanup
211+
kh.restoreKinematics();
212+
}
213+
214+
// Test getKinematics throws exception when pointer is null
215+
TEST(KinematicParameters, GetKinematicsNullPointerHandling) {
216+
class TestableKinematicsHandler : public dwb_plugins::KinematicsHandler
217+
{
218+
public:
219+
void simulateNullKinematics()
220+
{
221+
// Temporarily set kinematics_ to null
222+
dwb_plugins::KinematicParameters * old_ptr = kinematics_.exchange(nullptr);
223+
delete old_ptr;
224+
}
225+
};
226+
227+
std::string nodeName = "test_node_get_null";
228+
auto node = std::make_shared<nav2::LifecycleNode>(nodeName);
229+
TestableKinematicsHandler kh;
230+
kh.initialize(node, nodeName);
231+
232+
// Simulate null pointer scenario
233+
kh.simulateNullKinematics();
234+
235+
// getKinematics should throw when pointer is null
236+
EXPECT_THROW(kh.getKinematics(), std::runtime_error);
237+
}
238+
131239

132240
int main(int argc, char ** argv)
133241
{

0 commit comments

Comments
 (0)