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.

Sign up for GitHub

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

Bugfix/protocol handler memory issues #2270

Merged
merged 5 commits into from
Jun 6, 2020
Merged

Bugfix/protocol handler memory issues #2270

merged 5 commits into from
Jun 6, 2020

Conversation

shoamano83
Copy link
Contributor

Fixes #2269, except for the last memory leak issue.

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Re-run the unit tests with valgrind and make sure memory issues are fixed.

Summary

This PR addresses several memory issues found during protocol_handler_test unit tests.

Changelog

Breaking Changes
  • None
Enhancements
  • None
Bug Fixes
  • fix: memory leak in ProtocolHandlerImpl::NotifySessionStarted()
  • fix: mismatched memory alloc/dealloc in ProtocolHandlerImpl::NotifySessionStarted()
  • fix: buffer over-run in ProtocolPayloadTest
  • fix: invalid memory accesses in ProtocolHandlerImplTest
  • fix: valgrind warning in IncomingDataHandlerTest

Tasks Remaining:

  • None

CLA

@shoamano83
Copy link
Contributor Author

Note: issues (1) through (5) described in #2269 will be fixed with this PR.
I did not come up with a good idea to resolve (6) and I'd appreciate if somebody could look into it in another PR. A possible approach is to update SecurityManagerImpl::AddListener() to receive shared pointer. (However, I don't find a call to RemoveListener() anywhere in the code. Is this intentional??)

@shoamano83 shoamano83 force-pushed the bugfix/protocol_handler_memory_issues branch from f4d32cf to 2af5283 Compare July 5, 2018 07:28
@shoamano83
Copy link
Contributor Author

Rebased onto latest develop branch to resolve a merge conflict.

@shoamano83
fix: memory leak in ProtocolHandlerImpl::NotifySessionStarted()
1f620d2 
This is detected by valgrind.
@shoamano83
fix: buffer over-run in ProtocolPayloadTest
8c9daa8 
This is an issue of unit test implementation and only affects
unit testing.
@shoamano83
fix: invalid memory accesses in ProtocolHandlerImplTest
cde94af 
These affect unit testing only.
Invalid memory accesses occurred because the mock class didn't
configure protocol version field properly.
@shoamano83
fix: valgrind warning in IncomingDataHandlerTest
ab751fd 
This is not actually an issue. valgrind reported a warning
since the memory area was not initialized.
@shoamano83 shoamano83 force-pushed the bugfix/protocol_handler_memory_issues branch from 2af5283 to ab751fd Compare July 6, 2018 05:19
@JenkinsSDLOnCloud
Copy link

Can one of the admins verify this patch?

@iCollin iCollin merged commit 343f003 into smartdevicelink:develop Jun 6, 2020
This was referenced Jun 6, 2020
Closed
Closed
@AlbertGRMN AlbertGRMN mentioned this pull request Sep 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shoamano83 @JenkinsSDLOnCloud @iCollin