[21435] Solve SecurityManager memory issue (backport #5115) #5121
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A memory issue is reported by ASAN when running tests using security such as BlackboxTests_DDS_PIM.Security.BuiltinAuthenticationPlugin_second_participant_creation_loop.
The issue is that the events thread might call
SecurityManager::resend_handshake_message_token
, which internally callsWriterHistory::remove_change_and_reuse
, the latter not being (atomically) thread safe. If at the same time a handshake is processed (SecurityManager::on_process_handshake
) from another thread, this may resolve in a change being added to the aforementioned writer history. As a result a change is added toHistory::m_changes
vector, which would invalidate the iterator dealt with inWriterHistory::remove_change_and_reuse
without protection.Two potential solutions are proposed:
WriterHistory::remove_change_and_reuse
from the security manager, as it's done when callingPDPServer::remove_change_from_history_nts
WriterHistory::remove_change_and_reuse
fully, converting it in an atomic thread-safe method. Note that since the functions internally called from within this method already take the mutex, no (new) deadlocks should be introduced.@Mergifyio backport 2.14.x 2.10.x
Contributor Checklist
Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
N/A: Any new/modified methods have been properly documented using Doxygen.
N/A: Any new configuration API has an equivalent XML API (with the corresponding XSD extension)
Changes are backport compatible: they do NOT break ABI nor change library core behavior.
Changes are API compatible.
N/A: New feature has been added to the
versions.md
file (if applicable).N/A: New feature has been documented/Current behavior is correctly described in the documentation.
Applicable backports have been included in the description.
Reviewer Checklist
This is an automatic backport of pull request #5115 done by [Mergify](https://mergify.com).