-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix bug UAF #5720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix bug UAF #5720
Conversation
| KinematicParameters* ptr = kinematics_.load(); | ||
| // Check for nullptr before dereferencing | ||
| if (ptr == nullptr) { | ||
| throw std::runtime_error( | ||
| "KinematicsHandler::getKinematics() called before kinematics_ is initialized"); | ||
| } | ||
| return *ptr; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intends here are incorrect for the code styping
| if (ptr != nullptr) { | ||
| delete ptr; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this indented?
| KinematicParameters* ptr = kinematics_.load(); | ||
| if (ptr == nullptr) { | ||
| return; // Nothing to update | ||
| } | ||
| KinematicParameters kinematics(*ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALl this is incorrect too
| KinematicParameters* ptr = kinematics_.load(); | ||
| if (ptr == nullptr) { | ||
| return; // Nothing to update | ||
| } | ||
| KinematicParameters kinematics(*ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| KinematicParameters* new_kinematics = new KinematicParameters(kinematics); | ||
| KinematicParameters* old_kinematics = kinematics_.exchange(new_kinematics); | ||
|
|
||
| if (old_kinematics != nullptr) { | ||
| delete old_kinematics; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
|
There's some linting problems here that needs resolution - CI also picked up on some of them. Also look at the DCO sign off. |
3bcc781 to
6bdc5ca
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
I have fixed the linting errors and made the commit. Please take your time to check if there are any other issues. |
mini-1235
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suifengersan123 Please add test for the following lines
| rcl_interfaces::msg::SetParametersResult result; | ||
| KinematicParameters kinematics(*kinematics_.load()); | ||
| KinematicParameters * ptr = kinematics_.load(); | ||
| if (ptr == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add test for the line here
| KinematicParameters kinematics(*kinematics_.load()); | ||
| KinematicParameters * ptr = kinematics_.load(); | ||
| if (ptr == nullptr) { | ||
| return; // Nothing to update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
|
@suifengersan123, your PR has failed to build. Please check CI outputs and resolve issues. |
Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
….com> Signed-off-by: suifengersan123 <[email protected]>
Signed-off-by: suifengersan123 <[email protected]>
Signed-off-by: suifengersan123 <[email protected]>
Signed-off-by: suifengersan123 <[email protected]>
Signed-off-by: suifengersan123 <[email protected]>
|
I have added test. Please have a look and let me know if there are any other issues. |
Basic Info
Description of contribution in a few bullet points
I applied the same repair logic from the previous PR (#5707) to the main branch.
For Maintainers:
backport-*.