Release change while authentication fails

[Barry-Xu-2018]

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]

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

[Mario-DL]

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.

[mergify]

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

[fujitatomoya]

@MiguelCompany @Mario-DL is there anything needs to be done on our side?

[fujitatomoya]

@Barry-Xu-2018 i think it would be better that you take a look at this? they added some changes based on your original fix.

[Barry-Xu-2018]

i think it would be better that you take a look at this? they added some changes based on your original fix.

I have seen the changes that Mario-DL helped with. He helped to add the regression test.

From this CI error report (#5935 (comment)), I am unable to find detailed error information. @Mario-DL @MiguelCompany Could you please guide me on how to fix the errors in the CI report?

[MiguelCompany]

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]

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

Reports here

[Barry-Xu-2018]

@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]

@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&&, eprosima::fastdds::rtps::security::Handle&, eprosima::fastdds::rtps::security::Handle const&, eprosima::fastdds::rtps::CDRMessage_t const&, eprosima::fastdds::rtps::security::SecurityException&) /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

[Barry-Xu-2018]

@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]

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

[mergify]

backport 3.3.x 3.2.x 2.14.x

✅ Backports have been created

• #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