Add limit to Metric URIs to avoid buffer overflow
This commit Limits the Number of Metrics to 150 URIs to avoid
overpopulating the Event to be Sent. This is Necessary to avoid Buffer
Overflow During Creation of Periodic MetricReport (RedfishEvent) via
Event Service Subscription. This Buffer overflow occurs as the max size
of buffer used to send Events is limited to 1024*64
This commit also removes max-reading-parameter as max-number-metrics
indirectly places constraint over number of reading parameters (which
makes them mutually exclusive).The parameter max-number-metrics provides
a better control over size of the metrics compared to
max-reading-parameter as it restricts the number of overall uris instead
of number of metrics.
Additional limit for appendLimit was added to ensure we can store all
metric values in persistent memory.
Tested:
- Created Metric Report Defination by POST to
/redfish/v1/TelemetryService/MetricReportDefinitions/
with <150, =150 and >150 Metric Uris
Successfully returned error for metric uris>150.
- Observed no buffer overflow for Maximum of 150 Metric Uris.
- Verified buffer Overflow by Subscribing to Metric Reports
using SSE.
- Test case Passed
[ RUN ] TestReportManager.failToAddReportWith
MoreSensorParametersThanExpected
[ OK ] TestReportManager.failToAddReportWith
MoreSensorParametersThanExpected (10 ms)
[ RUN ] TestReportManager.failToAddReportWith
MoreMetricsThanExpected
[ OK ] TestReportManager.failToAddReportWith
MoreMetricsThanExpected (20 ms)
[ RUN ] TestReportManager.failToAddReportWith
AppendLimitGreaterThanMax
[ OK ] TestReportManager.failToAddReportWith
AppendLimitGreaterThanMax (20 ms)
Signed-off-by: Ankita Vilas Gawade <ankita.gawade@intel.com>
Change-Id: I113c15c7b1054364d827f39582c7d3fbe9495d12
diff --git a/meson.build b/meson.build
index 811749f..e97f671 100644
--- a/meson.build
+++ b/meson.build
@@ -67,6 +67,8 @@
'-DTELEMETRY_MAX_TRIGGERS=' + get_option('max-triggers').to_string(),
'-DTELEMETRY_MAX_DBUS_PATH_LENGTH=' +
get_option('max-dbus-path-length').to_string(),
+ '-DTELEMETRY_MAX_APPEND_LIMIT=' +
+ get_option('max-append-limit').to_string(),
language: 'cpp'
)
diff --git a/meson_options.txt b/meson_options.txt
index 04ed22e..23ec2aa 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -2,10 +2,12 @@
option('max-reports', type: 'integer', min: 1, value: 10,
description: 'Max number of Reports')
option('max-reading-parameters', type: 'integer', min: 1, value: 200,
- description: 'Max number of reading parameters in single report')
+ description: 'Max number of metric properties in single report')
option('min-interval', type: 'integer', min: 1, value: 1000,
description: 'Minimal value of interval in milliseconds')
option('max-triggers', type: 'integer', min: 1, value: 10,
description: 'Max number of Triggers')
option('max-dbus-path-length', type: 'integer', min: 1, value: 4095,
description: 'Max length of dbus object path')
+option('max-append-limit', type: 'integer', min: 0, value: 256,
+ description: 'Max AppendLimit value')
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index 7ac8537..8687316 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -127,7 +127,7 @@
void ReportManager::verifyAddReport(
const std::string& reportId, const std::string& reportName,
const ReportingType reportingType, Milliseconds interval,
- const ReportUpdates reportUpdates,
+ const ReportUpdates reportUpdates, const uint64_t appendLimit,
const std::vector<LabeledMetricParameters>& readingParams)
{
if (reportingType == ReportingType::onChange)
@@ -144,14 +144,29 @@
"Reached maximal report count");
}
+ if (appendLimit > maxAppendLimit)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Append limit out of range");
+ }
+
if (reportingType == ReportingType::periodic && interval < minInterval)
{
throw sdbusplus::exception::SdBusError(
static_cast<int>(std::errc::invalid_argument), "Invalid interval");
}
- if (readingParams.size() > maxReadingParams)
+ size_t metricCount = 0;
+ for (auto metricParam : readingParams)
+ {
+ auto metricParamsVec =
+ metricParam.at_label<utils::tstring::SensorPath>();
+ metricCount += metricParamsVec.size();
+ }
+ if (readingParams.size() > maxNumberMetrics ||
+ metricCount > maxNumberMetrics)
{
throw sdbusplus::exception::SdBusError(
static_cast<int>(std::errc::argument_list_too_long),
@@ -205,7 +220,7 @@
existingReportIds, maxReportIdLength);
verifyAddReport(id, name, reportingType, interval, reportUpdates,
- labeledMetricParams);
+ appendLimit, labeledMetricParams);
reports.emplace_back(reportFactory->make(
id, name, reportingType, reportActions, interval, appendLimit,
diff --git a/src/report_manager.hpp b/src/report_manager.hpp
index 34f98e0..93b2986 100644
--- a/src/report_manager.hpp
+++ b/src/report_manager.hpp
@@ -42,7 +42,7 @@
void verifyAddReport(
const std::string& reportId, const std::string& reportName,
const ReportingType reportingType, Milliseconds interval,
- const ReportUpdates reportUpdates,
+ const ReportUpdates reportUpdates, const uint64_t appendLimit,
const std::vector<LabeledMetricParameters>& readingParams);
interfaces::Report& addReport(
boost::asio::yield_context& yield, const std::string& reportId,
@@ -60,11 +60,12 @@
public:
static constexpr size_t maxReports{TELEMETRY_MAX_REPORTS};
- static constexpr size_t maxReadingParams{TELEMETRY_MAX_READING_PARAMS};
+ static constexpr size_t maxNumberMetrics{TELEMETRY_MAX_READING_PARAMS};
static constexpr size_t maxReportIdLength{
TELEMETRY_MAX_DBUS_PATH_LENGTH -
std::string_view(Report::reportDir).length()};
static constexpr Milliseconds minInterval{TELEMETRY_MIN_INTERVAL};
+ static constexpr size_t maxAppendLimit{TELEMETRY_MAX_APPEND_LIMIT};
static constexpr const char* reportManagerIfaceName =
"xyz.openbmc_project.Telemetry.ReportManager";
static constexpr const char* reportManagerPath =
diff --git a/tests/src/test_report_manager.cpp b/tests/src/test_report_manager.cpp
index 05a00ed..9af5c57 100644
--- a/tests/src/test_report_manager.cpp
+++ b/tests/src/test_report_manager.cpp
@@ -207,16 +207,32 @@
EXPECT_THAT(path, Eq(std::string()));
}
-TEST_F(TestReportManager, DISABLED_failToAddReportWithMoreSensorsThanExpected)
+TEST_F(TestReportManager,
+ DISABLED_failToAddReportWithMoreMetricPropertiesThanExpected)
{
reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
.Times(0);
+ reportParams.metricParameters(
+ std::vector<LabeledMetricParameters>{{LabeledMetricParameters{
+ {LabeledSensorParameters{"Service",
+ "/xyz/openbmc_project/sensors/power/p1",
+ "Metadata1"}},
+ OperationType::single,
+ "MetricId1",
+ CollectionTimeScope::point,
+ CollectionDuration(Milliseconds(0u))}}});
+
auto metricParams = reportParams.metricParameters();
- for (size_t i = 0; i < ReportManager::maxReadingParams + 1; i++)
+ auto& metricParamsVec =
+ metricParams[0].at_label<utils::tstring::SensorPath>();
+
+ for (size_t i = 0; i < ReportManager::maxNumberMetrics; i++)
{
- metricParams.push_back(metricParams.front());
+ metricParamsVec.emplace_back(LabeledSensorParameters{
+ "Service", "/xyz/openbmc_project/sensors/power/p1", "Metadata1"});
}
+
reportParams.metricParameters(std::move(metricParams));
auto [ec, path] = addReport(reportParams);
@@ -225,6 +241,44 @@
EXPECT_THAT(path, Eq(std::string()));
}
+TEST_F(TestReportManager, DISABLED_failToAddReportWithMoreMetricsThanExpected)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ auto metricParams = std::vector<LabeledMetricParameters>{};
+
+ for (size_t i = 0; i < ReportManager::maxNumberMetrics + 1; i++)
+ {
+ metricParams.emplace_back(
+ LabeledMetricParameters{{},
+ OperationType::single,
+ "MetricId1",
+ CollectionTimeScope::point,
+ CollectionDuration(Milliseconds(0u))});
+ }
+
+ reportParams.metricParameters(std::move(metricParams));
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::argument_list_too_long));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithAppendLimitGreaterThanMax)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ reportParams.appendLimit(ReportManager::maxAppendLimit + 1);
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
TEST_F(TestReportManager, DISABLED_failToAddReportWhenMaxReportIsReached)
{
reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))