Skip to content
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

Remove dst_size from strnlen usage #353

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

Blast545
Copy link
Contributor

Attempt to address #352

There's already a comparison to get the min value from dest_size and src_length here:

if (src_length >= dst_size) {

So I just removed the dst_size from the calculation of the source size.

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

@Blast545
Copy link
Contributor Author

Reproducing the warning, up-to rcutils:
Build Status

Testing with this PR:
Build Status

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 force-pushed the blast545/warning_error_handling_helpers_57 branch from af58772 to 99a038e Compare February 24, 2022 17:04
@Blast545
Copy link
Contributor Author

Removing ros1_bridge from the packaging test job:
Build Status

Removing ros1_bridge and testing with this PR:
Build Status

@Blast545 Blast545 self-assigned this Feb 24, 2022
@Blast545
Copy link
Contributor Author

Let me know if you think this fix is OK before running full CI @clalancette

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me; there is really no reason that we should use strnlen(src, dst_size), since src and dst_size are not correlated at all.

That said, I'll be interested to see what happens when we run CI on this on all platforms, because some of them may complain about using a "bare" strlen. Besides the normal CI (Linux, Linux aarch64, Windows), I'll suggest also running CI with Windows Debug, the clang job, and RHEL to see if anything complains.

@clalancette
Copy link
Contributor

Full CI on this change:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Windows Debug Build Status
  • Linux Clang Build Status
  • RHEL Build Status
  • Packaging Linux Build Status
  • Packaging Linux aarch64 Build Status
  • Packaging Windows Build Status

@clalancette
Copy link
Contributor

clalancette commented Feb 28, 2022

All right, running through CI results here:

  • Linux-aarch64 is unstable, but that is happening on the nightlies as well and is due to a bug in Eigen
  • Windows Debug is unstable; most of the tests there are failing in the nightlies as well, except for the test_action_client_server__rclpy__rmw_fastrtps_cpp
  • Linux Clang is failing, but that is failing on the nightlies as well. Update to curl 7.81. ros/resource_retriever#74 improves (though does not completely fix) the situation.
  • RHEL looks to be about the same as the nightlies
  • Packaging Linux looks to be about the same as the nightlies, minus one additional warning
  • Ditto for Packaging Linux aarch64

So with all of that, the only "new" thing here is the one failure on Windows Debug. I'm going to rerun that one to see if it is a flake:

  • Windows Debug Build Status

@clalancette
Copy link
Contributor

All right, Windows Debug is much happier on the latest round. I'm going to go ahead and merge this one in, thanks @Blast545 !

@clalancette clalancette merged commit 82aa706 into master Mar 1, 2022
@delete-merged-branch delete-merged-branch bot deleted the blast545/warning_error_handling_helpers_57 branch March 1, 2022 19:39
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.

None yet

3 participants