Skip to content

Commit

Permalink
Improve content filter expression parameters checks and verbosity. (#…
Browse files Browse the repository at this point in the history 
…3565) (#3592)

* Refs #18763: Added max expression parameter test

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Fixed maximum parameter size checks when deserializing ContentFilteredTopics

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Added test for qos expression_parameter constraints

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Added check for expression_parameter constraints

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Added runtime modification to test

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Added check to set_expression_parameters

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Fixed signed/unsigned mismatch

Signed-off-by: Javier Santiago <[email protected]>

* Refs #18763: Applied suggestions

Signed-off-by: Javier Santiago <[email protected]>

---------

Signed-off-by: Javier Santiago <[email protected]>
(cherry picked from commit 3073c64)

Co-authored-by: jsantiago-eProsima <[email protected]>
  • Loading branch information
@mergify @jsan-rt
mergify[bot] and jsan-rt authored Jun 15, 2023
1 parent 481c0e1 commit 9f41cca
Show file tree
Hide file tree
Showing 5 changed files with 201 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/cpp/fastdds/core/policy/ParameterSerializer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ class ParameterSerializer<fastdds::rtps::ContentFilterProperty>
valid = fastrtps::rtps::CDRMessage::readUInt32(cdr_message, &num_parameters);
if (valid)
{
valid = (num_parameters < 100) && (num_parameters < parameter.expression_parameters.max_size());
valid = (num_parameters <= 100) && (num_parameters <= parameter.expression_parameters.max_size());
}
if (valid)
{
Expand Down
15 changes: 15 additions & 0 deletions src/cpp/fastdds/domain/DomainParticipantImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,21 @@ ContentFilteredTopic* DomainParticipantImpl::create_contentfilteredtopic(
return nullptr;
}

if (expression_parameters.size() > qos_.allocation().content_filter.expression_parameters.maximum)
{
EPROSIMA_LOG_ERROR(PARTICIPANT, "Number of expression parameters exceeds maximum allocation limit: "
<< expression_parameters.size() << " > "
<< qos_.allocation().content_filter.expression_parameters.maximum);
return nullptr;
}

if (expression_parameters.size() > 100)
{
EPROSIMA_LOG_ERROR(PARTICIPANT, "Number of expression parameters exceeds maximum protocol limit: "
<< expression_parameters.size() << " > 100");
return nullptr;
}

TopicProxy* topic_impl = dynamic_cast<TopicProxy*>(related_topic->get_impl());
assert(nullptr != topic_impl);
const TypeSupport& type = topic_impl->get_type();
Expand Down
19 changes: 16 additions & 3 deletions src/cpp/fastdds/topic/ContentFilteredTopicImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@

#include <algorithm>

#include <fastdds/dds/core/policy/ParameterTypes.hpp>
#include <fastrtps/utils/md5.h>

#include <fastdds/core/policy/ParameterList.hpp>
#include <fastdds/dds/core/policy/ParameterTypes.hpp>
#include <fastdds/dds/domain/DomainParticipant.hpp>
#include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
#include <fastdds/rtps/messages/CDRMessage.h>
#include <fastdds/subscriber/DataReaderImpl.hpp>
#include <fastdds/topic/ContentFilterUtils.hpp>
#include <fastdds/topic/TopicProxy.hpp>

#include <fastrtps/types/TypesBase.h>
#include <fastrtps/utils/md5.h>

namespace eprosima {
namespace fastdds {
namespace dds {
Expand Down Expand Up @@ -60,6 +63,16 @@ ReturnCode_t ContentFilteredTopicImpl::set_expression_parameters(
assert(nullptr != topic_impl);
const TypeSupport& type = topic_impl->get_type();

DomainParticipantQos pqos;
related_topic->get_participant()->get_qos(pqos);
if (new_expression_parameters.size() > pqos.allocation().content_filter.expression_parameters.maximum )
{
EPROSIMA_LOG_ERROR(CONTENT_FILTERED_TOPIC, "Number of expression parameters exceeds maximum allocation limit: "
<< new_expression_parameters.size() << " > "
<< pqos.allocation().content_filter.expression_parameters.maximum);
return ReturnCode_t::RETCODE_BAD_PARAMETER;
}

LoanableSequence<const char*>::size_type n_params;
n_params = static_cast<LoanableSequence<const char*>::size_type>(new_expression_parameters.size());
LoanableSequence<const char*> filter_parameters(n_params);
Expand Down
65 changes: 65 additions & 0 deletions test/unittest/dds/participant/ParticipantTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2166,6 +2166,71 @@ TEST(ParticipantTests, DeleteTopicInUse)
ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
}

// Check that the constraints on maximum expression parameter size are honored
TEST(ParticipantTests, ExpressionParameterLimits)
{

DomainParticipantQos pqos = PARTICIPANT_QOS_DEFAULT;
TypeSupport type(new TopicDataTypeMock());

// Testing QoS Default limit
DomainParticipant* participant_default_max_parameters =
DomainParticipantFactory::get_instance()->create_participant(0, pqos);
type.register_type(participant_default_max_parameters, "footype");

Topic* topic_default_max_parameters = participant_default_max_parameters->create_topic("footopic", "footype",
TOPIC_QOS_DEFAULT);

std::vector<std::string> expression_parameters;
for (int i = 0; i < 101; i++)
{
expression_parameters.push_back("Parameter");
}

ContentFilteredTopic* content_filtered_topic_default_max_parameters =
participant_default_max_parameters->create_contentfilteredtopic("contentfilteredtopic",
topic_default_max_parameters, "", expression_parameters);
ASSERT_EQ(content_filtered_topic_default_max_parameters, nullptr);

ContentFilteredTopic* content_filtered_topic_default_valid_parameters =
participant_default_max_parameters->create_contentfilteredtopic("contentfilteredtopic",
topic_default_max_parameters, "", {"Parameter1", "Parameter2"});
ASSERT_NE(content_filtered_topic_default_valid_parameters, nullptr);

ASSERT_EQ(participant_default_max_parameters->delete_contentfilteredtopic(
content_filtered_topic_default_valid_parameters), ReturnCode_t::RETCODE_OK);
ASSERT_EQ(participant_default_max_parameters->delete_topic(topic_default_max_parameters), ReturnCode_t::RETCODE_OK);
ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(
participant_default_max_parameters), ReturnCode_t::RETCODE_OK);

// Test user defined limits
pqos.allocation().content_filter.expression_parameters.maximum = 1;

DomainParticipant* participant =
DomainParticipantFactory::get_instance()->create_participant(0, pqos);

type.register_type(participant, "footype");

Topic* topic = participant->create_topic("footopic", "footype", TOPIC_QOS_DEFAULT);

ContentFilteredTopic* content_filtered_topic = participant->create_contentfilteredtopic("contentfilteredtopic",
topic, "", {"Parameter1", "Parameter2"});
ASSERT_EQ(content_filtered_topic, nullptr);

content_filtered_topic = participant->create_contentfilteredtopic("contentfilteredtopic",
topic, "", {"Parameter1"});
ASSERT_NE(content_filtered_topic, nullptr);

ASSERT_EQ(fastrtps::types::ReturnCode_t::RETCODE_BAD_PARAMETER, content_filtered_topic->set_expression_parameters(
{"Parameter1",
"Parameter2"}));

ASSERT_EQ(participant->delete_contentfilteredtopic(content_filtered_topic), ReturnCode_t::RETCODE_OK);
ASSERT_EQ(participant->delete_topic(topic), ReturnCode_t::RETCODE_OK);

ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK);
}


void set_listener_test (
DomainParticipant* participant,
Expand Down
104 changes: 104 additions & 0 deletions test/unittest/rtps/builtin/BuiltinDataSerializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,110 @@ TEST(BuiltinDataSerializationTests, contentfilterproperty_interoperability)
ASSERT_EQ("200", out.content_filter().expression_parameters[3].to_string());
}

/*!
* \test Test for deserialization of messages with expression parameters list
* with sizes of 100 or greater
*/
TEST(BuiltinDataSerializationTests, contentfilterproperty_max_parameter_check)
{
// Empty value
{
ReaderProxyData out(max_unicast_locators, max_multicast_locators);

// Perform serialization
CDRMessage_t msg(5000);
EXPECT_TRUE(fastdds::dds::ParameterList::writeEncapsulationToCDRMsg(&msg));
const std::string content_filtered_topic_name("CFT_TEST");
const std::string related_topic_name("TEST");
const std::string filter_class_name("MyFilterClass");
const std::string filter_expression("This is a custom test filter expression");
std::vector<std::string> expression_parameters;
for (int i = 0; i < 100; ++i)
{
expression_parameters.push_back("Parameter");
}

// Manual serialization of a ContentFilterProperty.
{
uint32_t len = manual_content_filter_cdr_serialized_size(
content_filtered_topic_name,
related_topic_name,
filter_class_name,
filter_expression,
expression_parameters
);
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt16(&msg, fastdds::dds::PID_CONTENT_FILTER_PROPERTY));
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt16(&msg, static_cast<uint16_t>(len - 4)));
// content_filtered_topic_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg, content_filtered_topic_name));
// related_topic_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg, related_topic_name));
// filter_class_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg, filter_class_name));
// filter_expression
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg, filter_expression));
// expression_parameters
// sequence length
uint32_t num_params = static_cast<uint32_t>(expression_parameters.size());
EXPECT_EQ(num_params, 100u);
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt32(&msg, num_params));
// Add all parameters
for (const std::string& param : expression_parameters)
{
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg, param));
}
}
EXPECT_TRUE(fastdds::dds::ParameterSerializer<Parameter_t>::add_parameter_sentinel(&msg));

msg.pos = 0;
EXPECT_TRUE(out.readFromCDRMessage(&msg, network, true));

ASSERT_EQ(100, out.content_filter().expression_parameters.size());

CDRMessage_t msg_fault(5000);
EXPECT_TRUE(fastdds::dds::ParameterList::writeEncapsulationToCDRMsg(&msg_fault));

// Manual serialization of a ContentFilterProperty.
{
uint32_t len = manual_content_filter_cdr_serialized_size(
content_filtered_topic_name,
related_topic_name,
filter_class_name,
filter_expression,
expression_parameters
);
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt16(&msg_fault, fastdds::dds::PID_CONTENT_FILTER_PROPERTY));
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt16(&msg_fault, static_cast<uint16_t>(len - 4)));
// content_filtered_topic_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg_fault, content_filtered_topic_name));
// related_topic_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg_fault, related_topic_name));
// filter_class_name
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg_fault, filter_class_name));
// filter_expression
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg_fault, filter_expression));
// expression_parameters
// sequence length

// Add the 101st parameter to the list
expression_parameters.push_back("Parameter");
uint32_t num_params = static_cast<uint32_t>(expression_parameters.size());
EXPECT_EQ(num_params, 101u);
EXPECT_TRUE(fastrtps::rtps::CDRMessage::addUInt32(&msg_fault, num_params));
// Add all parameters
for (const std::string& param : expression_parameters)
{
EXPECT_TRUE(fastrtps::rtps::CDRMessage::add_string(&msg_fault, param));
}
}
EXPECT_TRUE(fastdds::dds::ParameterSerializer<Parameter_t>::add_parameter_sentinel(&msg_fault));

msg_fault.pos = 0;
// Deserialization of messages with more than 100 parameters should fail
ASSERT_FALSE(out.readFromCDRMessage(&msg_fault, network, true));
}
}

} // namespace rtps
} // namespace fastrtps
} // namespace eprosima
Expand Down

0 comments on commit 9f41cca

Please sign in to comment.