]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Do not use static initialization to register modules (#1422)
authorAlex Rousskov <rousskov@measurement-factory.com>
Fri, 14 Jul 2023 17:46:30 +0000 (17:46 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Sun, 16 Jul 2023 21:42:32 +0000 (21:42 +0000)
    ERROR: ... Unknown authentication scheme 'ntlm'.

When a translation unit does not contain main() and its code is not used
by the rest of the executable, the linker may exclude that translation
unit from the executable. This exclusion by LTO may happen even if that
code _is_ used to initialize static variables in that translation unit:
"If no variable or function is odr-used from a given translation unit,
the non-local variables defined in that translation unit may never be
initialized"[^1].

[^1]: https://en.cppreference.com/w/cpp/language/initialization

For example, src/auth/ntlm/Scheme.o translation unit contains nothing
but NtlmAuthRr class definition and static initialization code. The
linker knows that the rest of Squid does not use NtlmAuthRr and excludes
that translation unit from the squid executable, effectively disabling
NTLM module registration required to parse "auth_param ntlm" directives.

The problem does affect existing NTLM module, and may affect any future
module code as we reduce module's external footprint. This change
converts all RegisteredRunner registrations from using side effects of
static initialization to explicit registration calls from SquidMain().
Relying on "life before main()" is a design bug. This PR fixes that bug
with respect to RegisteredRunner registrations.

Due to indeterminate C++ static initialization order, no module can
require registration before main() starts. Thus, moving registration
timing to the beginning of SquidMain() should have no negative effects.

The new registration API still does not expose main.cc to any module
details (other than the name of the registration function itself). We
still do not need to #include any module headers into main.cc. Compiler
or linker does catch most typos in RegisteredRunner names.

Unfortunately, explicit registration still cannot prevent bugs where we
forget to register a module or fail to register a module due to wrong
registration code guards. Eventually, CI will expose such bugs.

17 files changed:
src/CollapsedForwarding.cc
src/DiskIO/IpcIo/IpcIoFile.cc
src/MemStore.cc
src/PeerPoolMgr.cc
src/Transients.cc
src/auth/ntlm/Scheme.cc
src/base/RunnersRegistry.cc
src/base/RunnersRegistry.h
src/cache_cf.cc
src/client_db.cc
src/dns_internal.cc
src/esi/ExpatParser.cc
src/esi/Libxml2Parser.cc
src/fs/rock/RockSwapDir.cc
src/ipc/mem/Pages.cc
src/main.cc
src/security/Session.cc

index 4d87420bb208f05aaa552a03edf92a0bb3472cbd..62d8a993cfaf14067e326da8312e7d071bb3f779 100644 (file)
@@ -186,7 +186,7 @@ private:
     Ipc::MultiQueue::Owner *owner;
 };
 
-RunnerRegistrationEntry(CollapsedForwardingRr);
+DefineRunnerRegistrator(CollapsedForwardingRr);
 
 void CollapsedForwardingRr::create()
 {
index 6aab46794b180b95ef3d1d840cc3606d41d09b85..c156a1dc2675f9989bb0e7e46a04f2b146f96ad2 100644 (file)
@@ -1026,7 +1026,7 @@ private:
     Ipc::FewToFewBiQueue::Owner *owner;
 };
 
-RunnerRegistrationEntry(IpcIoRr);
+DefineRunnerRegistrator(IpcIoRr);
 
 void
 IpcIoRr::claimMemoryNeeds()
index 009c12a64c6d6c7e0726d0aa5844c3c0d67803de..fd329e0702b2992466c23476654eb78aac7c4b41 100644 (file)
@@ -987,7 +987,7 @@ private:
     Ipc::Mem::Owner<MemStoreMapExtras> *extrasOwner; ///< PageIds Owner
 };
 
-RunnerRegistrationEntry(MemStoreRr);
+DefineRunnerRegistrator(MemStoreRr);
 
 void
 MemStoreRr::claimMemoryNeeds()
index 58f203eea4538cb13d057ab76ad9f0b39cbbf13e..7becaf636d7ab26349ebfb26fd11adea4aff2795 100644 (file)
@@ -239,7 +239,7 @@ public:
     void syncConfig() override;
 };
 
-RunnerRegistrationEntry(PeerPoolMgrsRr);
+DefineRunnerRegistrator(PeerPoolMgrsRr);
 
 void
 PeerPoolMgrsRr::syncConfig()
index fbcf832c1d3045bd129a10cb68d455e177bbdc58..e33659138c9b98143b04134a3efedbc0b76d8f21 100644 (file)
@@ -393,7 +393,7 @@ private:
     TransientsMap::Owner *mapOwner = nullptr;
 };
 
-RunnerRegistrationEntry(TransientsRr);
+DefineRunnerRegistrator(TransientsRr);
 
 void
 TransientsRr::useConfig()
index e0a720d102e183dde02b18986785086e2eff3fd9..00d92759d2db4b0dac59be073a002b67a557a187 100644 (file)
@@ -23,7 +23,8 @@ public:
         debugs(29, 2, "Initialized Authentication Scheme '" << type << "'");
     }
 };
-RunnerRegistrationEntry(NtlmAuthRr);
+
+DefineRunnerRegistrator(NtlmAuthRr);
 
 Auth::Scheme::Pointer
 Auth::Ntlm::Scheme::GetInstance()
index de4281fb7d24d23ab356fdb484abe00a64815553..896d285e8f47c9be7241db8e6f5955ea97429f9f 100644 (file)
@@ -108,9 +108,3 @@ IndependentRunner::registerRunner()
     // else do nothing past finishShutdown
 }
 
-bool
-UseThisStatic(const void *)
-{
-    return true;
-}
-
index b9e217d83b2790aa8a77067d84a7ad45b32c8808..c28696fbea3d70020dbf7336a67970a10ab10c7e 100644 (file)
@@ -118,14 +118,46 @@ protected:
     debugs(1, 2, "running " # m); \
     RunRegistered(&m)
 
-/// convenience function to "use" an otherwise unreferenced static variable
-bool UseThisStatic(const void *);
-
-/// convenience macro: register one RegisteredRunner kid as early as possible
-#define RunnerRegistrationEntry(Who) \
-    static const bool Who ## _Registered_ = \
-        RegisterRunner(new Who) > 0 && \
-        UseThisStatic(& Who ## _Registered_);
+/// helps DefineRunnerRegistrator() and CallRunnerRegistrator() (and their
+/// namespace-sensitive variants) to use the same registration function name
+#define NameRunnerRegistrator_(Namespace, ClassName) \
+    Register ## ClassName ## In ## Namespace()
+
+/// helper macro that implements DefineRunnerRegistrator() and
+/// DefineRunnerRegistratorIn() using supplied constructor expression
+#define DefineRunnerRegistrator_(Namespace, ClassName, Constructor) \
+    void NameRunnerRegistrator_(Namespace, ClassName); \
+    void NameRunnerRegistrator_(Namespace, ClassName) { \
+        const auto registered = RegisterRunner(new Constructor); \
+        assert(registered); \
+    }
+
+/// Define registration code for the given RegisteredRunner-derived class known
+/// as ClassName in Namespace. A matching CallRunnerRegistratorIn() call should
+/// run this code before any possible use of the being-registered module.
+/// \sa DefineRunnerRegistrator() for classes declared in the global namespace.
+#define DefineRunnerRegistratorIn(Namespace, ClassName) \
+    DefineRunnerRegistrator_(Namespace, ClassName, Namespace::ClassName)
+
+/// Define registration code for the given RegisteredRunner-derived class known
+/// as ClassName (in global namespace). A matching CallRunnerRegistrator() call
+/// should run this code before any possible use of the being-registered module.
+/// \sa DefineRunnerRegistratorIn() for classes declared in named namespaces.
+#define DefineRunnerRegistrator(ClassName) \
+    DefineRunnerRegistrator_(Global, ClassName, ClassName)
+
+/// Register one RegisteredRunner kid, known as ClassName in Namespace.
+/// \sa CallRunnerRegistrator() for classes declared in the global namespace.
+#define CallRunnerRegistratorIn(Namespace, ClassName) \
+    do { \
+        void NameRunnerRegistrator_(Namespace, ClassName); \
+        NameRunnerRegistrator_(Namespace, ClassName); \
+    } while (false)
+
+/// Register one RegisteredRunner kid, known as ClassName (in the global namespace).
+/// \sa CallRunnerRegistratorIn() for classes declared in named namespaces.
+#define CallRunnerRegistrator(ClassName) \
+    CallRunnerRegistratorIn(Global, ClassName)
 
 #endif /* SQUID_BASE_RUNNERSREGISTRY_H */
 
index 643d88f26beb0c3f41f41426e52fa319fdbcf519..31eb3ca249bfd5a4d6f2c52d4ec6edb0f7544a88 100644 (file)
@@ -4374,7 +4374,7 @@ public:
 
 Ssl::BumpMode sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd;
 
-RunnerRegistrationEntry(sslBumpCfgRr);
+DefineRunnerRegistrator(sslBumpCfgRr);
 
 void
 sslBumpCfgRr::finalizeConfig()
index 3d377f77a028b6f078367c25843055f426be75af..ac70562cf244eb372db3c3e29c76ba8924733140 100644 (file)
@@ -102,7 +102,7 @@ public:
     /* RegisteredRunner API */
     void useConfig() override;
 };
-RunnerRegistrationEntry(ClientDbRr);
+DefineRunnerRegistrator(ClientDbRr);
 
 void
 ClientDbRr::useConfig()
index 319d34400304ca8b3367a697512871c1b06f51f2..de3171a6e3023a35e43d902feed3260d7aca1b67 100644 (file)
@@ -210,10 +210,10 @@ public:
     void endingShutdown() override;
 };
 
-RunnerRegistrationEntry(ConfigRr);
-
 } // namespace Dns
 
+DefineRunnerRegistratorIn(Dns, ConfigRr);
+
 struct _sp {
     char domain[NS_MAXDNAME];
     int queries;
index 126fb3c80144fba9452a2e5ca15abfd284067972..5b1a9257769b096d701dbb409531d6364d134860 100644 (file)
@@ -32,10 +32,10 @@ private:
     std::unique_ptr<ESIParser::Register> registration;
 };
 
-RunnerRegistrationEntry(ExpatRr);
-
 }
 
+DefineRunnerRegistratorIn(Esi, ExpatRr);
+
 EsiParserDefinition(ESIExpatParser);
 
 ESIExpatParser::ESIExpatParser(ESIParserClient *aClient) : theClient (aClient)
index 0f2e770682c35f9f6ce8196401c04e142652a7b5..f0375288511d4a7981df4b59528b3e5c1cef0386 100644 (file)
@@ -36,10 +36,10 @@ private:
     std::unique_ptr<ESIParser::Register> registration;
 };
 
-RunnerRegistrationEntry(Libxml2Rr);
-
 }
 
+DefineRunnerRegistratorIn(Esi, Libxml2Rr);
+
 // the global document that will store the resolved entity
 // definitions
 static htmlDocPtr entity_doc = nullptr;
index e14487b43f364dbd7f102f951d0ef92c966bfdc9..ad376aad26b8aee5903c3a787b641beca179b681 100644 (file)
@@ -1117,10 +1117,7 @@ Rock::SwapDir::hasReadableEntry(const StoreEntry &e) const
     return map->hasReadableEntry(reinterpret_cast<const cache_key*>(e.key));
 }
 
-namespace Rock
-{
-RunnerRegistrationEntry(SwapDirRr);
-}
+DefineRunnerRegistratorIn(Rock, SwapDirRr);
 
 void Rock::SwapDirRr::create()
 {
index 9abcaca40effb39f55c30515d63371fb12af511f..939f339a04ae0a71991e4aa64ccbea8b0a4a2e2d 100644 (file)
@@ -103,7 +103,7 @@ private:
     Ipc::Mem::PagePool::Owner *owner;
 };
 
-RunnerRegistrationEntry(SharedMemPagesRr);
+DefineRunnerRegistrator(SharedMemPagesRr);
 
 void
 SharedMemPagesRr::useConfig()
index fc0bd0d686bccfbd90b6d513c67e76d624fd8d96..4c19f7e4ab00eaf84f085dc0839f9b147b03b014 100644 (file)
@@ -1437,9 +1437,58 @@ StartUsingConfig()
     }
 }
 
+/// register all known modules for handling future RegisteredRunner events
+static void
+RegisterModules()
+{
+    // These registration calls do not represent a RegisteredRunner "event". The
+    // modules registered here should be initialized later, during those events.
+
+    // RegisteredRunner event handlers should not depend on handler call order
+    // and, hence, should not depend on the registration call order below.
+
+    CallRunnerRegistrator(ClientDbRr);
+    CallRunnerRegistrator(CollapsedForwardingRr);
+    CallRunnerRegistrator(MemStoreRr);
+    CallRunnerRegistrator(PeerPoolMgrsRr);
+    CallRunnerRegistrator(SharedMemPagesRr);
+    CallRunnerRegistrator(SharedSessionCacheRr);
+    CallRunnerRegistrator(TransientsRr);
+    CallRunnerRegistratorIn(Dns, ConfigRr);
+
+#if HAVE_DISKIO_MODULE_IPCIO
+    CallRunnerRegistrator(IpcIoRr);
+#endif
+
+#if HAVE_AUTH_MODULE_NTLM
+    CallRunnerRegistrator(NtlmAuthRr);
+#endif
+
+#if USE_OPENSSL
+    CallRunnerRegistrator(sslBumpCfgRr);
+#endif
+
+#if USE_SQUID_ESI && HAVE_LIBEXPAT
+    CallRunnerRegistratorIn(Esi, ExpatRr);
+#endif
+
+#if USE_SQUID_ESI && HAVE_LIBXML2
+    CallRunnerRegistratorIn(Esi, Libxml2Rr);
+#endif
+
+#if HAVE_FS_ROCK
+    CallRunnerRegistratorIn(Rock, SwapDirRr);
+#endif
+}
+
 int
 SquidMain(int argc, char **argv)
 {
+    // We must register all modules before the first RunRegisteredHere() call.
+    // We do it ASAP/here so that we do not need to move this code when we add
+    // earlier hooks to the RegisteredRunner API.
+    RegisterModules();
+
     const CommandLine cmdLine(argc, argv, shortOpStr, squidOptions);
 
     ConfigureCurrentKid(cmdLine);
index d655cb6e41ce2d25e794e9ae4974803110be308a..f09f95b8826094dfe647c2e8cbb509c5113a9ac8 100644 (file)
@@ -423,7 +423,7 @@ private:
     Ipc::MemMap::Owner *owner;
 };
 
-RunnerRegistrationEntry(SharedSessionCacheRr);
+DefineRunnerRegistrator(SharedSessionCacheRr);
 
 void
 SharedSessionCacheRr::useConfig()