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

Fix undefined behavior in log #62

Merged
merged 11 commits into from
May 5, 2022
Merged

Fix undefined behavior in log #62

merged 11 commits into from
May 5, 2022

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented May 5, 2022

This PR fixes a case of undefined behavior that can happen in the task logging system.

Previously, when a log was read twice in a row without any new entries being written, the flawed logic in the reader would increment an iterator past the end of its container, which leads to undefined behavior. This PR fixes that logic and prevents the iterators from going past the end.

This branch builds off of the branch for #61 so we should merge that PR first.

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey mxgrey requested a review from aaronchongth May 5, 2022 04:24
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #62 (9ad33ed) into main (a5e294a) will increase coverage by 0.27%.
The diff coverage is 44.92%.

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   36.21%   36.48%   +0.27%     
==========================================
  Files          65       65              
  Lines        2491     2557      +66     
  Branches     1366     1416      +50     
==========================================
+ Hits          902      933      +31     
+ Misses        552      550       -2     
- Partials     1037     1074      +37     
Flag Coverage Δ
tests 36.48% <44.92%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rmf_task/test/unit/test_Log.cpp 40.47% <44.26%> (+10.04%) ⬆️
rmf_task/src/rmf_task/Log.cpp 60.39% <50.00%> (-0.22%) ⬇️
rmf_task/include/rmf_task/Log.hpp 66.66% <0.00%> (+33.33%) ⬆️

aaronchongth
aaronchongth previously approved these changes May 5, 2022
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for adding the test.

I don't believe it should be a concern, but just a random note, testing locally I see that it spins up to about 1GB of RAM during those 5 second tests.

mxgrey added 2 commits May 5, 2022 15:14
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey
Copy link
Contributor Author

mxgrey commented May 5, 2022

@aaronchongth 5s and 1GB is admittedly pretty extreme. I've dialed it down to 1s, but unfortunately that made your review stale.

mxgrey added 4 commits May 5, 2022 15:20
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey mxgrey merged commit 32de25f into main May 5, 2022
@mxgrey mxgrey deleted the investigate_log_segfault branch May 5, 2022 14:19
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

2 participants