Release change while authentication fails by Barry-Xu-2018 · Pull Request #5935 · eProsima/Fast-DDS

Barry-Xu-2018 · 2025-07-15T07:26:13Z

Description

Address #5934

particiapant_stateless_message_writer_history should be properly freed when authentication fails.

@Mergifyio backport 3.3.x 3.2.x 2.14.x

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
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
Any new/modified methods have been properly documented using Doxygen.
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

The PR has a milestone assigned.
The title and description correctly express the PR's purpose.
Check contributor checklist is correct.
If this is a critical bug fix, backports to the critical-only supported branches have been requested.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Mario-DL · 2025-07-31T10:00:43Z

@Barry-Xu-2018 thanks for the investigation and the fix, leaving a regression test

Mario-DL · 2025-07-31T10:10:59Z

For the shake of completeness.
Even with the fix, we should bear in mind that the history of the writer could potentially be full depending on the configuration of the handshake properties i.e having more than 100 requests ongoing without having failed in any of them yet.

test/blackbox/common/BlackboxTestsSecurity.cpp

Signed-off-by: Barry Xu <barry.xu@sony.com>

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

mergify · 2025-08-26T11:06:35Z

🧪 CI Insights

Here's what we observed from your CI run for ea9c411.

🟢 All jobs passed!

But CI Insights is watching 👀

MiguelCompany · 2025-09-02T08:08:26Z

From this CI error report (#5935 (comment)), I am unable to find detailed error information

The CI run expired. I triggered it again here

MiguelCompany · 2025-09-02T14:25:52Z

@Barry-Xu-2018 The tests look good to me. I am only worried about the ASAN job.

Reports here

Barry-Xu-2018 · 2025-09-03T07:27:53Z

@MiguelCompany

@Barry-Xu-2018 The tests look good to me. I am only worried about the ASAN job.

Reports here

I checked the contents of the Report, but the failed tests listed there are not related to ASAN.
I can find the ASAN failed tests on the ASAN job page.

test_report_fastdds-sanitizers-ci_tsan_fastdds_test

I investigated the data race issues from before and found that they are all related to the following code.

send_to_fixed_locators() and mark_all_readers_interested() can be called simultaneously in two threads. However, in send_to_fixed_locators(), access to should_reach_all_destinations_, fixed_locators_, and interested_readers_ are not protected by a mutex, which leads to a data race.

Fast-DDS/src/cpp/rtps/builtin/discovery/participant/simple/PDPStatelessWriter.cpp

Lines 104 to 130 in 95410fc

    
           bool PDPStatelessWriter::send_to_fixed_locators( 
        
                   const std::vector<eprosima::fastdds::rtps::NetworkBuffer>& buffers, 
        
                   const uint32_t& total_bytes, 
        
                   std::chrono::steady_clock::time_point& max_blocking_time_point) const 
        
           { 
        
               bool ret = true; 
        
               if (should_reach_all_destinations_) 
        
               { 
        
                   ret = initial_peers_.empty() || 
        
                           mp_RTPSParticipant->sendSync(buffers, total_bytes, m_guid, 
        
                                   Locators(initial_peers_.begin()), Locators(initial_peers_.end()), 
        
                                   max_blocking_time_point, transport_priority_); 
        
                   if (ret) 
        
                   { 
        
                       fixed_locators_.clear(); 
        
                       should_reach_all_destinations_ = false; 
        
                   } 
        
               } 
        
               else 
        
               { 
        
                   interested_readers_.clear(); 
        
               } 
        
               return ret; 
        
           }

Fast-DDS/src/cpp/rtps/builtin/discovery/participant/simple/PDPStatelessWriter.cpp

Lines 140 to 148 in 95410fc

    
           void PDPStatelessWriter::mark_all_readers_interested() 
        
           { 
        
               std::lock_guard<RecursiveTimedMutex> guard(mp_mutex); 
        
               should_reach_all_destinations_ = true; 
        
               interested_readers_.clear(); 
        
               fixed_locators_.clear(); 
        
               fixed_locators_.push_back(initial_peers_); 
        
               reader_data_filter(nullptr); 
        
           }

This issue doesn't seem to have been introduced by this PR.

BTW，Could you guide me on running ASAN check in a local environment?

MiguelCompany · 2025-09-03T08:14:44Z

@Barry-Xu-2018 I was referring to ASAN, not TSAN.

For the new test added, it reports the following:

=================================================================
==37706==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1536 byte(s) in 24 object(s) allocated from:
    #0 0x7f38d6eb61e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x7f38d5479588 in eprosima::fastdds::rtps::security::PKIDH::begin_handshake_reply(eprosima::fastdds::rtps::security::Handle**, eprosima::fastdds::rtps::DataHolder**, eprosima::fastdds::rtps::DataHolder&amp;&amp;, eprosima::fastdds::rtps::security::Handle&amp;, eprosima::fastdds::rtps::security::Handle const&amp;, eprosima::fastdds::rtps::CDRMessage_t const&amp;, eprosima::fastdds::rtps::security::SecurityException&amp;) /home/runner/work/Fast-DDS/Fast-DDS/src/fastdds/src/cpp/security/authentication/PKIDH.cpp:1698

Could you guide me on running ASAN check in a local environment?
Use CMake args -DSANITIZER=Address -DCMAKE_BUILD_TYPE=Debug when building fastdds

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2025-09-04T08:20:38Z

@MiguelCompany

I have identified the cause of the memory leak.
Please review this fix 2895379

MiguelCompany

LGTM with green CI.
Thank you @Barry-Xu-2018 !

MiguelCompany · 2025-09-10T06:18:21Z

@Mergifyio backport 3.3.x 3.2.x 2.14.x

mergify · 2025-09-10T06:18:27Z

backport 3.3.x 3.2.x 2.14.x

✅ Backports have been created

Details

#6020 Release change while authentication fails (backport #5935) has been created for branch 3.3.x
#6021 Release change while authentication fails (backport #5935) has been created for branch 3.2.x
#6022 Release change while authentication fails (backport #5935) has been created for branch 2.14.x

* Release change while authentication fails Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Add regression test Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Apply Miguel's suggestions Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Fix windows compilation Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Uncrustify Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Fix a memory leak on handshake handle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix an Uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> --------- Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit db64a12)

* Release change while authentication fails * Fix uncrustify error * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods * Refs #23431: Add regression test * Refs #23431: Apply Miguel's suggestions * Refs #23431: Fix windows compilation * Refs #23431: Uncrustify * Fix a memory leak on handshake handle * Fix an Uncrustify error --------- (cherry picked from commit db64a12) Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Barry Xu <barry.xu@sony.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com>

* Release change while authentication fails Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Add regression test Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Apply Miguel's suggestions Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Fix windows compilation Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Uncrustify Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Fix a memory leak on handshake handle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix an Uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> --------- Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit db64a12)

* Release change while authentication fails (#5935) * Release change while authentication fails Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Add regression test Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Apply Miguel's suggestions Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Fix windows compilation Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Uncrustify Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Fix a memory leak on handshake handle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix an Uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> --------- Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit db64a12) * Fix error in decprecated tests Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Fix backport issues in blackbox tests Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> * Uncrustify Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> --------- Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: Emilio Cuesta <emiliocuesta@eprosima.com> Co-authored-by: Barry Xu <barry.xu@sony.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Emilio Cuesta <emiliocuesta@eprosima.com>

* Release change while authentication fails Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Add regression test Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Apply Miguel's suggestions Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Fix windows compilation Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Refs #23431: Uncrustify Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Fix a memory leak on handshake handle Signed-off-by: Barry Xu <barry.xu@sony.com> * Fix an Uncrustify error Signed-off-by: Barry Xu <barry.xu@sony.com> --------- Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com> (cherry picked from commit db64a12)

* Release change while authentication fails * Fix uncrustify error * Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods * Refs #23431: Add regression test * Refs #23431: Apply Miguel's suggestions * Refs #23431: Fix windows compilation * Refs #23431: Uncrustify * Fix a memory leak on handshake handle * Fix an Uncrustify error --------- (cherry picked from commit db64a12) Signed-off-by: Barry Xu <barry.xu@sony.com> Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Co-authored-by: Barry Xu <barry.xu@sony.com> Co-authored-by: Mario Dominguez <mariodominguez@eprosima.com>

Barry-Xu-2018 mentioned this pull request Jul 15, 2025

In security mode, when the authentication fails, the change for the handshake request is not released. #5934

Closed

1 task

Mario-DL added this to the v3.3.1 milestone Jul 31, 2025

Mario-DL marked this pull request as ready for review July 31, 2025 10:01

Mario-DL requested review from MiguelCompany and removed request for MiguelCompany July 31, 2025 10:01

MiguelCompany modified the milestones: v3.3.1, v3.4.0 Jul 31, 2025

MiguelCompany linked an issue Jul 31, 2025 that may be closed by this pull request

In security mode, when the authentication fails, the change for the handshake request is not released. #5934

Closed

1 task

MiguelCompany requested changes Jul 31, 2025

View reviewed changes

test/blackbox/common/BlackboxTestsSecurity.cpp Outdated Show resolved Hide resolved

test/blackbox/common/BlackboxTestsSecurity.cpp Outdated Show resolved Hide resolved

Barry-Xu-2018 and others added 5 commits July 31, 2025 12:27

Release change while authentication fails

cff975f

Signed-off-by: Barry Xu <barry.xu@sony.com>

Fix uncrustify error

c997625

Signed-off-by: Barry Xu <barry.xu@sony.com>

Refs #23431: Refactor PubSubReader/Writer (un)authorized() methods

e25a00f

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Refs #23431: Add regression test

ab16fe7

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Refs #23431: Apply Miguel's suggestions

5818698

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL force-pushed the fix-bug-on-release-change-after-auth-fail branch from 49773ce to 5818698 Compare July 31, 2025 10:27

MiguelCompany previously approved these changes Jul 31, 2025

View reviewed changes

Mario-DL requested review from MiguelCompany and removed request for MiguelCompany July 31, 2025 10:29

Refs #23431: Fix windows compilation

d531c23

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

Mario-DL dismissed MiguelCompany’s stale review via d531c23 July 31, 2025 12:44

Mario-DL requested review from MiguelCompany and removed request for MiguelCompany July 31, 2025 12:44

Refs #23431: Uncrustify

3cb936f

Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com>

MiguelCompany self-requested a review July 31, 2025 13:45

MiguelCompany previously approved these changes Jul 31, 2025

View reviewed changes

MiguelCompany requested review from richiprosima and removed request for richiprosima August 26, 2025 05:29

MiguelCompany requested review from richiprosima and removed request for richiprosima September 2, 2025 08:07

Fix a memory leak on handshake handle

2895379

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 dismissed MiguelCompany’s stale review via 2895379 September 4, 2025 08:12

Fix an Uncrustify error

ea9c411

Signed-off-by: Barry Xu <barry.xu@sony.com>

MiguelCompany self-requested a review September 4, 2025 09:19

MiguelCompany approved these changes Sep 4, 2025

View reviewed changes

MiguelCompany merged commit db64a12 into eProsima:master Sep 10, 2025
37 of 42 checks passed

This was referenced Sep 10, 2025

Release change while authentication fails (backport #5935) #6020

Merged

Release change while authentication fails (backport #5935) #6021

Merged

mergify bot mentioned this pull request Sep 10, 2025

Release change while authentication fails (backport #5935) #6022

Merged

14 tasks

MiguelCompany mentioned this pull request Sep 10, 2025

The memory usage of FastDDS was gradually increasing under security mode #5900

Closed

1 task

fujitatomoya mentioned this pull request Sep 11, 2025

Upgrade Fast-DDS version for Humble to 2.14.x? ros2/ros2#1748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release change while authentication fails#5935

Release change while authentication fails#5935
MiguelCompany merged 9 commits intoeProsima:masterfrom
Barry-Xu-2018:fix-bug-on-release-change-after-auth-fail

Barry-Xu-2018 commented Jul 15, 2025 •

edited by MiguelCompany

Loading

Uh oh!

Mario-DL commented Jul 31, 2025 •

edited

Loading

Uh oh!

Mario-DL commented Jul 31, 2025

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Aug 26, 2025 •

edited

Loading

Uh oh!

MiguelCompany commented Sep 2, 2025

Uh oh!

MiguelCompany commented Sep 2, 2025

Uh oh!

Barry-Xu-2018 commented Sep 3, 2025

Uh oh!

MiguelCompany commented Sep 3, 2025

Uh oh!

Barry-Xu-2018 commented Sep 4, 2025

Uh oh!

MiguelCompany left a comment

Uh oh!

Uh oh!

MiguelCompany commented Sep 10, 2025

Uh oh!

mergify bot commented Sep 10, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Barry-Xu-2018 commented Jul 15, 2025 • edited by MiguelCompany Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Checklist

Reviewer Checklist

Uh oh!

Mario-DL commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mario-DL commented Jul 31, 2025

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 CI Insights

🟢 All jobs passed!

Uh oh!

MiguelCompany commented Sep 2, 2025

Uh oh!

MiguelCompany commented Sep 2, 2025

Uh oh!

Barry-Xu-2018 commented Sep 3, 2025

Uh oh!

MiguelCompany commented Sep 3, 2025

Uh oh!

Barry-Xu-2018 commented Sep 4, 2025

Uh oh!

MiguelCompany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MiguelCompany commented Sep 10, 2025

Uh oh!

mergify bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Barry-Xu-2018 commented Jul 15, 2025 •

edited by MiguelCompany

Loading

Mario-DL commented Jul 31, 2025 •

edited

Loading

mergify bot commented Aug 26, 2025 •

edited

Loading

mergify bot commented Sep 10, 2025 •

edited

Loading