Incorrect ReaderProxyData object serialization code

[i-and]

Is there an already existing issue for this?

• [X] 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: https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L297-L302 https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L346-L351 and https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L303-L307 https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L341-L345 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: https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L582-L589 I suggest deleting this code because the object m_type_information is written correctly in this method below: https://github.com/eProsima/Fast-DDS/blob/74d8e279a4b18b5b525e64116aad112860dffcb9/src/cpp/rtps/builtin/data/ReaderProxyData.cpp#L638-L645 I'm not sure if a separate regression test is needed for these cases.

[Javgilavi]

Hi @i-and,

A regression test would be needed. But even so, it would be great if you could add this changes directly in a new PR under your name.