[23923] Fix RTPSParticipantAttributes internal data races#6370
Conversation
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
|
259 Data Races reported by ASAN, none of them related with |
MiguelCompany
left a comment
There was a problem hiding this comment.
Just one nit. It does not require to rerun CI.
Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com>
|
@Mergifyio backport 3.4.x 3.2.x 2.14.x |
✅ Backports have been createdDetails
Cherry-pick of 7dd4b4d has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 7dd4b4d has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of 7dd4b4d has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
* Refs #23923: Take mutex in getter and env file callback Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Only update mutable attributes Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Copy attributes in getter Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Undo copy attributes Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Avoid calling get_attributes in SecurityManager constructor Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Copy attributes Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> * Refs #23923: Protect missing cases Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Create new method to avoid API break Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Doxygen & TODO in next major Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Copy method in RTPSParticipant Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Mock and tests changes Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Revision Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Add missing mutable attribute Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Create const copy Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Uncrustify Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Spelling Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Split const and mutable RTPSParticipantAttributes Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Review - Fix Mutable & Constant attributes Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Review - Apply methods and composition of BuiltinAttributes Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Review - Update Tests Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Review - Add ConstantDiscoverySettings Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Solve using statement visibility Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Uncrustify Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Store constant attributes set at 'setup_' methods Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Init const attributes and update later Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> * Refs #23923: Review - Improve doxygen Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> --------- Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> Co-authored-by: Juan Lopez Fernandez <juanlopez@eprosima.com> (cherry picked from commit 7dd4b4d)
* Refs #23923: Take mutex in getter and env file callback * Refs #23923: Only update mutable attributes * Refs #23923: Copy attributes in getter * Refs #23923: Undo copy attributes * Refs #23923: Avoid calling get_attributes in SecurityManager constructor * Refs #23923: Copy attributes * Refs #23923: Protect missing cases * Refs #23923: Create new method to avoid API break * Refs #23923: Doxygen & TODO in next major * Refs #23923: Copy method in RTPSParticipant * Refs #23923: Mock and tests changes * Refs #23923: Revision * Refs #23923: Add missing mutable attribute * Refs #23923: Create const copy * Refs #23923: Uncrustify * Refs #23923: Spelling * Refs #23923: Split const and mutable RTPSParticipantAttributes * Refs #23923: Review - Fix Mutable & Constant attributes * Refs #23923: Review - Apply methods and composition of BuiltinAttributes * Refs #23923: Review - Update Tests * Refs #23923: Review - Add ConstantDiscoverySettings * Refs #23923: Solve using statement visibility * Refs #23923: Uncrustify * Refs #23923: Store constant attributes set at 'setup_' methods * Refs #23923: Init const attributes and update later * Refs #23923: Review - Improve doxygen --------- (cherry picked from commit 7dd4b4d) Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com> Signed-off-by: Carlos Ferreira González <carlosferreira@eprosima.com> Co-authored-by: Carlos Ferreira González <carlosferreira@eprosima.com> Co-authored-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Description
This PR solves a bunch of data races caused by the not-thread-safe access to RTPSParticipantAttributes via its main getter
get_attributes(). This method returns a const reference to the object, which can be concurrently modified from other functions likeupdate_attributes()causing a read-write access data race.The proposed fix should modify the original method to return a copy of the class attribute with the mutex locked, making the access safe. However, this would imply an API break. Thus, a new method has been created to be used internally, leaving the
get_attributes()method untouched and marked to be fixed in the next major release.Related PR:
@Mergifyio backport 3.4.x 3.2.x 2.14.x
Contributor Checklist
versions.mdfile (if applicable).Reviewer Checklist