[trafficserver] branch 9.0.x updated: Assure no SM survives plugin factory deactivation. (#6299)

[zw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zwoop pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/9.0.x by this push:
     new 5813277  Assure no SM survives plugin factory deactivation. (#6299)
5813277 is described below

commit 5813277fa88204b5d18ee926e1c6739c5da4c172
Author: Gancho Tenev <10...@users.noreply.github.com>
AuthorDate: Mon Jan 6 13:05:38 2020 -0800

    Assure no SM survives plugin factory deactivation. (#6299)
    
    Currently plugins sometimes destroy continuations used for hooks
    in TSRemapDeleteInstance. To make sure no SM is still alive when
    TSRemapDeleteInstance is called "deactivating" the remap plugin
    factory as late as possible right before its destruction.
    
    Fixed minor memory leak when a corrupted DSO file loading is
    attempted during config reload, added few extra debug messages.
    
    (9.0.x specific commit, cherry-picked e6cf1bf)
---
 proxy/ReverseProxy.cc             | 13 ++++++-------
 proxy/http/remap/PluginFactory.cc |  9 ++++++++-
 proxy/http/remap/UrlRewrite.cc    |  3 +++
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/proxy/ReverseProxy.cc b/proxy/ReverseProxy.cc
index 0d68c8b..22b44af 100644
--- a/proxy/ReverseProxy.cc
+++ b/proxy/ReverseProxy.cc
@@ -141,7 +141,7 @@ reloadUrlRewrite()
   Debug("url_rewrite", "%s updated, reloading...", ts::filename::REMAP);
   newTable = new UrlRewrite();
   if (newTable->load()) {
-    static const char *msg = "%s finished loading";
+    static const char *msg_format = "%s finished loading";
 
     // Hold at least one lease, until we reload the configuration
     newTable->acquire();
@@ -152,18 +152,17 @@ reloadUrlRewrite()
     ink_assert(oldTable != nullptr);
 
     // Release the old one
-    oldTable->pluginFactory.deactivate();
     oldTable->release();
 
-    Debug("url_rewrite", msg, ts::filename::REMAP);
-    Note("%s", msg);
+    Debug("url_rewrite", msg_format, ts::filename::REMAP);
+    Note(msg_format, ts::filename::REMAP);
     return true;
   } else {
-    static const char *msg = "%s failed to load";
+    static const char *msg_format = "%s failed to load";
 
     delete newTable;
-    Debug("url_rewrite", msg, ts::filename::REMAP);
-    Error(msg, ts::filename::REMAP);
+    Debug("url_rewrite", msg_format, ts::filename::REMAP);
+    Error(msg_format, ts::filename::REMAP);
     return false;
   }
 }
diff --git a/proxy/http/remap/PluginFactory.cc b/proxy/http/remap/PluginFactory.cc
index b55875b..6773384 100644
--- a/proxy/http/remap/PluginFactory.cc
+++ b/proxy/http/remap/PluginFactory.cc
@@ -147,6 +147,8 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
   fs::path effectivePath = getEffectivePath(configPath);
   if (effectivePath.empty()) {
     error.assign("failed to find plugin '").append(configPath.string()).append("'");
+    // The error will be reported by the caller but add debug log entry with this tag for convenience.
+    PluginDebug(_tag, "%s", error.c_str());
     return nullptr;
   }
 
@@ -175,9 +177,12 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
           PluginDso::loadedPlugins()->add(plugin);
           inst = RemapPluginInst::init(plugin, argc, argv, error);
           if (nullptr != inst) {
+            /* Plugin loading and instance init went fine. */
             _instList.append(inst);
           }
         } else {
+          /* Plugin DSO load succeeded but instance init failed. */
+          PluginDebug(_tag, "plugin '%s' instance init failed", configPath.c_str());
           plugin->unload(error);
           delete plugin;
         }
@@ -186,7 +191,9 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
           clean(error);
         }
       } else {
-        return nullptr;
+        /* Plugin DSO load failed. */
+        PluginDebug(_tag, "plugin '%s' DSO load failed", configPath.c_str());
+        delete plugin;
       }
     }
   } else {
diff --git a/proxy/http/remap/UrlRewrite.cc b/proxy/http/remap/UrlRewrite.cc
index c0dc69b..e720e7d 100644
--- a/proxy/http/remap/UrlRewrite.cc
+++ b/proxy/http/remap/UrlRewrite.cc
@@ -106,6 +106,9 @@ UrlRewrite::~UrlRewrite()
   DestroyStore(temporary_redirects);
   DestroyStore(forward_mappings_with_recv_port);
   _valid = false;
+
+  /* Deactivate the factory when all SM are gone for sure. */
+  pluginFactory.deactivate();
 }
 
 /** Sets the reverse proxy flag. */