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

Jump to bottom

Split bug fix #206

Merged
merged 5 commits into from Feb 13, 2020
Merged

Split bug fix #206

merged 5 commits into from Feb 13, 2020

Conversation

Blast545
Copy link
Contributor

This is a bug I encountered adding tests for the split.c module. When calling the split function with the following str "my//hello//world/////" and this delimiter '/', the module crashes trying to free a memory not allocated.

In Line 81 it can be seen the behavior expected for consecutive delimiters, no deallocation is required there.

@Blast545
Remove extra deallocation on memory not allocated
9c59968 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@Blast545
Remove not needed comment
44d0037 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 self-assigned this Feb 11, 2020
@Blast545 Blast545 added the bug Something isn't working label Feb 11, 2020
mjcarroll
mjcarroll approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM, works locally for me, and the test doesn't have any leaks from valgrind.

clalancette
clalancette reviewed Feb 11, 2020
View reviewed changes
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.

The change looks OK to me. Do we have a test that covers this particular corner-case?

hidmic
hidmic reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment •
edited

Choose a reason for hiding this comment

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

A test validating this bugfix would be nice, yeah.

@Blast545
Add test case for delimiters at the end of string
a59d732 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

Blast545 commented Feb 11, 2020 •
edited

I added the case for the failing condition, I'm posting CI results as well:

  • Linux Build Status
  • Linux-aarch64 Build Status (Console results points to error in building the virtualenv, will ask buildfarm)
  • macOS Build Status (CI is known to be failing on macOS at the moment)
  • Windows Build Status

@Blast545 Blast545 requested a review from hidmic February 11, 2020 17:17
hidmic
hidmic reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM pending green CI

test/test_split.cpp Show resolved Hide resolved
@Blast545
Add assert tests for data size before indexing
37edbbb 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 requested a review from hidmic February 11, 2020 21:19
clalancette
clalancette requested changes Feb 12, 2020
View reviewed changes
test/test_split.cpp Outdated Show resolved Hide resolved
@Blast545
Remove redundant asserts performed on test_split funcs
067645d 
Signed-off-by: Jorge J. Perez <jjperez@ekumenlabs.com>
clalancette
clalancette approved these changes Feb 13, 2020
View reviewed changes
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.

Looks good to me with green CI.

@Blast545
Copy link
Contributor Author

Blast545 commented Feb 13, 2020 •
edited

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Blast545 Blast545 mentioned this pull request Feb 13, 2020
Merged
@Blast545 Blast545 merged commit e31371d into master Feb 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the blast545/rcutils_split_bug_fix branch February 13, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@hidmic hidmic Awaiting requested review from hidmic

Assignees

@Blast545 Blast545

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Blast545 @clalancette @mjcarroll @hidmic