Skip to content

node_parameters: reject non-finite values in floating-point range check#3143

Merged
fujitatomoya merged 1 commit into
ros2:rollingfrom
bartalor:bar/issue-2898
May 28, 2026
Merged

node_parameters: reject non-finite values in floating-point range check#3143
fujitatomoya merged 1 commit into
ros2:rollingfrom
bartalor:bar/issue-2898

Conversation

@bartalor
Copy link
Copy Markdown
Contributor

@bartalor bartalor commented May 10, 2026

Description

Fixes #2898. __check_double_range accepted +inf, -inf, and NaN for parameters declared with a floating_point_range. Fix guards __are_doubles_equal against non-finite operands and rewrites the bound check so NaN is rejected.

Is this user-facing behavior change?

Yes. set_parameter with +inf, -inf, or NaN on a parameter with a floating_point_range now returns successful=false.

Did you use Generative AI?

Yes — Claude Opus 4.7.

Additional Information

Adds a regression test in test_node.cpp for the three non-finite cases. The new scope block pushes the existing TEST_F over cpplint's 800-line limit, so a // NOLINT(readability/fn_size) is added on its closing brace — same pattern other ROS 2 packages use for this case.

fujitatomoya
fujitatomoya approved these changes May 13, 2026
View reviewed changes
Comment thread rclcpp/test/rclcpp/test_node.cpp
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented May 13, 2026

Pulls: #3143
Gist: https://gist.githubusercontent.com/fujitatomoya/e96e535bdc812744725b6237e9eb22e6/raw/ab84ebe82133d581d33d732a722c20660dfc487c/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19255

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

@fujitatomoya fujitatomoya mentioned this pull request May 13, 2026
Closed
@bartalor
node_parameters: reject non-finite values in floating-point range check
0bdfeb5 
Fix ros2#2898. __check_double_range accepted +inf, -inf, and NaN for a
declared floating_point_range. Two causes:

1. The boundary fast path used __are_doubles_equal, whose ULP-tolerance
   arithmetic degenerates on non-finite operands (e.g. it claims +inf
   equals any finite boundary), so +inf and -inf slipped past.
2. The bound check (value < from) || (value > to) is false on both
   sides for NaN, so NaN slipped past.

Fix:

- Guard __are_doubles_equal: if either operand is non-finite, fall
  back to exact ==.
- Rewrite the bound check as !(value >= from && value <= to), which
  rejects NaN.

Adds a regression test for +inf, -inf, and NaN.

Signed-off-by: Bar <bartalor@gmail.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented May 14, 2026

  • Windows Build Status

bartalor added a commit to bartalor/rclcpp that referenced this pull request May 26, 2026
@bartalor
docs(ci): flake vs signal analysis of PR ros2#3143 builds
c2631de 
Classifies every failure observed on PR ros2#3143 across the five Jenkins
jobs (ci_linux, ci_linux-aarch64, ci_linux-rhel, ci_windows #27970,
ci_windows #27999) plus the windows-repro GHA run. Compares against
same-job builds in the same 2026-05-10..05-22 window:

- rhel #8929 uncrustify failures: flake (recurred on #8965).
- windows #27970: infra flake (Jenkins agent disconnect).
- windows #27999 test_rosidl_buffer/rmw_fastrtps_cpp x9: environmental,
  not PR-caused (didn't recur on other Windows builds; our GHA repro
  passed all 9).
@bartalor
Copy link
Copy Markdown
Contributor Author

bartalor commented May 27, 2026

  • Windows Build Status

I haven't yet been able to reproduce the RHEL and Windows failures through GitHub Actions builds, but I'm actively investigating. I'll post an update here once I have findings.

bartalor added a commit to bartalor/rclcpp that referenced this pull request May 27, 2026
@bartalor
docs(windows-repro): clarify we're reproducing env, not PR-caused fai…
d7c7b69 
…lures

A green GHA run means we failed to match the ci.ros2.org environment,
not that PR ros2#3143 is safe — the failing tests fail on rolling too.
@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented May 27, 2026

The windows Ci failures are unrelated you can ignore them

@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented May 27, 2026

Same for rhel this pr is good to be merged

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented May 28, 2026

@jmachowinski thanks for checking that!

@fujitatomoya fujitatomoya merged commit fa8478f into ros2:rolling May 28, 2026
3 checks passed
bartalor added a commit to bartalor/rclcpp that referenced this pull request May 28, 2026
@bartalor
docs(ci): flake vs signal analysis of PR ros2#3143 builds
4badd07 
Classifies every failure observed on PR ros2#3143 across the five Jenkins
jobs (ci_linux, ci_linux-aarch64, ci_linux-rhel, ci_windows #27970,
ci_windows #27999) plus the windows-repro GHA run. Compares against
same-job builds in the same 2026-05-10..05-22 window:

- rhel #8929 uncrustify failures: flake (recurred on #8965).
- windows #27970: infra flake (Jenkins agent disconnect).
- windows #27999 test_rosidl_buffer/rmw_fastrtps_cpp x9: environmental,
  not PR-caused (didn't recur on other Windows builds; our GHA repro
  passed all 9).
bartalor added a commit to bartalor/rclcpp that referenced this pull request May 28, 2026
@bartalor
docs(windows-repro): clarify we're reproducing env, not PR-caused fai…
d5bef52 
…lures

A green GHA run means we failed to match the ci.ros2.org environment,
not that PR ros2#3143 is safe — the failing tests fail on rolling too.
@pum1k
Copy link
Copy Markdown
Contributor

pum1k commented Jun 1, 2026

Hi, @fujitatomoya, could this be backported?

@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Jun 1, 2026

@Mergifyio backport kilted jazzy

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jun 1, 2026 •
edited
Loading

backport kilted jazzy

✅ Backports have been created

Details

Cherry-pick of fa8478f has failed:

On branch mergify/bp/kilted/pr-3143
Your branch is up to date with 'origin/kilted'.

You are currently cherry-picking commit fa8478f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
	both modified:   rclcpp/test/rclcpp/test_node.cpp

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Cherry-pick of fa8478f has failed:

On branch mergify/bp/jazzy/pr-3143
Your branch is up to date with 'origin/jazzy'.

You are currently cherry-picking commit fa8478f.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp
	both modified:   rclcpp/test/rclcpp/test_node.cpp

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

This was referenced Jun 1, 2026
Open
Open
@pum1k
Copy link
Copy Markdown
Contributor

pum1k commented Jun 1, 2026

Was lyrical skipped intentionally?

@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Jun 1, 2026 •
edited
Loading

Nope, it is just too new and not in the muscle memory 😜

@Mergifyio backport lyrical

@jmachowinski
Copy link
Copy Markdown
Collaborator

jmachowinski commented Jun 1, 2026

@Mergifyio backport lyrical

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jun 1, 2026 •
edited
Loading

backport lyrical

✅ Backports have been created

Details

@mergify mergify Bot mentioned this pull request Jun 1, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Faulty boundary condition check in dynamic parameter update of floating point parameters

4 participants

@bartalor @fujitatomoya @jmachowinski @pum1k