Skip to content

fix: keep ADS handles alive and update on_init signature #8

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.

Sign up for GitHub

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

Open
mfabregat wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

fix: keep ADS handles alive and update on_init signature #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...are_interface/include/beckhoff_ads_hardware_interface/beckhoff_ads_hardware_interface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <string>
#include <vector>
#include <limits>
#include <optional>

#include "hardware_interface/system_interface.hpp"
#include "hardware_interface/handle.hpp"
Expand Down Expand Up @@ -51,6 +52,7 @@ namespace beckhoff_ads_hardware_interface
// Configured from yaml
std::string plc_name_symbolic; // e.g., "MAIN.Joint_Pos_State". Used to get the handle.
PLCType plc_type;
std::optional<AdsHandle> ads_handle_ref; // Keep handle alive; destructor releases it.

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use-after-free: on_shutdown() destroys the AdsDevice while the retained handles still point at it.

AdsHandle is std::unique_ptr<uint32_t, ResourceDeleter<uint32_t>>, and AdsDevice::GetHandle() builds the deleter as std::bind(&AdsDevice::DeleteSymbolHandle, this, _1) - i.e. it stores a raw AdsDevice const*. Now that this PR keeps those handles alive inside ADSDataLayout, their lifetime is tied to ads_device_.

on_shutdown() (src line ~712) does:

if (ads_device_) { ads_device_.reset(); }

while ads_item_layouts_read_ / ads_item_layouts_write_ still own ads_handle_ref objects. When the hardware component is later destroyed, every ~unique_ptr calls DeleteSymbolHandle on the freed AdsDevice -> reads m_LocalPort/m_Addr out of freed memory and sends a bogus ADS request, or segfaults.

(Note the member declaration order currently saves the implicit destruction path only by luck: ads_device_ is declared before the layout vectors, so the layouts are destroyed first. The explicit reset() defeats that.)

Fix: release the handles before dropping the device, e.g. in on_shutdown():

for (auto & l : ads_item_layouts_read_)  { l.ads_handle_ref.reset(); }
for (auto & l : ads_item_layouts_write_) { l.ads_handle_ref.reset(); }
ads_device_.reset();

uint32_t ads_handle; // PLC Handle for the symbolic name. Not using AdsHandle, as we don't need a shared ptr, just a value to paste in the message

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ads_handle is left uninitialized when GetHandle() throws, and the failure is swallowed.

uint32_t ads_handle; has no initializer. In on_configure() (src lines 53-76) a GetHandle() failure is caught, logged, and the loop continues - layout.ads_handle is never assigned, so it keeps indeterminate stack/heap garbage. build_sum_read_buffers() / build_sum_write_buffers() then copy it straight into header.indexOffset, so the driver issues ADSIGRP_SYM_VALBYHND requests against a random handle, and on_configure() still returns SUCCESS.

Two fixes needed:

  1. uint32_t ads_handle = 0; (or drop the field entirely, see below).
  2. Make the handle-acquisition failure fatal - return CallbackReturn::ERROR; in the catch, instead of only logging "Read/Write operations for this variable will fail."
Suggested change
uint32_t ads_handle; // PLC Handle for the symbolic name. Not using AdsHandle, as we don't need a shared ptr, just a value to paste in the message
uint32_t ads_handle = 0; // Cached value of *ads_handle_ref, for the hot path

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment. The trailing comment still says "Not using AdsHandle, as we don't need a shared ptr, just a value to paste in the message", but the line directly above now stores exactly that AdsHandle. It reads as if the two fields contradict each other.

Also worth considering: ads_handle is now fully derivable from ads_handle_ref (*ads_handle_ref.value()), i.e. duplicated state that can silently drift (it already does - see the uninitialized-on-throw case). Since the value is only read in build_sum_*_buffers() and in one log line in write(), not in the per-cycle hot path (the handles are baked into the pre-packed request buffers), keeping the cached copy buys nothing. Dropping it and reading *layout.ads_handle_ref.value() at the two build sites removes the whole class of desync bugs.


size_t num_elements; // 6 for LREAL[6], 1 for single LREAL/BOOL etc.
Expand Down Expand Up @@ -98,7 +100,7 @@ namespace beckhoff_ads_hardware_interface
class BeckhoffADSHardwareInterface : public hardware_interface::SystemInterface
{
public:
hardware_interface::CallbackReturn on_init(const hardware_interface::HardwareComponentParams &params);
hardware_interface::CallbackReturn on_init(const hardware_interface::HardwareComponentInterfaceParams &params) override;

hardware_interface::CallbackReturn on_configure(
const rclcpp_lifecycle::State &previous_state) override;
Expand Down
24 changes: 16 additions & 8 deletions beckhoff_ads_hardware_interface/src/beckhoff_ads_hardware_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <vector>
#include <cstdint>
#include <algorithm> // std::transform
#include <utility>

#include "beckhoff_ads_hardware_interface/beckhoff_ads_hardware_interface.hpp"
#include "hardware_interface/types/hardware_interface_type_values.hpp"
Expand All @@ -21,8 +22,13 @@
namespace beckhoff_ads_hardware_interface
{
hardware_interface::CallbackReturn BeckhoffADSHardwareInterface::on_init(
const hardware_interface::HardwareComponentParams & /*params*/)
const hardware_interface::HardwareComponentInterfaceParams &params)
{
if (hardware_interface::SystemInterface::on_init(params) != CallbackReturn::SUCCESS)

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_sum_write_buffers() runs before the command->state interface linking loop, so the NaN fallback is dead code.

Order inside on_configure():

  1. line ~85 build_sum_write_buffers() - reads layout.state_command_interfaces_map_ at line ~212-215 to fill write_instruction.fallback_state_interface_name.
  2. line ~93-107 - the loop that actually populates command_layout.state_command_interfaces_map_.

At step 1 the map is always empty, so fallback_state_interface_name is always "" for every write instruction. Consequence: in write(), when a controller stops publishing and get_command() returns NaN, the if (!fallback_state_interface_name.empty()) branch is never taken, so the intended "mirror the state interface" behaviour never happens and the stale last-valid command is re-sent forever.

Fix: move the linking loop (lines ~92-107) to before build_sum_write_buffers().

{

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this collapses the base class's CallbackReturn::FAILURE into CallbackReturn::ERROR. FAILURE means "can be retried", ERROR routes to on_error. Forwarding the base result verbatim is more faithful:

Suggested change
{
const auto ret = hardware_interface::SystemInterface::on_init(params);
if (ret != CallbackReturn::SUCCESS)
{
return ret;
}

return CallbackReturn::ERROR;
}

logging_throttle_clock_ = std::make_shared<rclcpp::Clock>(RCL_STEADY_TIME);

return CallbackReturn::SUCCESS;
Expand All @@ -48,7 +54,8 @@ namespace beckhoff_ads_hardware_interface
{
try
{
layout.ads_handle = *(ads_device_->GetHandle(layout.plc_name_symbolic));
layout.ads_handle_ref.emplace(ads_device_->GetHandle(layout.plc_name_symbolic));

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use-after-free on re-configure (cleanup -> configure).

on_configure() calls configure_ads_device() first, which does ads_device_ = std::make_unique<AdsDevice>(...) (src line ~749). That assignment destroys the previous AdsDevice while the ads_handle_ref objects from the previous configure cycle are still alive in ads_item_layouts_read_/ads_item_layouts_write_.

Two lines later ads_read_layout_configure() does ads_item_layouts_read_.clear(), which runs each retained handle's deleter - std::bind(&AdsDevice::DeleteSymbolHandle, <freed device>, _1). Use-after-free.

on_cleanup() is not overridden, so nothing releases the handles between cycles, and the lifecycle manager does allow configure -> cleanup -> configure.

Fix: clear the layout vectors (or at least reset() every ads_handle_ref) before configure_ads_device() replaces ads_device_, and/or implement on_cleanup() to release handles then the device.

layout.ads_handle = *layout.ads_handle_ref.value();

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ads_read_instructions_ / ads_write_instructions_ are never cleared - they accumulate across configure cycles.

ads_read_layout_configure() / ads_write_layout_configure() clear ads_item_layouts_read_/write_, but build_sum_read_buffers() (line ~158) and build_sum_write_buffers() (line ~218) only push_back into the instruction vectors.

On a second on_configure() (cleanup -> configure, or a URDF/param change) the instruction vectors contain both the old and the new entries. read() iterates ads_read_instructions_ and memcpys at read_buffer_offset_error_code / read_buffer_offset_data computed against the old buffer sizes. If the new configuration is smaller, those offsets are past the end of ads_buffer_sum_read_response_ -> heap out-of-bounds read; if larger, you get duplicate/garbage set_state() calls.

Fix: ads_read_instructions_.clear(); at the top of build_sum_read_buffers() and ads_write_instructions_.clear(); at the top of build_sum_write_buffers() (note both also need to clear before the early return true when there are 0 items).

}

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unchecked std::map::find() dereference in the interface-linking loop (src line ~102).

for (size_t k = 0; k < command_layout.num_elements; ++k)
{
    auto pair = std::make_pair(command_layout.ros2_interfaces_.find(k)->second,
                               state_layout.ros2_interfaces_.find(k)->second);

num_elements comes from the n_elements URDF parameter of the first interface seen for that PLC symbol, while ros2_interfaces_ is keyed by each interface's index parameter. Nothing guarantees the keys are exactly 0 .. num_elements-1, and nothing guarantees the read layout and the write layout expose the same indices.

Concrete failure: MAIN.Joint_Cmd declared as n_elements=6 but only 3 command interfaces (index 0,1,2) present, or a state interface set that covers indices 0-5 while the command set covers 0-2. At k == 3 find(3) returns end() and ->second dereferences the map's end sentinel -> undefined behaviour / crash during on_configure().

Fix: look the iterators up, skip (and log) when either is end().

catch (const std::exception &ex)

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unchecked find() dereference in build_sum_write_buffers() (src line ~214).

if (!layout.state_command_interfaces_map_.empty())
{
    write_instruction.fallback_state_interface_name =
        layout.state_command_interfaces_map_.find(interface_name)->second;
}

The empty() guard only proves some entry exists, not that interface_name is one of them. The linking loop only inserts keys for k < num_elements found in ros2_interfaces_, so a command interface whose index is outside that range is present in ros2_interfaces_ but absent from state_command_interfaces_map_ -> end() dereference.

This is currently masked because the map is always empty here (see the ordering bug on on_configure()), so fixing that ordering will turn this into a live crash. Please guard with auto it = ...find(name); if (it != ...end()) in the same change.

{

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_sum_read_buffers() / build_sum_write_buffers() can never return false.

Both functions return true on every path, so the if (!build_sum_read_buffers()) { RCLCPP_FATAL(...); return ERROR; } guards here are dead code and on_configure() reports SUCCESS even when handles failed to resolve or a layout has plc_element_byte_size == 0 (which plcTypeByteSize() returns for UNKNOWN).

Either make them validate (missing/zero handle, zero element size, num_elements == 0) and return false, or change the return type to void so the dead error path is not misleading.

Expand All @@ -59,7 +66,8 @@ namespace beckhoff_ads_hardware_interface
{
try
{
layout.ads_handle = *(ads_device_->GetHandle(layout.plc_name_symbolic));
layout.ads_handle_ref.emplace(ads_device_->GetHandle(layout.plc_name_symbolic));

@MatthijsBurgh MatthijsBurgh Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated handle-acquisition loops, and duplicate handles for shared symbols.

The read loop (lines 53-64) and this write loop (lines 65-76) are byte-identical apart from the words "Read"/"Write" in the log message. A single helper taking the vector and a label would remove the copy-paste - and would make it a one-line change to add the missing return CallbackReturn::ERROR on failure.

Separately: a PLC symbol that has both a state and a command interface now gets two ADS symbol handles allocated on the PLC (one from each loop), each held for the lifetime of the component. Previously the extra handle was released immediately by the temporary's destructor, so this PR doubles the handle footprint on the PLC for every read/write symbol. Consider resolving each distinct plc_name_symbolic once into a shared map and reusing the value.

layout.ads_handle = *layout.ads_handle_ref.value();
}
catch (const std::exception &ex)
{
Expand Down Expand Up @@ -276,7 +284,7 @@ namespace beckhoff_ads_hardware_interface
else
{
layout.plc_element_byte_size = plcTypeByteSize(layout.plc_type);
ads_item_layouts_read_.push_back(layout);
ads_item_layouts_read_.push_back(std::move(layout));
processed_plc_symbols[plc_symbol] = true;
}
}
Expand All @@ -285,7 +293,7 @@ namespace beckhoff_ads_hardware_interface
{
// Find the ADS Data Layout object of the corresponding PLC symbol
auto it = std::find_if(ads_item_layouts_read_.begin(), ads_item_layouts_read_.end(),
[&plc_symbol](ADSDataLayout layout)
[&plc_symbol](const ADSDataLayout &layout)
{ return layout.plc_name_symbolic == plc_symbol; });

// Add the interface name the layout
Expand Down Expand Up @@ -374,7 +382,7 @@ namespace beckhoff_ads_hardware_interface
else
{
layout.plc_element_byte_size = plcTypeByteSize(layout.plc_type);
ads_item_layouts_write_.push_back(layout);
ads_item_layouts_write_.push_back(std::move(layout));
processed_plc_symbols[plc_symbol] = true;
}
}
Expand All @@ -383,7 +391,7 @@ namespace beckhoff_ads_hardware_interface
{
// Look for the ADS Data Layout of the corresponding PLC symbol
auto it = std::find_if(ads_item_layouts_write_.begin(), ads_item_layouts_write_.end(),
[&plc_symbol](ADSDataLayout layout)
[&plc_symbol](const ADSDataLayout &layout)
{ return layout.plc_name_symbolic == plc_symbol; });

// Add the command interface name the layout
Expand Down Expand Up @@ -836,4 +844,4 @@ namespace beckhoff_ads_hardware_interface
#include "pluginlib/class_list_macros.hpp"

PLUGINLIB_EXPORT_CLASS(
beckhoff_ads_hardware_interface::BeckhoffADSHardwareInterface, hardware_interface::SystemInterface)
beckhoff_ads_hardware_interface::BeckhoffADSHardwareInterface, hardware_interface::SystemInterface)