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

Should generate_service_topics(..) return an error if len(topic_name) > RMW_UXRCE_TOPIC_NAME_MAX_LENGTH? #253

Closed
gavanderhoorn opened this issue May 17, 2022 · 6 comments

Comments

@gavanderhoorn
Copy link
Contributor

As per subject really.

I just ran into a situation where service 'topic' names were being truncated to 60 chars, which did not result in any warnings or errors, other than on the Agent side which complained with:

[WARN] [1652802737.011662749] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/send_goalRe'
[WARN] [1652802737.011679535] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/cancel_goal'
[WARN] [1652802737.011692818] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/get_resultR'

I notice generate_service_topics(..) does pass buffer_size to snprintf(..):

int generate_service_topics(
const char * service_name,
char * request_topic,
char * reply_topic,
size_t buffer_size)
{
snprintf(
request_topic, buffer_size, "%s%s%s", ros_request_prefix,
service_name, ros_request_subfix);
snprintf(
reply_topic, buffer_size, "%s%s%s", ros_reply_prefix,
service_name, ros_reply_subfix);
return 1;
}

but it doesn't check the return value of snprintf(..), which could help detect the topic name was truncated (from here):

If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.

For services specifically, buffer_size is set to RMW_UXRCE_TOPIC_NAME_MAX_LENGTH:

generate_service_topics(
service_name, req_topic_name, res_topic_name,
RMW_UXRCE_TOPIC_NAME_MAX_LENGTH);

so it's known what the maximum is and it could be checked.

@gavanderhoorn
Copy link
Contributor Author

For context: I ran into this with a node name of my_node, and a namespace set to my_namespace. An action server called follow_joint_trajectory will then have a fully-qualified name of /my_namespace/follow_joint_trajectory, which is already 37 chars long.

This resulted in the hidden services getting their names truncated.

This is not obvious, as those services are not (directly) created by application code, but are part of the hidden ROS API.

An error from the initialisation code could help debugging significantly.

@gavanderhoorn
Copy link
Contributor Author

gavanderhoorn commented May 17, 2022

Does generate_topic_name(..) already do this:

int generate_topic_name(
const char * topic_name,
char * full_topic_name,
size_t full_topic_name_size)
{
int ret = snprintf(
full_topic_name,
full_topic_name_size,
"%s%s",
ros_topic_prefix,
topic_name);
if ((ret < 0) && (ret >= (int)full_topic_name_size)) {
return 0;
}
return ret;
}

?


Edit: should that be || instead of &&?

@pablogs9
Copy link
Member

Hi @gavanderhoorn, good catch.

Let us know if #254 solves this.
Feel free to approve the PR if you are ok with it.

@gavanderhoorn
Copy link
Contributor Author

I've not had a chance to test the (already merged) fix in #254.

I believe you've addressed my immediate issue though, so you could perhaps close this issue.

@gavanderhoorn
Copy link
Contributor Author

Thanks again.

@pablogs9
Copy link
Member

Ok, I'm closing. Thanks for the report @gavanderhoorn !

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

No branches or pull requests

2 participants