Incorrect ReaderProxyData object serialization code

[i-and]

Is there an already existing issue for this?

• I have searched the existing issues

Expected behavior

The calculated size of the ReaderProxyData object is correct.

Current behavior

The calculated size of the ReaderProxyData object is not correct.

Steps to reproduce

See code fragment

Fast DDS version/commit

master

Platform/Architecture

Ubuntu Focal 20.04 amd64

Transport layer

UDPv4

Additional context

1. In the ReaderProxyData::get_serialized_size() method, objects m_type_information and m_qos.type_consistency are counted twice.
2. In the ReaderProxyData::writeToCDRMessage() method, object m_type_information is serialized when object m_type_id exists.

Proposed changes:

diff --git a/src/cpp/rtps/builtin/data/ReaderProxyData.cpp b/src/cpp/rtps/builtin/data/ReaderProxyData.cpp
index e3c2f1b6d..c3d16c9c9 100644
--- a/src/cpp/rtps/builtin/data/ReaderProxyData.cpp
+++ b/src/cpp/rtps/builtin/data/ReaderProxyData.cpp
@@ -294,17 +294,6 @@ uint32_t ReaderProxyData::get_serialized_size(
         ret_val += dds::QosPoliciesSerializer<dds::DisablePositiveACKsQosPolicy>::cdr_serialized_size(
             m_qos.m_disablePositiveACKs);
     }
-    if (m_type_information && m_type_information->assigned())
-    {
-        ret_val +=
-                dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::cdr_serialized_size(
-            *m_type_information);
-    }
-    if (m_qos.type_consistency.send_always() || m_qos.type_consistency.hasChanged)
-    {
-        ret_val += dds::QosPoliciesSerializer<dds::TypeConsistencyEnforcementQosPolicy>::cdr_serialized_size(
-            m_qos.type_consistency);
-    }
     if ((m_qos.data_sharing.send_always() || m_qos.data_sharing.hasChanged) &&
             m_qos.data_sharing.kind() != fastdds::dds::OFF)
     {
@@ -579,14 +568,6 @@ bool ReaderProxyData::writeToCDRMessage(
         }
     }
 
-    if (m_type_id && m_type_id->m_type_identifier._d() != fastdds::dds::xtypes::TK_NONE)
-    {
-        if (!dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::add_to_cdr_message(
-                    *m_type_information, msg))
-        {
-            return false;
-        }
-    }
     if (m_properties.size() > 0)
     {
         if (!dds::ParameterSerializer<dds::ParameterPropertyList_t>::add_to_cdr_message(m_properties, msg))

XML configuration file

No response

Relevant log output

No response

Network traffic capture

No response

[MiguelCompany]

@i-and Would be nice if you open a PR with the proposed changes and a regression test

[i-and]

These suggestions appeared based on the results of code analysis of the ReaderProxyData and they are not related to any observed failures. But the following was noticed:

1. In the ReaderProxyData::get_serialized_size() method, two code fragments are duplicated:
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 297 to 302 in 74d8e27

   	if (m_type_information && m_type_information->assigned())
   	{
   	ret_val +=
   	dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::cdr_serialized_size(
   	*m_type_information);
   	}

   
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 346 to 351 in 74d8e27

   	if (m_type_information && m_type_information->assigned())
   	{
   	ret_val +=
   	dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::cdr_serialized_size(
   	*m_type_information);
   	}

   
   and
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 303 to 307 in 74d8e27

   	if (m_qos.type_consistency.send_always() || m_qos.type_consistency.hasChanged)
   	{
   	ret_val += dds::QosPoliciesSerializer<dds::TypeConsistencyEnforcementQosPolicy>::cdr_serialized_size(
   	m_qos.type_consistency);
   	}

   
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 341 to 345 in 74d8e27

   	if (m_qos.type_consistency.send_always() || m_qos.type_consistency.hasChanged)
   	{
   	ret_val += dds::QosPoliciesSerializer<dds::TypeConsistencyEnforcementQosPolicy>::cdr_serialized_size(
   	m_qos.type_consistency);
   	}

   
   This leads to excessive memory requirements because the sizes of objects m_type_information and m_qos.type_consistency are taken into account twice. Therefore, it was proposed to remove duplicate sections of the code.
2. In the ReaderProxyData::writeToCDRMessage() one object is checked (m_type_id) and another (m_type_information) is used, which can lead to access to a non-existent object m_type_information:
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 582 to 589 in 74d8e27

   	if (m_type_id && m_type_id->m_type_identifier._d() != fastdds::dds::xtypes::TK_NONE)
   	{
   	if (!dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::add_to_cdr_message(
   	*m_type_information, msg))
   	{
   	return false;
   	}
   	}

   
   I suggest deleting this code because the object m_type_information is written correctly in this method below:
   

   Fast-DDS/src/cpp/rtps/builtin/data/ReaderProxyData.cpp

   Lines 638 to 645 in 74d8e27

   	if (m_type_information && m_type_information->assigned())
   	{
   	if (!dds::QosPoliciesSerializer<dds::xtypes::TypeInformationParameter>::add_to_cdr_message(
   	*m_type_information, msg))
   	{
   	return false;
   	}
   	}

   
   I'm not sure if a separate regression test is needed for these cases.