Patch CVE-2024-42002

[mjcarroll]

Replaces #999

[ahcorde]

Pulls: #1001
Gist: https://gist.githubusercontent.com/ahcorde/418d81fdef70ee2209e3fd6c7d23ab9e/raw/5366d57a0db798da244c1205658c21f84aa69ae2/ros2.repos
BUILD args: --packages-above-and-dependencies ros2topic
TEST args: --packages-above ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15571

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[mjcarroll]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[florcabral]

Hi @mjcarroll. I noticed there was a CI issue when merging this CVE patch. Any help needed?

[mjcarroll]

Hey @florcabral if you wouldn't mind taking a look at the CI issues here, I didn't catch them in my local testing before merging.

[florcabral]

@mjcarroll Trying to drill down into the actual tests that failed after this patch - am I looking at the right build log here? If so, for ros2topic I see some failures related to tests for the echo verb, but the patch only affected the hz verb, plus some linter issues. I am not seeing any failing tests related to the patch itself. Have you looked into this yourself? Maybe I'm missing something.

[mjcarroll]

Pulls: #1001
Gist: https://gist.githubusercontent.com/mjcarroll/bc4c882cb2e32c4f4fe313f12076b61d/raw/5366d57a0db798da244c1205658c21f84aa69ae2/ros2.repos
BUILD args: --packages-up-to ros2topic
TEST args: --packages-select ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16047

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[mjcarroll]

@florcabral I have rerun the CI, you can see the logs here: https://ci.ros2.org/job/ci_linux-aarch64/17812/testReport/junit/ros2topic.ros2topic.test/test_cli/test_cli/

[ahcorde]

Pulls: #1001
Gist: https://gist.githubusercontent.com/ahcorde/0a289b20c1051c4fbc33dad3293c0bc3/raw/fef082edf7944539f8bb3200e713c318928b13dd/ros2.repos
BUILD args: --packages-above-and-dependencies ros2topic
TEST args: --packages-above ros2topic
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17223

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ahcorde]

@ros-pull-request-builder retest this please

[christophebedard]

IIRC, there was something wrong with this implementation.

[christophebedard]

Also, it severely limits Python expression-based filtering: #1003 (comment)

[florcabral]

Suggested change

	print(self.model.attributes)
	print(node)

[florcabral]

Hello,
Coming back to this discussion: I tried to reproduce locally the tests reported to fail due to these changes, specifically these 2:

test_cli/test_cli/ test_cli.TestROS2TopicCLI.test_filtered_topic_hz[rmw_fastrtps_cpp] failed 

test_cli.TestROS2TopicCLI.test_filtered_topic_hz[rmw_zenoh_cpp] failed

I found that ros2topic/eval/__init__.py was printing internal evaluator state every time a filter expression was evaluated due to print statements left in ros2topic/ros2topic/eval/__init__.py. These looked like:

['capitalize', 'casefold', ..., 'data']
<ast.Attribute object at 0x7f...>

These warnings go to stdout, so the launch_testing output checks fail because the expected pattern (ie average rate: ... lines) in tests such as test_filtered_topic_hz expect exact output, so the extra lines caused failures like:

AssertionError: Output does not match:
['capitalize', ..., 'data']
<ast.Attribute ...>
average rate: 0.500

Removing those debug prints should restore the expected behavior. The failures seemed to come from the unexpected stdout pollution, not from the filter logic itself.

As for the evaluation of data msg type, reported on the jazzy PR, I also couldn't reproduce that test failure for this branch, and manual local tests seem to show the filtering logic isn't blocking stuff like m.data.<...>:

# ros2 topic hz /test --filter "int(m.data.partition(':')[-1]) % 2 == 0"
average rate: 1.000
...

If you accept my recent suggestion on ros2topic/ros2topic/eval/__init__.py we should at least get cleaner, easier to validate tests.

Could we run your CI to see exactly what is blocking this PR at this stage?

Thanks!

[florcabral]

@christophebedard Any feedback to my last update? Thanks.