From 230d44101b32f49466265b3c3dd7c9b9e6b96763 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 14 Jul 2023 17:46:30 +0000 Subject: [PATCH] Do not use static initialization to register modules (#1422) 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. --- src/CollapsedForwarding.cc | 2 +- src/DiskIO/IpcIo/IpcIoFile.cc | 2 +- src/MemStore.cc | 2 +- src/PeerPoolMgr.cc | 2 +- src/Transients.cc | 2 +- src/auth/ntlm/Scheme.cc | 3 ++- src/base/RunnersRegistry.cc | 6 ----- src/base/RunnersRegistry.h | 48 ++++++++++++++++++++++++++++------ src/cache_cf.cc | 2 +- src/client_db.cc | 2 +- src/dns_internal.cc | 4 +-- src/esi/ExpatParser.cc | 4 +-- src/esi/Libxml2Parser.cc | 4 +-- src/fs/rock/RockSwapDir.cc | 5 +--- src/ipc/mem/Pages.cc | 2 +- src/main.cc | 49 +++++++++++++++++++++++++++++++++++ src/security/Session.cc | 2 +- 17 files changed, 107 insertions(+), 34 deletions(-) diff --git a/src/CollapsedForwarding.cc b/src/CollapsedForwarding.cc index 4d87420bb2..62d8a993cf 100644 --- a/src/CollapsedForwarding.cc +++ b/src/CollapsedForwarding.cc @@ -186,7 +186,7 @@ private: Ipc::MultiQueue::Owner *owner; }; -RunnerRegistrationEntry(CollapsedForwardingRr); +DefineRunnerRegistrator(CollapsedForwardingRr); void CollapsedForwardingRr::create() { diff --git a/src/DiskIO/IpcIo/IpcIoFile.cc b/src/DiskIO/IpcIo/IpcIoFile.cc index 6aab46794b..c156a1dc26 100644 --- a/src/DiskIO/IpcIo/IpcIoFile.cc +++ b/src/DiskIO/IpcIo/IpcIoFile.cc @@ -1026,7 +1026,7 @@ private: Ipc::FewToFewBiQueue::Owner *owner; }; -RunnerRegistrationEntry(IpcIoRr); +DefineRunnerRegistrator(IpcIoRr); void IpcIoRr::claimMemoryNeeds() diff --git a/src/MemStore.cc b/src/MemStore.cc index 009c12a64c..fd329e0702 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -987,7 +987,7 @@ private: Ipc::Mem::Owner *extrasOwner; ///< PageIds Owner }; -RunnerRegistrationEntry(MemStoreRr); +DefineRunnerRegistrator(MemStoreRr); void MemStoreRr::claimMemoryNeeds() diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 58f203eea4..7becaf636d 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -239,7 +239,7 @@ public: void syncConfig() override; }; -RunnerRegistrationEntry(PeerPoolMgrsRr); +DefineRunnerRegistrator(PeerPoolMgrsRr); void PeerPoolMgrsRr::syncConfig() diff --git a/src/Transients.cc b/src/Transients.cc index fbcf832c1d..e33659138c 100644 --- a/src/Transients.cc +++ b/src/Transients.cc @@ -393,7 +393,7 @@ private: TransientsMap::Owner *mapOwner = nullptr; }; -RunnerRegistrationEntry(TransientsRr); +DefineRunnerRegistrator(TransientsRr); void TransientsRr::useConfig() diff --git a/src/auth/ntlm/Scheme.cc b/src/auth/ntlm/Scheme.cc index e0a720d102..00d92759d2 100644 --- a/src/auth/ntlm/Scheme.cc +++ b/src/auth/ntlm/Scheme.cc @@ -23,7 +23,8 @@ public: debugs(29, 2, "Initialized Authentication Scheme '" << type << "'"); } }; -RunnerRegistrationEntry(NtlmAuthRr); + +DefineRunnerRegistrator(NtlmAuthRr); Auth::Scheme::Pointer Auth::Ntlm::Scheme::GetInstance() diff --git a/src/base/RunnersRegistry.cc b/src/base/RunnersRegistry.cc index de4281fb7d..896d285e8f 100644 --- a/src/base/RunnersRegistry.cc +++ b/src/base/RunnersRegistry.cc @@ -108,9 +108,3 @@ IndependentRunner::registerRunner() // else do nothing past finishShutdown } -bool -UseThisStatic(const void *) -{ - return true; -} - diff --git a/src/base/RunnersRegistry.h b/src/base/RunnersRegistry.h index b9e217d83b..c28696fbea 100644 --- a/src/base/RunnersRegistry.h +++ b/src/base/RunnersRegistry.h @@ -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 */ diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 643d88f26b..31eb3ca249 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -4374,7 +4374,7 @@ public: Ssl::BumpMode sslBumpCfgRr::lastDeprecatedRule = Ssl::bumpEnd; -RunnerRegistrationEntry(sslBumpCfgRr); +DefineRunnerRegistrator(sslBumpCfgRr); void sslBumpCfgRr::finalizeConfig() diff --git a/src/client_db.cc b/src/client_db.cc index 3d377f77a0..ac70562cf2 100644 --- a/src/client_db.cc +++ b/src/client_db.cc @@ -102,7 +102,7 @@ public: /* RegisteredRunner API */ void useConfig() override; }; -RunnerRegistrationEntry(ClientDbRr); +DefineRunnerRegistrator(ClientDbRr); void ClientDbRr::useConfig() diff --git a/src/dns_internal.cc b/src/dns_internal.cc index 319d344003..de3171a6e3 100644 --- a/src/dns_internal.cc +++ b/src/dns_internal.cc @@ -210,10 +210,10 @@ public: void endingShutdown() override; }; -RunnerRegistrationEntry(ConfigRr); - } // namespace Dns +DefineRunnerRegistratorIn(Dns, ConfigRr); + struct _sp { char domain[NS_MAXDNAME]; int queries; diff --git a/src/esi/ExpatParser.cc b/src/esi/ExpatParser.cc index 126fb3c801..5b1a925776 100644 --- a/src/esi/ExpatParser.cc +++ b/src/esi/ExpatParser.cc @@ -32,10 +32,10 @@ private: std::unique_ptr registration; }; -RunnerRegistrationEntry(ExpatRr); - } +DefineRunnerRegistratorIn(Esi, ExpatRr); + EsiParserDefinition(ESIExpatParser); ESIExpatParser::ESIExpatParser(ESIParserClient *aClient) : theClient (aClient) diff --git a/src/esi/Libxml2Parser.cc b/src/esi/Libxml2Parser.cc index 0f2e770682..f037528851 100644 --- a/src/esi/Libxml2Parser.cc +++ b/src/esi/Libxml2Parser.cc @@ -36,10 +36,10 @@ private: std::unique_ptr registration; }; -RunnerRegistrationEntry(Libxml2Rr); - } +DefineRunnerRegistratorIn(Esi, Libxml2Rr); + // the global document that will store the resolved entity // definitions static htmlDocPtr entity_doc = nullptr; diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc index e14487b43f..ad376aad26 100644 --- a/src/fs/rock/RockSwapDir.cc +++ b/src/fs/rock/RockSwapDir.cc @@ -1117,10 +1117,7 @@ Rock::SwapDir::hasReadableEntry(const StoreEntry &e) const return map->hasReadableEntry(reinterpret_cast(e.key)); } -namespace Rock -{ -RunnerRegistrationEntry(SwapDirRr); -} +DefineRunnerRegistratorIn(Rock, SwapDirRr); void Rock::SwapDirRr::create() { diff --git a/src/ipc/mem/Pages.cc b/src/ipc/mem/Pages.cc index 9abcaca40e..939f339a04 100644 --- a/src/ipc/mem/Pages.cc +++ b/src/ipc/mem/Pages.cc @@ -103,7 +103,7 @@ private: Ipc::Mem::PagePool::Owner *owner; }; -RunnerRegistrationEntry(SharedMemPagesRr); +DefineRunnerRegistrator(SharedMemPagesRr); void SharedMemPagesRr::useConfig() diff --git a/src/main.cc b/src/main.cc index fc0bd0d686..4c19f7e4ab 100644 --- a/src/main.cc +++ b/src/main.cc @@ -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); diff --git a/src/security/Session.cc b/src/security/Session.cc index d655cb6e41..f09f95b882 100644 --- a/src/security/Session.cc +++ b/src/security/Session.cc @@ -423,7 +423,7 @@ private: Ipc::MemMap::Owner *owner; }; -RunnerRegistrationEntry(SharedSessionCacheRr); +DefineRunnerRegistrator(SharedSessionCacheRr); void SharedSessionCacheRr::useConfig() -- 2.39.2