Fix potential memory corruption due to lifetime of default arguments

Functions which contain constant reference optional arguments must be wary when assigning default values to them. Object construction in headers here are bound to the scope of the function and will be deallocated at the end. Our code suffered heavily from using this pattern to facilite dependency injection that enabled testing. For further reading see the following: https://stackoverflow.com/questions/12554619/what-is-the-lifetime-of-a-default-argument-temporary-bound-to-a-reference-parame
KanoComputing · Oct 24, 2019 · c27f8b3 · c27f8b3
1 parent 5f72d23
commit c27f8b3
Show file tree

Hide file tree

Showing 37 changed files with 240 additions and 280 deletions.
diff --git a/debian/changelog b/debian/changelog
@@ -3,8 +3,9 @@ mercury (4.3.3-0) unstable; urgency=low
   * Compiling project with C++14 standard
   * Fixed issue with KanoWorld, KESDLT, KESMP clients when ran
     under sudo
+  * Fixed potential memory corruption issues
 
- -- Team Kano <[email protected]>  Fri, 3 Oct 2019 14:25:00 +0100
+ -- Team Kano <[email protected]>  Thu, 24 Oct 2019 22:25:00 +0100
 
 mercury (4.3.2-0) unstable; urgency=low
 

diff --git a/include/kes_dashboard_live_tiles_client/ITileFactory.h b/include/kes_dashboard_live_tiles_client/ITileFactory.h
@@ -16,6 +16,7 @@
 #include <string>
 
 #include "kes_dashboard_live_tiles_client/ITile.h"
+#include "mercury/http/http_client_interface.h"
 
 
 namespace KESDLTC {
@@ -35,7 +36,9 @@ class ITileFactory {
         const std::string& app,
         const std::string& openUrl,
         const std::string& fallbackUrl,
-        const std::string& coverPath = "") const = 0;
+        const std::string& coverPath = "",
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient =
+            nullptr) const = 0;
 };
 
 }  // namespace KESDLTC

diff --git a/include/kes_dashboard_live_tiles_client/Tile.h b/include/kes_dashboard_live_tiles_client/Tile.h
@@ -18,8 +18,6 @@
 #include <string>
 
 #include "kes_dashboard_live_tiles_client/ITile.h"
-
-#include "mercury/http/http_client.h"
 #include "mercury/http/http_client_interface.h"
 
 
@@ -28,8 +26,7 @@ namespace KESDLTC {
 class Tile : public ITile {
  public:  // Constructors & destructors.
     explicit Tile(
-        const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>());
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr);  // NOLINT
 
     Tile(const std::string& id,
          const std::string& cover,
@@ -40,10 +37,7 @@ class Tile : public ITile {
          const std::string& openUrl,
          const std::string& fallbackUrl,
          const std::string& coverPath = "",
-         const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>());
-
-    ~Tile();
+         const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr);  // NOLINT
 
  public:  // ISerialisable Methods.
     bool initialise(JSON_Value* serialisedData) override;
@@ -73,7 +67,7 @@ class Tile : public ITile {
     std::string app;
     std::string openUrl;
     std::string fallbackUrl;
-    const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
+    std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
 };
 
 }  // namespace KESDLTC

diff --git a/include/kes_dashboard_live_tiles_client/TileFactory.h b/include/kes_dashboard_live_tiles_client/TileFactory.h
@@ -17,15 +17,12 @@
 
 #include "kes_dashboard_live_tiles_client/ITile.h"
 #include "kes_dashboard_live_tiles_client/ITileFactory.h"
+#include "mercury/http/http_client_interface.h"
 
 
 namespace KESDLTC {
 
 class TileFactory : public ITileFactory {
- public:  // Constructors & destructors.
-    TileFactory();
-    ~TileFactory();
-
  public:  // ITileFactory Methods.
     std::shared_ptr<ITile> create() const override;
     std::shared_ptr<ITile> create(
@@ -37,7 +34,9 @@ class TileFactory : public ITileFactory {
         const std::string& app,
         const std::string& openUrl,
         const std::string& fallbackUrl,
-        const std::string& coverPath = "") const override;
+        const std::string& coverPath = "",
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient =
+            nullptr) const override;
 };
 
 }  // namespace KESDLTC

diff --git a/include/kes_dashboard_live_tiles_client/TileManager.h b/include/kes_dashboard_live_tiles_client/TileManager.h
@@ -28,13 +28,12 @@ namespace KESDLTC {
 
 class TileManager : public ITileManager {
  public:  // Constructors & destructors.
-    TileManager(
+    explicit TileManager(
         const std::string& cacheDir = "",
-        const std::shared_ptr<KESDLTC::internal::IOnlineLoader> onlineLoader = nullptr,  // NOLINT
-        const std::shared_ptr<KESDLTC::internal::ITileCache> tileCache = nullptr,  // NOLINT
-        const std::shared_ptr<KESDLTC::internal::ITileLoader> defaultTileLoader = nullptr,  // NOLINT
-        std::shared_ptr<Mercury::Utils::IEnvironment> env = nullptr);
-    ~TileManager();
+        const std::shared_ptr<KESDLTC::internal::IOnlineLoader>& onlineLoader = nullptr,  // NOLINT
+        const std::shared_ptr<KESDLTC::internal::ITileCache>& tileCache = nullptr,  // NOLINT
+        const std::shared_ptr<KESDLTC::internal::ITileLoader>& defaultTileLoader = nullptr,  // NOLINT
+        const std::shared_ptr<Mercury::Utils::IEnvironment>& env = nullptr);
 
  public:  // ITileManager Methods.
     std::list<std::shared_ptr<ITile>> getTiles(bool cache = false) const override;  // NOLINT

diff --git a/include/kes_dashboard_live_tiles_client/internal/DefaultTileLoader.h b/include/kes_dashboard_live_tiles_client/internal/DefaultTileLoader.h
@@ -17,24 +17,21 @@
 #include "kes_dashboard_live_tiles_client/internal/ITileLoader.h"
 #include "kes_dashboard_live_tiles_client/ITile.h"
 #include "kes_dashboard_live_tiles_client/ITileFactory.h"
-#include "kes_dashboard_live_tiles_client/TileFactory.h"
 
 
 namespace KESDLTC {
 namespace internal {
 
 class DefaultTileLoader : public ITileLoader {
  public:  // Constructors & destructors.
-    DefaultTileLoader(
-        const std::shared_ptr<ITileFactory> tileFactory =
-            std::make_shared<TileFactory>());
-    ~DefaultTileLoader();
+    explicit DefaultTileLoader(
+        const std::shared_ptr<ITileFactory>& tileFactory = nullptr);
 
  public:  // ITileLoader Methods.
     std::list<std::shared_ptr<ITile>> getTiles() override;
 
  private:  // Members.
-    const std::shared_ptr<ITileFactory> tileFactory;
+    std::shared_ptr<ITileFactory> tileFactory;
 };
 
 }  // namespace internal

diff --git a/include/kes_dashboard_live_tiles_client/internal/OnlineLoader.h b/include/kes_dashboard_live_tiles_client/internal/OnlineLoader.h
@@ -19,9 +19,6 @@
 #include "kes_dashboard_live_tiles_client/internal/IOnlineLoader.h"
 #include "kes_dashboard_live_tiles_client/ITile.h"
 #include "kes_dashboard_live_tiles_client/ITileFactory.h"
-#include "kes_dashboard_live_tiles_client/TileFactory.h"
-
-#include "mercury/http/http_client.h"
 #include "mercury/http/http_client_interface.h"
 
 
@@ -36,12 +33,8 @@ class OnlineLoader : public IOnlineLoader {
  public:  // Constructors & destructors.
     explicit OnlineLoader(
         const std::string& cacheDir,
-        const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>(),
-        const std::shared_ptr<ITileFactory> tileFactory =
-            std::make_shared<TileFactory>());
-
-    ~OnlineLoader();
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr,
+        const std::shared_ptr<ITileFactory>& tileFactory = nullptr);
 
  public:  // ITileLoader Methods.
     /**
@@ -58,8 +51,8 @@ class OnlineLoader : public IOnlineLoader {
     // static constexpr const char* KES_DLT_URL = "https://dlt.os.kes.kessandbox.co.uk/";  // NOLINT
 
  private:  // Members.
-    const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
-    const std::shared_ptr<ITileFactory> tileFactory;
+    std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
+    std::shared_ptr<ITileFactory> tileFactory;
     const std::string cacheDir;
 
  private:  // Constants.

diff --git a/include/kes_dashboard_live_tiles_client/internal/TileCache.h b/include/kes_dashboard_live_tiles_client/internal/TileCache.h
@@ -19,7 +19,6 @@
 #include "kes_dashboard_live_tiles_client/internal/ITileCache.h"
 #include "kes_dashboard_live_tiles_client/ITile.h"
 #include "kes_dashboard_live_tiles_client/ITileFactory.h"
-#include "kes_dashboard_live_tiles_client/TileFactory.h"
 
 
 namespace KESDLTC {
@@ -29,9 +28,7 @@ class TileCache : public ITileCache {
  public:  // Constructors & destructors.
     explicit TileCache(
         const std::string& cacheDir,
-        const std::shared_ptr<ITileFactory> tileFactory =
-            std::make_shared<TileFactory>());
-    ~TileCache();
+        const std::shared_ptr<ITileFactory>& tileFactory = nullptr);
 
  public:  // ITileLoader Methods.
     std::list<std::shared_ptr<ITile>> getTiles() override;
@@ -50,9 +47,9 @@ class TileCache : public ITileCache {
     std::string cachePath;
     std::shared_ptr<JSON_Value> cacheDataRoot;
     JSON_Object* cacheData;
+    std::shared_ptr<ITileFactory> tileFactory;
 
  private:  // Constants.
-    const std::shared_ptr<ITileFactory> tileFactory;
     const std::string CACHE_FILE = "cache.json";
 };
 

diff --git a/include/kes_moma_picks_client/IPaintingFactory.h b/include/kes_moma_picks_client/IPaintingFactory.h
@@ -17,6 +17,8 @@
 
 #include "kes_moma_picks_client/IPainting.h"
 
+#include "mercury/http/http_client_interface.h"
+
 
 namespace KESMPC {
 
@@ -34,7 +36,9 @@ class IPaintingFactory {
         const std::string& dateCreated,
         const std::string& openUrl,
         const std::string& fallbackUrl,
-        const std::string& coverPath = "") const = 0;
+        const std::string& coverPath = "",
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient =
+            nullptr) const = 0;
 };
 
 }  // namespace KESMPC

diff --git a/include/kes_moma_picks_client/Painting.h b/include/kes_moma_picks_client/Painting.h
@@ -19,7 +19,6 @@
 
 #include "kes_moma_picks_client/IPainting.h"
 
-#include "mercury/http/http_client.h"
 #include "mercury/http/http_client_interface.h"
 
 
@@ -28,8 +27,7 @@ namespace KESMPC {
 class Painting : public IPainting {
  public:  // Constructors & destructors.
     explicit Painting(
-        const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>());
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr);  // NOLINT
 
     Painting(
         const std::string& id,
@@ -40,10 +38,7 @@ class Painting : public IPainting {
         const std::string& openUrl,
         const std::string& fallbackUrl,
         const std::string& coverPath = "",
-        const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>());
-
-    ~Painting();
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr);  // NOLINT
 
  public:  // ISerialisable Methods.
     bool initialise(JSON_Value* serialisedData) override;
@@ -71,7 +66,7 @@ class Painting : public IPainting {
     std::string dateCreated;
     std::string openUrl;
     std::string fallbackUrl;
-    const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
+    std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
 };
 
 }  // namespace KESMPC

diff --git a/include/kes_moma_picks_client/PaintingFactory.h b/include/kes_moma_picks_client/PaintingFactory.h
@@ -18,14 +18,12 @@
 #include "kes_moma_picks_client/IPainting.h"
 #include "kes_moma_picks_client/IPaintingFactory.h"
 
+#include "mercury/http/http_client_interface.h"
+
 
 namespace KESMPC {
 
 class PaintingFactory : public IPaintingFactory {
- public:  // Constructors & destructors.
-    PaintingFactory();
-    ~PaintingFactory();
-
  public:  // IPaintingFactory Methods.
     std::shared_ptr<IPainting> create() const override;
     std::shared_ptr<IPainting> create(
@@ -36,7 +34,9 @@ class PaintingFactory : public IPaintingFactory {
         const std::string& dateCreated,
         const std::string& openUrl,
         const std::string& fallbackUrl,
-        const std::string& coverPath = "") const override;
+        const std::string& coverPath = "",
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient =
+            nullptr) const override;
 };
 
 }  // namespace KESMPC

diff --git a/include/kes_moma_picks_client/PaintingManager.h b/include/kes_moma_picks_client/PaintingManager.h
@@ -28,13 +28,12 @@ namespace KESMPC {
 
 class PaintingManager : public IPaintingManager {
  public:  // Constructors & destructors.
-    PaintingManager(
+    explicit PaintingManager(
         const std::string& cacheDir = "",
-        const std::shared_ptr<KESMPC::internal::IOnlineLoader> onlineLoader = nullptr,  // NOLINT
-        const std::shared_ptr<KESMPC::internal::IPaintingCache> paintingCache = nullptr,  // NOLINT
-        const std::shared_ptr<KESMPC::internal::IPaintingLoader> defaultPaintingLoader = nullptr,  // NOLINT
-        const std::shared_ptr<Mercury::Utils::IEnvironment> env = nullptr);
-    ~PaintingManager();
+        const std::shared_ptr<KESMPC::internal::IOnlineLoader>& onlineLoader = nullptr,  // NOLINT
+        const std::shared_ptr<KESMPC::internal::IPaintingCache>& paintingCache = nullptr,  // NOLINT
+        const std::shared_ptr<KESMPC::internal::IPaintingLoader>& defaultPaintingLoader = nullptr,  // NOLINT
+        const std::shared_ptr<Mercury::Utils::IEnvironment>& env = nullptr);
 
  public:  // IPaintingManager Methods.
     std::list<std::shared_ptr<IPainting>> getPaintings(bool cache = false) const override;  // NOLINT

diff --git a/include/kes_moma_picks_client/internal/DefaultPaintingLoader.h b/include/kes_moma_picks_client/internal/DefaultPaintingLoader.h
@@ -11,30 +11,28 @@
 #ifndef INCLUDE_KES_MOMA_PICKS_CLIENT_INTERNAL_DEFAULTPAINTINGLOADER_H_
 #define INCLUDE_KES_MOMA_PICKS_CLIENT_INTERNAL_DEFAULTPAINTINGLOADER_H_
 
+
 #include <list>
 #include <memory>
 
 #include "kes_moma_picks_client/internal/IPaintingLoader.h"
 #include "kes_moma_picks_client/IPainting.h"
 #include "kes_moma_picks_client/IPaintingFactory.h"
-#include "kes_moma_picks_client/PaintingFactory.h"
 
 
 namespace KESMPC {
 namespace internal {
 
 class DefaultPaintingLoader : public IPaintingLoader {
  public:  // Constructors & destructors.
-    DefaultPaintingLoader(
-        const std::shared_ptr<IPaintingFactory> paintingFactory =
-            std::make_shared<PaintingFactory>());
-    ~DefaultPaintingLoader();
+    explicit DefaultPaintingLoader(
+        const std::shared_ptr<IPaintingFactory>& paintingFactory = nullptr);
 
  public:  // IPaintingLoader Methods.
     std::list<std::shared_ptr<IPainting>> getPaintings() override;
 
  private:  // Members.
-    const std::shared_ptr<IPaintingFactory> paintingFactory;
+    std::shared_ptr<IPaintingFactory> paintingFactory;
 };
 
 }  // namespace internal

diff --git a/include/kes_moma_picks_client/internal/OnlineLoader.h b/include/kes_moma_picks_client/internal/OnlineLoader.h
@@ -19,9 +19,6 @@
 #include "kes_moma_picks_client/internal/IOnlineLoader.h"
 #include "kes_moma_picks_client/IPainting.h"
 #include "kes_moma_picks_client/IPaintingFactory.h"
-#include "kes_moma_picks_client/PaintingFactory.h"
-
-#include "mercury/http/http_client.h"
 #include "mercury/http/http_client_interface.h"
 
 
@@ -36,12 +33,8 @@ class OnlineLoader : public IOnlineLoader {
  public:  // Constructors & destructors.
     explicit OnlineLoader(
         const std::string& cacheDir,
-        const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient =
-            std::make_shared<Mercury::HTTP::HTTPClient>(),
-        const std::shared_ptr<IPaintingFactory> paintingFactory =
-            std::make_shared<PaintingFactory>());
-
-    ~OnlineLoader();
+        const std::shared_ptr<Mercury::HTTP::IHTTPClient>& httpClient = nullptr,  // NOLINT
+        const std::shared_ptr<IPaintingFactory>& paintingFactory = nullptr);
 
  public:  // IPaintingLoader Methods.
     /**
@@ -58,9 +51,9 @@ class OnlineLoader : public IOnlineLoader {
     // static constexpr const char* KES_MP_URL = "https://mp.os.kes.kessandbox.co.uk/";  // NOLINT
 
  private:  // Members.
-    const std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
-    const std::shared_ptr<IPaintingFactory> paintingFactory;
     const std::string cacheDir;
+    std::shared_ptr<Mercury::HTTP::IHTTPClient> httpClient;
+    std::shared_ptr<IPaintingFactory> paintingFactory;
 
  private:  // Constants.
     const double QUERY_COOLDOWN = ONE_HOUR_MS;