Skip to content

Reset string array size when allocation fails in init - #593

Merged
ahcorde merged 1 commit into
ros2:rollingfrom
Dev-next-gen:fix/string-array-init-size-on-alloc-failure
Sep 15, 2026
Merged

ahcorde merged 1 commit into
ros2:rollingfrom
Dev-next-gen:fix/string-array-init-size-on-alloc-failure

Conversation

@Dev-next-gen

@Dev-next-gen Dev-next-gen commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Description

rcutils_string_array_init() writes the requested size into the array before it tries to allocate:

string_array->size = size;
string_array->data = allocator->zero_allocate(size, sizeof(char *), allocator->state);
if (NULL == string_array->data && 0 != size) {
  RCUTILS_SET_ERROR_MSG("failed to allocate string array");
  return RCUTILS_RET_BAD_ALLOC;
}

When the allocation fails, the function returns RCUTILS_RET_BAD_ALLOC but leaves size set to the requested count while data stays NULL. The array then advertises entries that cannot be read. A caller that checks the return value is fine, but one that inspects the array afterwards — or passes it to code that iterates 0..size — walks a null pointer.

This PR sets size back to 0 on that error path, so a failed init leaves the array exactly as rcutils_get_zero_initialized_string_array() would.

Fixes # (no issue filed; found while reading the allocation failure paths)

Is this user-facing behavior change?

Only on the failure path. On success nothing changes. After a failed rcutils_string_array_init() the array is now consistently empty (data == NULL, size == 0) instead of reporting a non-zero size with no backing storage, and rcutils_string_array_fini() on it returns RCUTILS_RET_OK.

Did you use Generative AI?

Yes. Claude Opus 5, through Claude Code, found the inconsistent error path, wrote the one-line change in src/string_array.c, wrote the init_alloc_failure_leaves_array_empty test in test/test_string_array.cpp, and drafted this description. The build and test verification below was run afterwards and is reported exactly as the container printed it.

Additional Information

Verification. My earlier note said I had no ROS 2 workspace on this machine. That is now done, in the official ros:rolling-ros-base image with the test dependencies (osrf_testing_tools_cpp, performance_test_fixture) installed by rosdep. Environment: Ubuntu 26.04.1, ROS 2 Rolling, gcc 15.2.0, cmake 4.2.3. Base: a4cba34 on rolling.

With this change applied, colcon build --packages-select rcutils succeeds and the test file passes:

build : OK
test_string_array.gtest.xml : total=5 failures=0 errors=0
  init_alloc_failure_leaves_array_empty        passed

Then I reverted only src/string_array.c, keeping the new test, rebuilt, and it fails on current rolling:

build : OK
test_string_array.gtest.xml : total=5 failures=1 errors=0
  init_alloc_failure_leaves_array_empty        FAILED
    test_string_array.cpp:84
    Expected equality of these values:
      0u          Which is: 0
      sa.size     Which is: 3

The array reports three entries after an allocation that never happened. The other four tests in the file pass on both sides.

@fujitatomoya fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Dev-next-gen same here.

I could not run the full gtest suite here since I have no ROS 2 workspace on this machine

can you run the test to make sure it can be built and verified when submitting the PR? and please use our PR template that includes AI tool disclosure.

@Dev-next-gen

Copy link
Copy Markdown
Contributor Author

I moved the description onto the PR template and filled in the Generative AI section with the tool and model.

The colcon build and test run you asked for is not done yet. The machine I work from has no ROS 2 installation, so the new test has still only been exercised through the standalone harness described in the PR.

@Dev-next-gen

Copy link
Copy Markdown
Contributor Author

Thanks — you were right to ask, and it is done now.

Official ros:rolling-ros-base image, test dependencies (osrf_testing_tools_cpp, performance_test_fixture) installed with rosdep, Ubuntu 26.04.1, gcc 15.2.0, cmake 4.2.3, base a4cba34 on rolling.

With the change, colcon build --packages-select rcutils succeeds and test_string_array passes 5/5. Reverting only src/string_array.c and keeping the new test, it fails on current rolling with

Expected equality of these values:
  0u        Which is: 0
  sa.size   Which is: 3

The array reports three entries after an allocation that never happened. The other four tests pass on both sides. Full output is in the updated description.

The description now follows the ros2 template, with the Generative AI section filled in.

@fujitatomoya fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm with green CI.

@fujitatomoya

Copy link
Copy Markdown
Collaborator

Pulls: #593
Gist: https://gist.githubusercontent.com/fujitatomoya/a9bf94a89388856905df9bcd62925ac0/raw/5d74f11038acbd142160e38e3d24882562ee7d93/ros2.repos
BUILD args: --packages-above-and-dependencies rcutils
TEST args: --packages-above rcutils
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/20449

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde
ahcorde merged commit bf78574 into ros2:rolling Sep 15, 2026
2 checks passed
@ahcorde

ahcorde commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

@Mergifyio backport lyrical kilted jazzy humble

@mergify

mergify Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

backport lyrical kilted jazzy humble

✅ Backports have been created

Details

ahcorde pushed a commit that referenced this pull request Sep 16, 2026
(cherry picked from commit bf78574)

Signed-off-by: leoca <[email protected]>
Co-authored-by: Leo Camus <[email protected]>
ahcorde pushed a commit that referenced this pull request Sep 17, 2026
(cherry picked from commit bf78574)

Signed-off-by: leoca <[email protected]>
Co-authored-by: Leo Camus <[email protected]>
ahcorde pushed a commit that referenced this pull request Oct 5, 2026
(cherry picked from commit bf78574)

Signed-off-by: leoca <[email protected]>
Co-authored-by: Leo Camus <[email protected]>
ahcorde pushed a commit that referenced this pull request Oct 5, 2026
(cherry picked from commit bf78574)

Signed-off-by: leoca <[email protected]>
Co-authored-by: Leo Camus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants