Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

[xtypes] publisher / subscriber segfaults#12

Merged
dirk-thomas merged 1 commit intomasterfrom
fix_segfault
Mar 30, 2015
Merged

[xtypes] publisher / subscriber segfaults#12
dirk-thomas merged 1 commit intomasterfrom
fix_segfault

Conversation

@dirk-thomas
Copy link
Copy Markdown
Member

Example to reproduce: userland unit test test_(publisher|subscriber)__rmw_connext_xtypes_dynamic_cpp

@dirk-thomas dirk-thomas added the bug Something isn't working label Mar 30, 2015
@dirk-thomas dirk-thomas self-assigned this Mar 30, 2015
@dirk-thomas dirk-thomas added the in progress Actively being worked on (Kanban column) label Mar 30, 2015
@dirk-thomas
Copy link
Copy Markdown
Member Author

This patch fixed the current segfault. But we have to revisit the type support identifiers and restore the checking functionality it should provide without requiring string comparison.

I have created a separate ticket since that will likely involve more effort: ros2/ros2#30

@esteve @tfoote @wjwwood Please review this workaround.

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 30, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you replace this with strncmp?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What would you suggest to pass as n? Because if I use strlen to determine the length the problem would be same if the string wouldn't have a terminating zero character.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We know the length of the identifier (e.g. rosidl_typesupport_introspection_cpp::typesupport_introspection_identifier), we could just use that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated to use strncmp(A, B, strlen(B)). Thanks.

@esteve
Copy link
Copy Markdown
Member

esteve commented Mar 30, 2015

+1

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 30, 2015

+1

dirk-thomas added a commit that referenced this pull request Mar 30, 2015
[xtypes] publisher / subscriber segfaults
@dirk-thomas dirk-thomas merged commit c28063d into master Mar 30, 2015
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 30, 2015
@dirk-thomas dirk-thomas deleted the fix_segfault branch March 30, 2015 22:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants