From 3fe31a5a6cbd469a91c67a02b9517b3e5fd81011 Mon Sep 17 00:00:00 2001
From: Maxim Pimenov <m@maps.me>
Date: Tue, 22 Aug 2017 20:53:00 +0300
Subject: [PATCH] [routing] Fixed a data race in AltitudeLoader.

AltitudeLoader holds a FileReader of the altitudes section.
This reader, being a caching one, is not thread safe. However,
AltitudeLoader did not keep the MwmHandle from which it was
supposed to construct its reader, so the reader could be reused
(via MwmValue via MwmHandle) by another thread.

FeaturesRoadGraph tries to avoid this issue by keeping its
own cache of MwmHandles but it did not stop RoadGeometry
from bypassing this cache when creating its AltitudeLoader.

The question of whether we need to fix this other cache
and whether we need it at all is out of the scope of this CL.
---
 indexer/altitude_loader.cpp     | 8 +++++++-
 indexer/altitude_loader.hpp     | 4 +++-
 routing/features_road_graph.cpp | 7 ++++---
 routing/features_road_graph.hpp | 3 ++-
 routing/geometry.cpp            | 2 +-
 5 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/indexer/altitude_loader.cpp b/indexer/altitude_loader.cpp
index cb05bc4cf6..57e8189064 100644
--- a/indexer/altitude_loader.cpp
+++ b/indexer/altitude_loader.cpp
@@ -29,8 +29,14 @@ void LoadAndMap(size_t dataSize, ReaderSource<FilesContainerR::TReader> & src, T
 
 namespace feature
 {
-AltitudeLoader::AltitudeLoader(MwmValue const & mwmValue)
+AltitudeLoader::AltitudeLoader(Index const & index, MwmSet::MwmId const & mwmId)
+  : m_handle(index.GetMwmHandleById(mwmId))
 {
+  if (!m_handle.IsAlive())
+    return;
+
+  auto const & mwmValue = *m_handle.GetValue<MwmValue>();
+
   m_countryFileName = mwmValue.GetCountryFileName();
 
   if (mwmValue.GetHeader().GetFormat() < version::Format::v8)
diff --git a/indexer/altitude_loader.hpp b/indexer/altitude_loader.hpp
index a828c673fd..543e2a3036 100644
--- a/indexer/altitude_loader.hpp
+++ b/indexer/altitude_loader.hpp
@@ -1,6 +1,7 @@
 #pragma once
 #include "indexer/feature_altitude.hpp"
 #include "indexer/index.hpp"
+#include "indexer/mwm_set.hpp"
 
 #include "coding/memory_region.hpp"
 
@@ -15,7 +16,7 @@ namespace feature
 class AltitudeLoader
 {
 public:
-  explicit AltitudeLoader(MwmValue const & mwmValue);
+  explicit AltitudeLoader(Index const & index, MwmSet::MwmId const & mwmId);
 
   /// \returns altitude of feature with |featureId|. All items of the returned vector are valid
   /// or the returned vector is empty.
@@ -36,5 +37,6 @@ private:
   map<uint32_t, TAltitudes> m_cache;
   AltitudeHeader m_header;
   string m_countryFileName;
+  MwmSet::MwmHandle m_handle;
 };
 }  // namespace feature
diff --git a/routing/features_road_graph.cpp b/routing/features_road_graph.cpp
index 7a7392f948..138fce0e1e 100644
--- a/routing/features_road_graph.cpp
+++ b/routing/features_road_graph.cpp
@@ -27,12 +27,13 @@ double constexpr kMwmCrossingNodeEqualityRadiusMeters = 100.0;
 
 }  // namespace
 
-FeaturesRoadGraph::Value::Value(MwmSet::MwmHandle handle) : m_mwmHandle(move(handle))
+FeaturesRoadGraph::Value::Value(Index const & index, MwmSet::MwmHandle handle)
+  : m_mwmHandle(move(handle))
 {
   if (!m_mwmHandle.IsAlive())
     return;
 
-  m_altitudeLoader = make_unique<feature::AltitudeLoader>(*m_mwmHandle.GetValue<MwmValue>());
+  m_altitudeLoader = make_unique<feature::AltitudeLoader>(index, m_mwmHandle.GetId());
 }
 
 FeaturesRoadGraph::CrossCountryVehicleModel::CrossCountryVehicleModel(
@@ -331,7 +332,7 @@ FeaturesRoadGraph::Value const & FeaturesRoadGraph::LockMwm(MwmSet::MwmId const
   if (itr != m_mwmLocks.end())
     return itr->second;
 
-  return m_mwmLocks.insert(make_pair(move(mwmId), Value(m_index.GetMwmHandleById(mwmId))))
+  return m_mwmLocks.insert(make_pair(move(mwmId), Value(m_index, m_index.GetMwmHandleById(mwmId))))
       .first->second;
 }
 }  // namespace routing
diff --git a/routing/features_road_graph.hpp b/routing/features_road_graph.hpp
index d44a6d9c77..6b8bc896fd 100644
--- a/routing/features_road_graph.hpp
+++ b/routing/features_road_graph.hpp
@@ -6,6 +6,7 @@
 
 #include "indexer/altitude_loader.hpp"
 #include "indexer/feature_data.hpp"
+#include "indexer/index.hpp"
 #include "indexer/mwm_set.hpp"
 
 #include "geometry/point2d.hpp"
@@ -87,7 +88,7 @@ private:
   struct Value
   {
     Value() = default;
-    explicit Value(MwmSet::MwmHandle handle);
+    explicit Value(Index const & index, MwmSet::MwmHandle handle);
 
     bool IsAlive() const { return m_mwmHandle.IsAlive(); }
 
diff --git a/routing/geometry.cpp b/routing/geometry.cpp
index a40d92ad42..d12fff0ff2 100644
--- a/routing/geometry.cpp
+++ b/routing/geometry.cpp
@@ -38,7 +38,7 @@ GeometryLoaderImpl::GeometryLoaderImpl(Index const & index, MwmSet::MwmHandle co
   : m_vehicleModel(move(vehicleModel))
   , m_guard(index, handle.GetId())
   , m_country(handle.GetInfo()->GetCountryName())
-  , m_altitudeLoader(*handle.GetValue<MwmValue>())
+  , m_altitudeLoader(index, handle.GetId())
   , m_loadAltitudes(loadAltitudes)
 {
   CHECK(handle.IsAlive(), ());