From 99cc045868c5b1fd6d2b31135c60fff6e85f9f42 Mon Sep 17 00:00:00 2001 From: Tom Veasey Date: Tue, 14 Jul 2020 12:54:20 +0100 Subject: [PATCH] [ML] Fix for SIGSEGV forecasting (#1402) It is possible that a model which hasn't received any recent updates is removed. In this case, when forecasting we'd hit a null pointer dereference. This moves the offending code under the check that the model still exists. --- docs/CHANGELOG.asciidoc | 1 + lib/model/CAnomalyDetector.cc | 27 +++++++++++++-------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index a71c21fcd7..baca12ba96 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -33,6 +33,7 @@ === Bug Fixes * Fix restoration of change detectors after seasonality change. (See {ml-pull}1391[#1391].) +* Fix potential SIGSEGV when forecasting. (See {ml-pull}1402[#1402], issue: {ml-issue}1401[#1401].) == {es} version 7.8.1 diff --git a/lib/model/CAnomalyDetector.cc b/lib/model/CAnomalyDetector.cc index 2a851ec056..2e8e26753c 100644 --- a/lib/model/CAnomalyDetector.cc +++ b/lib/model/CAnomalyDetector.cc @@ -499,14 +499,14 @@ CAnomalyDetector::getForecastModels(bool persistOnDisk, return series; } - TModelDetailsViewUPtr view = m_Model.get()->details(); + TModelDetailsViewUPtr view{m_Model.get()->details()}; // The view can be empty, e.g. for the counting model. if (view.get() == nullptr) { return series; } - const CSearchKey& key = m_DataGatherer->searchKey(); + const CSearchKey& key{m_DataGatherer->searchKey()}; series.s_ByFieldName = key.byFieldName(); series.s_DetectorIndex = m_DetectorIndex; series.s_PartitionFieldName = key.partitionFieldName(); @@ -516,16 +516,15 @@ CAnomalyDetector::getForecastModels(bool persistOnDisk, if (persistOnDisk) { CForecastModelPersist::CPersist persister(persistenceFolder); - for (std::size_t pid = 0u, maxPid = m_DataGatherer->numberPeople(); - pid < maxPid; ++pid) { + for (std::size_t pid = 0; pid < m_DataGatherer->numberPeople(); ++pid) { // todo: Add terms filtering here if (m_DataGatherer->isPersonActive(pid)) { for (auto feature : view->features()) { - const maths::CModel* model = view->model(feature, pid); - core_t::TTime firstDataTime; - core_t::TTime lastDataTime; - std::tie(firstDataTime, lastDataTime) = view->dataTimeInterval(pid); + const maths::CModel* model{view->model(feature, pid)}; if (model != nullptr && model->isForecastPossible()) { + core_t::TTime firstDataTime; + core_t::TTime lastDataTime; + std::tie(firstDataTime, lastDataTime) = view->dataTimeInterval(pid); persister.addModel(model, firstDataTime, lastDataTime, feature, m_DataGatherer->personName(pid)); } @@ -535,16 +534,16 @@ CAnomalyDetector::getForecastModels(bool persistOnDisk, series.s_ToForecastPersisted = persister.finalizePersistAndGetFile(); } else { - for (std::size_t pid = 0u, maxPid = m_DataGatherer->numberPeople(); - pid < maxPid; ++pid) { + + for (std::size_t pid = 0; pid < m_DataGatherer->numberPeople(); ++pid) { // todo: Add terms filtering here if (m_DataGatherer->isPersonActive(pid)) { for (auto feature : view->features()) { - const maths::CModel* model = view->model(feature, pid); - core_t::TTime firstDataTime; - core_t::TTime lastDataTime; - std::tie(firstDataTime, lastDataTime) = view->dataTimeInterval(pid); + const maths::CModel* model{view->model(feature, pid)}; if (model != nullptr && model->isForecastPossible()) { + core_t::TTime firstDataTime; + core_t::TTime lastDataTime; + std::tie(firstDataTime, lastDataTime) = view->dataTimeInterval(pid); series.s_ToForecast.emplace_back( feature, m_DataGatherer->personName(pid), CForecastDataSink::TMathsModelPtr(model->cloneForForecast()),