From bc35e3f45231007f8af02da9daa8c94119a780b4 Mon Sep 17 00:00:00 2001 From: Felix Hanau Date: Tue, 20 Aug 2024 01:37:33 +0000 Subject: [PATCH] Mark io target static variables as const to avoid race conditions Also cleans up remaining instances of uninitialized variables in that target. --- src/workerd/api/crypto/ec.c++ | 6 +- src/workerd/api/crypto/impl.c++ | 2 +- src/workerd/api/crypto/impl.h | 2 +- src/workerd/api/crypto/keys.c++ | 4 +- src/workerd/api/crypto/rsa.c++ | 8 +- src/workerd/api/crypto/x509.c++ | 8 +- src/workerd/api/encoding.c++ | 4 +- src/workerd/api/encoding.h | 4 +- src/workerd/api/eventsource.c++ | 2 +- src/workerd/api/form-data.c++ | 2 +- src/workerd/api/node/crypto-keys.c++ | 2 +- src/workerd/api/sql.c++ | 8 +- src/workerd/api/sql.h | 8 +- src/workerd/api/streams/queue-test.c++ | 5 +- src/workerd/api/streams/readable.c++ | 2 +- src/workerd/api/url.c++ | 4 +- src/workerd/io/actor-cache.c++ | 2 +- src/workerd/io/actor-cache.h | 4 +- src/workerd/io/actor-sqlite.c++ | 2 +- src/workerd/io/actor-sqlite.h | 4 +- src/workerd/io/compatibility-date.c++ | 4 +- src/workerd/io/io-gate.c++ | 4 +- src/workerd/io/io-gate.h | 8 +- src/workerd/io/worker.c++ | 2 +- src/workerd/jsg/dom-exception.c++ | 2 +- src/workerd/jsg/jsg.h | 2 +- src/workerd/jsg/modules.c++ | 2 +- src/workerd/jsg/resource.h | 2 +- src/workerd/server/server.c++ | 2 +- src/workerd/server/workerd-api.c++ | 8 +- src/workerd/server/workerd-api.h | 2 +- src/workerd/tests/test-fixture.c++ | 2 +- src/workerd/util/mimetype-test.c++ | 4 +- src/workerd/util/sqlite-test.c++ | 6 +- src/workerd/util/sqlite.c++ | 21 +++-- src/workerd/util/sqlite.h | 121 ++++++++++++------------- 36 files changed, 139 insertions(+), 136 deletions(-) diff --git a/src/workerd/api/crypto/ec.c++ b/src/workerd/api/crypto/ec.c++ index ffeaaaf2d29..da970ae6709 100644 --- a/src/workerd/api/crypto/ec.c++ +++ b/src/workerd/api/crypto/ec.c++ @@ -581,7 +581,7 @@ kj::Own ellipticJwkReader(int curveId, SubtleCrypto::JsonWebKey&& keyD KJ_IF_SOME(alg, keyDataJwk.alg) { // If this JWK specifies an algorithm, make sure it jives with the hash we were passed via // importKey(). - static std::map ecdsaAlgorithms { + static const std::map ecdsaAlgorithms { {"ES256", NID_X9_62_prime256v1}, {"ES384", NID_secp384r1}, {"ES512", NID_secp521r1}, @@ -999,8 +999,8 @@ template AsymmetricKeyCryptoKeyImpl::exportKeyExt( auto bio = OSSL_BIO_MEM(); struct EncDetail { - char* pass = &EMPTY_PASSPHRASE[0]; + char* pass = const_cast(&EMPTY_PASSPHRASE[0]); size_t pass_len = 0; const EVP_CIPHER* cipher = nullptr; }; diff --git a/src/workerd/api/crypto/rsa.c++ b/src/workerd/api/crypto/rsa.c++ index d16778c8382..590cde926c9 100644 --- a/src/workerd/api/crypto/rsa.c++ +++ b/src/workerd/api/crypto/rsa.c++ @@ -902,19 +902,19 @@ kj::Own CryptoKey::Impl::importRsa( KJ_IF_SOME(alg, keyDataJwk.alg) { // If this JWK specifies an algorithm, make sure it jives with the hash we were passed via // importKey(). - static std::map rsaShaAlgorithms{ + static const std::map rsaShaAlgorithms{ {"RS1", EVP_sha1()}, {"RS256", EVP_sha256()}, {"RS384", EVP_sha384()}, {"RS512", EVP_sha512()}, }; - static std::map rsaPssAlgorithms{ + static const std::map rsaPssAlgorithms{ {"PS1", EVP_sha1()}, {"PS256", EVP_sha256()}, {"PS384", EVP_sha384()}, {"PS512", EVP_sha512()}, }; - static std::map rsaOaepAlgorithms{ + static const std::map rsaOaepAlgorithms{ {"RSA-OAEP", EVP_sha1()}, {"RSA-OAEP-256", EVP_sha256()}, {"RSA-OAEP-384", EVP_sha384()}, @@ -996,7 +996,7 @@ kj::Own CryptoKey::Impl::importRsaRaw( KJ_IF_SOME(alg, keyDataJwk.alg) { // If this JWK specifies an algorithm, make sure it jives with the hash we were passed via // importKey(). - static std::map rsaAlgorithms{ + static const std::map rsaAlgorithms{ {"RS1", EVP_sha1()}, {"RS256", EVP_sha256()}, {"RS384", EVP_sha384()}, diff --git a/src/workerd/api/crypto/x509.c++ b/src/workerd/api/crypto/x509.c++ index 94c41d0891e..bb1bb1c8083 100644 --- a/src/workerd/api/crypto/x509.c++ +++ b/src/workerd/api/crypto/x509.c++ @@ -211,7 +211,7 @@ bool printGeneralName(BIO* out, const GENERAL_NAME* gen) { } else if (gen->type == GEN_RID) { // Unlike OpenSSL's default implementation, never print the OID as text and // instead always print its numeric representation. - char oline[256]; + char oline[256] = {0}; OBJ_obj2txt(oline, sizeof(oline), gen->d.rid, true); BIO_printf(out, "Registered ID:%s", oline); } else if (gen->type == GEN_OTHERNAME) { @@ -314,7 +314,7 @@ bool safeX509InfoAccessPrint(BIO* out, X509_EXTENSION* ext) { if (i != 0) BIO_write(out, "\n", 1); - char objtmp[80]; + char objtmp[80] = {0}; i2t_ASN1_OBJECT(objtmp, sizeof(objtmp), desc->method); BIO_printf(out, "%s - ", objtmp); if (!(ok = printGeneralName(out, desc->location))) { @@ -491,7 +491,7 @@ kj::Maybe getX509NameObject(jsg::Lock& js, X509* cert) { // If OpenSSL knows the type, use the short name of the type as the key, and // the numeric representation of the type's OID otherwise. int type_nid = OBJ_obj2nid(type); - char type_buf[80]; + char type_buf[80] = {0}; const char* type_str; if (type_nid != NID_undef) { type_str = OBJ_nid2sn(type_nid); @@ -540,7 +540,7 @@ struct StackOfXASN1Disposer: public kj::Disposer { sk_ASN1_OBJECT_pop_free(ptr, ASN1_OBJECT_free); } }; -StackOfXASN1Disposer stackOfXASN1Disposer; +constexpr StackOfXASN1Disposer stackOfXASN1Disposer; } // namespace kj::Maybe> X509Certificate::parse(kj::Array raw) { diff --git a/src/workerd/api/encoding.c++ b/src/workerd/api/encoding.c++ index d7b71cadfe0..a43af7460b5 100644 --- a/src/workerd/api/encoding.c++ +++ b/src/workerd/api/encoding.c++ @@ -281,9 +281,9 @@ Encoding getEncodingForLabel(kj::StringPtr label) { } } // namespace -kj::Array TextDecoder::EMPTY = +const kj::Array TextDecoder::EMPTY = kj::Array(&DUMMY, 0, kj::NullArrayDisposer::instance); -TextDecoder::DecodeOptions TextDecoder::DEFAULT_OPTIONS = TextDecoder::DecodeOptions(); +const TextDecoder::DecodeOptions TextDecoder::DEFAULT_OPTIONS = TextDecoder::DecodeOptions(); kj::Maybe IcuDecoder::create(Encoding encoding, bool fatal, bool ignoreBom) { UErrorCode status = U_ZERO_ERROR; diff --git a/src/workerd/api/encoding.h b/src/workerd/api/encoding.h index 031d9015ac0..797b63fdbd6 100644 --- a/src/workerd/api/encoding.h +++ b/src/workerd/api/encoding.h @@ -184,9 +184,9 @@ class TextDecoder final: public jsg::Object { DecoderImpl decoder; ConstructorOptions ctorOptions; - static DecodeOptions DEFAULT_OPTIONS; + static const DecodeOptions DEFAULT_OPTIONS; static constexpr kj::byte DUMMY = 0; - static kj::Array EMPTY; + static const kj::Array EMPTY; }; // Implements the TextEncoder interface as prescribed by: diff --git a/src/workerd/api/eventsource.c++ b/src/workerd/api/eventsource.c++ index 748140acabe..d3b2a139ad9 100644 --- a/src/workerd/api/eventsource.c++ +++ b/src/workerd/api/eventsource.c++ @@ -343,7 +343,7 @@ void EventSource::start(jsg::Lock& js) { auto fetcher = i.options.fetcher.map([](jsg::Ref& f) { return f.addRef(); }); - static auto handleError = [](auto& js, auto& self, kj::String message) { + static constexpr auto handleError = [](auto& js, auto& self, kj::String message) { auto ex = js.domException(kj::str("AbortError"), kj::mv(message)); auto handle = KJ_ASSERT_NONNULL(ex.tryGetHandle(js)); self->notifyError(js, jsg::JsValue(handle)); diff --git a/src/workerd/api/form-data.c++ b/src/workerd/api/form-data.c++ index fdd4fc298f3..47e2f2352ad 100644 --- a/src/workerd/api/form-data.c++ +++ b/src/workerd/api/form-data.c++ @@ -44,7 +44,7 @@ struct FormDataHeaderTable { }; const FormDataHeaderTable& getFormDataHeaderTable() { - static FormDataHeaderTable table({}); + static const FormDataHeaderTable table({}); return table; } diff --git a/src/workerd/api/node/crypto-keys.c++ b/src/workerd/api/node/crypto-keys.c++ index bb4df98643f..adc08e58989 100644 --- a/src/workerd/api/node/crypto-keys.c++ +++ b/src/workerd/api/node/crypto-keys.c++ @@ -103,7 +103,7 @@ CryptoKey::AsymmetricKeyDetails CryptoImpl::getAsymmetricKeyDetail( } kj::StringPtr CryptoImpl::getAsymmetricKeyType(jsg::Lock& js, jsg::Ref key) { - static std::map mapping{ + static const std::map mapping{ {"RSASSA-PKCS1-v1_5", "rsa"}, {"RSA-PSS", "rsa"}, {"RSA-OAEP", "rsa"}, diff --git a/src/workerd/api/sql.c++ b/src/workerd/api/sql.c++ index a744247044e..e7a51a20d0c 100644 --- a/src/workerd/api/sql.c++ +++ b/src/workerd/api/sql.c++ @@ -37,19 +37,19 @@ double SqlStorage::getDatabaseSize() { return pages * getPageSize(); } -bool SqlStorage::isAllowedName(kj::StringPtr name) { +bool SqlStorage::isAllowedName(kj::StringPtr name) const { return !name.startsWith("_cf_"); } -bool SqlStorage::isAllowedTrigger(kj::StringPtr name) { +bool SqlStorage::isAllowedTrigger(kj::StringPtr name) const { return true; } -void SqlStorage::onError(kj::StringPtr message) { +void SqlStorage::onError(kj::StringPtr message) const { JSG_ASSERT(false, Error, message); } -bool SqlStorage::allowTransactions() { +bool SqlStorage::allowTransactions() const { if (IoContext::hasCurrent()) { IoContext::current().logWarningOnce( "To execute a transaction, please use the state.storage.transaction() API instead of the " diff --git a/src/workerd/api/sql.h b/src/workerd/api/sql.h index 47079ce44f8..fc19ce2855e 100644 --- a/src/workerd/api/sql.h +++ b/src/workerd/api/sql.h @@ -54,10 +54,10 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator { visitor.visit(storage); } - bool isAllowedName(kj::StringPtr name) override; - bool isAllowedTrigger(kj::StringPtr name) override; - void onError(kj::StringPtr message) override; - bool allowTransactions() override; + bool isAllowedName(kj::StringPtr name) const override; + bool isAllowedTrigger(kj::StringPtr name) const override; + void onError(kj::StringPtr message) const override; + bool allowTransactions() const override; IoPtr sqlite; jsg::Ref storage; diff --git a/src/workerd/api/streams/queue-test.c++ b/src/workerd/api/streams/queue-test.c++ index 1dd7142d83b..56f1af65581 100644 --- a/src/workerd/api/streams/queue-test.c++ +++ b/src/workerd/api/streams/queue-test.c++ @@ -31,7 +31,7 @@ using ReadContinuation = jsg::Promise(ReadResult&&); using CloseContinuation = jsg::Promise(ReadResult&&); using ReadErrorContinuation = jsg::Promise(jsg::Value&&); -kj::UnwindDetector unwindDetector; +const kj::MutexGuarded unwindDetectorMutex; template struct MustCall; @@ -61,7 +61,8 @@ struct MustCall { location(location) {} ~MustCall() { - if (!unwindDetector.isUnwinding()) { + auto unwindDetector = unwindDetectorMutex.lockExclusive(); + if (!unwindDetector->isUnwinding()) { KJ_ASSERT(called == expected, kj::str("MustCall function was not called ", expected, " times. [actual: ", called , "]"), location); diff --git a/src/workerd/api/streams/readable.c++ b/src/workerd/api/streams/readable.c++ index 4caace74f11..c0a85e26686 100644 --- a/src/workerd/api/streams/readable.c++ +++ b/src/workerd/api/streams/readable.c++ @@ -380,7 +380,7 @@ ReadableStream::Reader ReadableStream::getReader( jsg::Ref ReadableStream::values( jsg::Lock& js, jsg::Optional options) { - static auto defaultOptions = ValuesOptions {}; + static const auto defaultOptions = ValuesOptions {}; return jsg::alloc(AsyncIteratorState { .ioContext = ioContext, .reader = ReadableStreamDefaultReader::constructor(js, JSG_THIS), diff --git a/src/workerd/api/url.c++ b/src/workerd/api/url.c++ index 80ca31da7d8..24bd0fd71d2 100644 --- a/src/workerd/api/url.c++ +++ b/src/workerd/api/url.c++ @@ -19,13 +19,13 @@ namespace { bool isSpecialScheme(kj::StringPtr scheme) { // TODO(cleanup): Move this to kj::Url. - static std::set specialSchemes{ + static const std::set specialSchemes{ "ftp", "file", "gopher", "http", "https", "ws", "wss"}; return specialSchemes.count(scheme); } kj::Maybe defaultPortForScheme(kj::StringPtr scheme) { - static std::map defaultPorts { + static const std::map defaultPorts { { "ftp", "21" }, { "gopher", "70" }, { "http", "80" }, diff --git a/src/workerd/io/actor-cache.c++ b/src/workerd/io/actor-cache.c++ index c6daa89bb1a..9f8507147c6 100644 --- a/src/workerd/io/actor-cache.c++ +++ b/src/workerd/io/actor-cache.c++ @@ -23,7 +23,7 @@ namespace workerd { // hit this limit, so this is just a sanity check. static constexpr size_t MAX_ACTOR_STORAGE_RPC_WORDS = (16u << 20) / sizeof(capnp::word); -ActorCache::Hooks ActorCache::Hooks::DEFAULT; +const ActorCache::Hooks ActorCache::Hooks::DEFAULT; namespace { diff --git a/src/workerd/io/actor-cache.h b/src/workerd/io/actor-cache.h index bad8aa45698..19b1f0e6f45 100644 --- a/src/workerd/io/actor-cache.h +++ b/src/workerd/io/actor-cache.h @@ -258,7 +258,7 @@ class ActorCache final: public ActorCacheInterface { virtual void storageReadCompleted(kj::Duration latency) {} virtual void storageWriteCompleted(kj::Duration latency) {} - static Hooks DEFAULT; + static const Hooks DEFAULT; }; static constexpr auto SHUTDOWN_ERROR_MESSAGE = @@ -266,7 +266,7 @@ class ActorCache final: public ActorCacheInterface { "Durable Object storage is no longer accessible."_kj; ActorCache(rpc::ActorStorage::Stage::Client storage, const SharedLru& lru, OutputGate& gate, - Hooks& hooks = Hooks::DEFAULT); + Hooks& hooks = const_cast(Hooks::DEFAULT)); ~ActorCache() noexcept(false); kj::Maybe getSqliteDatabase() override { return kj::none; } diff --git a/src/workerd/io/actor-sqlite.c++ b/src/workerd/io/actor-sqlite.c++ index ec2070f7df6..b6ab1265b40 100644 --- a/src/workerd/io/actor-sqlite.c++ +++ b/src/workerd/io/actor-sqlite.c++ @@ -356,7 +356,7 @@ kj::Maybe> ActorSqlite::onNoPendingFlush() { return outputGate.wait(); } -ActorSqlite::Hooks ActorSqlite::Hooks::DEFAULT = ActorSqlite::Hooks{}; +const ActorSqlite::Hooks ActorSqlite::Hooks::DEFAULT = ActorSqlite::Hooks{}; kj::Maybe> ActorSqlite::Hooks::armAlarmHandler(kj::Date scheduledTime, bool noCache) { JSG_FAIL_REQUIRE(Error, "alarms are not yet implemented for SQLite-backed Durable Objects"); diff --git a/src/workerd/io/actor-sqlite.h b/src/workerd/io/actor-sqlite.h index 94323d4829c..98bb253d68a 100644 --- a/src/workerd/io/actor-sqlite.h +++ b/src/workerd/io/actor-sqlite.h @@ -27,7 +27,7 @@ class ActorSqlite final: public ActorCacheInterface, private kj::TaskSet::ErrorH virtual kj::Maybe> armAlarmHandler(kj::Date scheduledTime, bool noCache); virtual void cancelDeferredAlarmDeletion(); - static Hooks DEFAULT; + static const Hooks DEFAULT; }; // Constructs ActorSqlite, arranging to honor the output gate, that is, any writes to the @@ -41,7 +41,7 @@ class ActorSqlite final: public ActorCacheInterface, private kj::TaskSet::ErrorH // machines before being considered durable. explicit ActorSqlite(kj::Own dbParam, OutputGate& outputGate, kj::Function()> commitCallback, - Hooks& hooks = Hooks::DEFAULT); + Hooks& hooks = const_cast(Hooks::DEFAULT)); bool isCommitScheduled() { return !currentTxn.is(); } diff --git a/src/workerd/io/compatibility-date.c++ b/src/workerd/io/compatibility-date.c++ index bccab83df38..06fbc565e83 100644 --- a/src/workerd/io/compatibility-date.c++ +++ b/src/workerd/io/compatibility-date.c++ @@ -278,7 +278,7 @@ kj::Array makeFieldTable( } // namespace kj::Array decompileCompatibilityFlagsForFl(CompatibilityFlags::Reader input) { - static auto fieldTable = makeFieldTable( + static const auto fieldTable = makeFieldTable( capnp::Schema::from().getFields()); kj::Vector enableFlags; @@ -331,7 +331,7 @@ kj::Maybe getPythonSnapshotRelease( uint latestFieldOrdinal = 0; kj::Maybe result; - static auto fieldTable = + static const auto fieldTable = makePythonSnapshotFieldTable(capnp::Schema::from().getFields()); for (auto field: fieldTable) { diff --git a/src/workerd/io/io-gate.c++ b/src/workerd/io/io-gate.c++ index 8ce0245671d..52b4cae8e99 100644 --- a/src/workerd/io/io-gate.c++ +++ b/src/workerd/io/io-gate.c++ @@ -7,7 +7,7 @@ namespace workerd { -InputGate::Hooks InputGate::Hooks::DEFAULT; +const InputGate::Hooks InputGate::Hooks::DEFAULT; InputGate::InputGate(Hooks& hooks): InputGate(hooks, kj::newPromiseAndFulfiller()) {} InputGate::InputGate(Hooks& hooks, kj::PromiseFulfillerPair paf) @@ -306,7 +306,7 @@ OutputGate::OutputGate(Hooks& hooks) : hooks(hooks), pastLocksPromise(kj::Promise(kj::READY_NOW).fork()) {} OutputGate::~OutputGate() noexcept(false) {} -OutputGate::Hooks OutputGate::Hooks::DEFAULT; +const OutputGate::Hooks OutputGate::Hooks::DEFAULT; kj::Own> OutputGate::lock() { auto paf = kj::newPromiseAndFulfiller(); diff --git a/src/workerd/io/io-gate.h b/src/workerd/io/io-gate.h index 1272a97ee7b..e3cb51195e8 100644 --- a/src/workerd/io/io-gate.h +++ b/src/workerd/io/io-gate.h @@ -51,10 +51,10 @@ class InputGate { virtual void inputGateWaiterAdded() {} virtual void inputGateWaiterRemoved() {} - static Hooks DEFAULT; + static const Hooks DEFAULT; }; - InputGate(Hooks& hooks = Hooks::DEFAULT); + InputGate(Hooks& hooks = const_cast(Hooks::DEFAULT)); ~InputGate() noexcept; class CriticalSection; @@ -235,10 +235,10 @@ class OutputGate { virtual void outputGateWaiterAdded() {} virtual void outputGateWaiterRemoved() {} - static Hooks DEFAULT; + static const Hooks DEFAULT; }; - OutputGate(Hooks& hooks = Hooks::DEFAULT); + OutputGate(Hooks& hooks = const_cast(Hooks::DEFAULT)); ~OutputGate() noexcept(false); // Block all future `wait()` calls until `promise` completes. Returns a wrapper around `promise`. diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index 79d43a91346..7c2a6f6821e 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -1744,7 +1744,7 @@ void Worker::handleLog(jsg::Lock& js, ConsoleMode consoleMode, LogLevel level, KJ_LOG(INFO, "console.log()", message()); } else { // Write to stdio if allowed by console mode - static ColorMode COLOR_MODE = permitsColor(); + static const ColorMode COLOR_MODE = permitsColor(); #if _WIN32 static bool STDOUT_TTY = _isatty(_fileno(stdout)); static bool STDERR_TTY = _isatty(_fileno(stderr)); diff --git a/src/workerd/jsg/dom-exception.c++ b/src/workerd/jsg/dom-exception.c++ index 230cd85c922..2ed3b789034 100644 --- a/src/workerd/jsg/dom-exception.c++ +++ b/src/workerd/jsg/dom-exception.c++ @@ -47,7 +47,7 @@ kj::StringPtr DOMException::getMessage() { } int DOMException::getCode() { - static std::map legacyCodes{ + static const std::map legacyCodes{ #define MAP_ENTRY(name, code, friendlyName) {friendlyName, code}, JSG_DOM_EXCEPTION_FOR_EACH_ERROR_NAME(MAP_ENTRY) #undef MAP_ENTRY diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 96c7ba0bf63..68a10ac3ecc 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -1963,7 +1963,7 @@ struct JsgConfig { bool unwrapCustomThenables = false; }; -static JsgConfig DEFAULT_JSG_CONFIG = {}; +static constexpr JsgConfig DEFAULT_JSG_CONFIG = {}; template static const JsgConfig& getConfig(const Config& config) { diff --git a/src/workerd/jsg/modules.c++ b/src/workerd/jsg/modules.c++ index 199859699dd..c0019b9a95c 100644 --- a/src/workerd/jsg/modules.c++ +++ b/src/workerd/jsg/modules.c++ @@ -62,7 +62,7 @@ public: } static const CompileCache& get() { - static CompileCache instance; + static const CompileCache instance; return instance; } diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 869efee6362..9852bae9028 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -1079,7 +1079,7 @@ struct JsSetup { static void callback(v8::Local property, const v8::PropertyCallbackInfo& info) { liftKj(info, [&]() { - static auto path = kj::Path::parse(moduleName); + static const auto path = kj::Path::parse(moduleName); auto& js = Lock::from(info.GetIsolate()); auto context = js.v8Context(); diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index f8b73b32961..99df5bbc2d5 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -124,7 +124,7 @@ static kj::Vector escapeJsonString(kj::StringPtr text) { // TODO(now): Temporary class ServerResolveObserver final : public jsg::ResolveObserver {}; -ServerResolveObserver serverResolveObserver; +const ServerResolveObserver serverResolveObserver; } // namespace diff --git a/src/workerd/server/workerd-api.c++ b/src/workerd/server/workerd-api.c++ index c46ab8309f7..65a326b07a4 100644 --- a/src/workerd/server/workerd-api.c++ +++ b/src/workerd/server/workerd-api.c++ @@ -120,7 +120,7 @@ JSG_DECLARE_ISOLATE_TYPE(JsgWorkerdIsolate, jsg::NodeJsModuleContext); -static PythonConfig defaultConfig { +static const PythonConfig defaultConfig { .packageDiskCacheRoot = kj::none, .pyodideDiskCacheRoot = kj::none, .createSnapshot = false, @@ -133,7 +133,7 @@ struct WorkerdApi::Impl final { kj::Maybe> maybeOwnedModuleRegistry; JsgWorkerdIsolate jsgIsolate; api::MemoryCacheProvider& memoryCacheProvider; - PythonConfig& pythonConfig; + const PythonConfig& pythonConfig; class Configuration { public: @@ -156,7 +156,7 @@ struct WorkerdApi::Impl final { IsolateLimitEnforcer& limitEnforcer, kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, - PythonConfig& pythonConfig = defaultConfig, + const PythonConfig& pythonConfig = defaultConfig, kj::Maybe> newModuleRegistry = kj::none) : features(capnp::clone(featuresParam)), maybeOwnedModuleRegistry(kj::mv(newModuleRegistry)), @@ -209,7 +209,7 @@ WorkerdApi::WorkerdApi(jsg::V8System& v8System, IsolateLimitEnforcer& limitEnforcer, kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, - PythonConfig &pythonConfig, + const PythonConfig& pythonConfig, kj::Maybe> newModuleRegistry) : impl(kj::heap(v8System, features, limitEnforcer, kj::mv(observer), memoryCacheProvider, pythonConfig, diff --git a/src/workerd/server/workerd-api.h b/src/workerd/server/workerd-api.h index d76ea2e688f..a236eed0e35 100644 --- a/src/workerd/server/workerd-api.h +++ b/src/workerd/server/workerd-api.h @@ -26,7 +26,7 @@ class WorkerdApi final: public Worker::Api { IsolateLimitEnforcer& limitEnforcer, kj::Own observer, api::MemoryCacheProvider& memoryCacheProvider, - PythonConfig& pythonConfig, + const PythonConfig& pythonConfig, kj::Maybe> newModuleRegistry); ~WorkerdApi() noexcept(false); diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index 7b08eb735d2..d726db7143e 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -264,7 +264,7 @@ class MockActorLoopback : public Worker::Actor::Loopback, public kj::Refcounted using api::pyodide::PythonConfig; -PythonConfig defaultPythonConfig { .packageDiskCacheRoot = kj::none, .pyodideDiskCacheRoot = kj::none, .createSnapshot = false, .createBaselineSnapshot = false }; +const PythonConfig defaultPythonConfig { .packageDiskCacheRoot = kj::none, .pyodideDiskCacheRoot = kj::none, .createSnapshot = false, .createBaselineSnapshot = false }; TestFixture::TestFixture(SetupParams&& params) : waitScope(params.waitScope), diff --git a/src/workerd/util/mimetype-test.c++ b/src/workerd/util/mimetype-test.c++ index c4de30473b1..1929e5e72bb 100644 --- a/src/workerd/util/mimetype-test.c++ +++ b/src/workerd/util/mimetype-test.c++ @@ -15,7 +15,7 @@ KJ_TEST("Basic MimeType parsing works") { kj::StringPtr output; kj::Maybe> params; }; - static TestCase kTests[] = { + static const TestCase kTests[] = { { .input = "text/plain"_kj, .type = "text"_kj, @@ -88,7 +88,7 @@ KJ_TEST("Basic MimeType parsing works") { struct ErrorTestCase { kj::StringPtr input; }; - static ErrorTestCase kErrorTests[] = { + static const ErrorTestCase kErrorTests[] = { { "" }, { "text" }, { "text/" }, diff --git a/src/workerd/util/sqlite-test.c++ b/src/workerd/util/sqlite-test.c++ index b607c8bfdfe..e36dae5ca85 100644 --- a/src/workerd/util/sqlite-test.c++ +++ b/src/workerd/util/sqlite-test.c++ @@ -395,7 +395,7 @@ KJ_TEST("SQLite Regulator") { public: RegulatorImpl(kj::StringPtr blocked): blocked(blocked) {} - bool isAllowedName(kj::StringPtr name) override { + bool isAllowedName(kj::StringPtr name) const override { if (alwaysFail) return false; return name != blocked; } @@ -471,7 +471,7 @@ struct RowCounts { }; template -RowCounts countRowsTouched(SqliteDatabase& db, SqliteDatabase::Regulator& regulator, kj::StringPtr sqlCode, Params... bindParams) { +RowCounts countRowsTouched(SqliteDatabase& db, const SqliteDatabase::Regulator& regulator, kj::StringPtr sqlCode, Params... bindParams) { uint64_t rowsFound = 0; // Runs a query; retrieves and discards all the data. @@ -668,7 +668,7 @@ KJ_TEST("SQLite row counters with triggers") { public: RegulatorImpl() = default; - bool isAllowedTrigger(kj::StringPtr name) override { + bool isAllowedTrigger(kj::StringPtr name) const override { // SqliteDatabase::TRUSTED doesn't let us use triggers at all. return true; } diff --git a/src/workerd/util/sqlite.c++ b/src/workerd/util/sqlite.c++ index 66e6472183a..1e71aa99947 100644 --- a/src/workerd/util/sqlite.c++ +++ b/src/workerd/util/sqlite.c++ @@ -365,7 +365,7 @@ static constexpr PragmaInfo ALLOWED_PRAGMAS[] = { // ======================================================================================= -SqliteDatabase::Regulator SqliteDatabase::TRUSTED; +constexpr SqliteDatabase::Regulator SqliteDatabase::TRUSTED; SqliteDatabase::SqliteDatabase(const Vfs& vfs, kj::PathPtr path, kj::Maybe maybeMode) { KJ_IF_SOME(mode, maybeMode) { @@ -435,7 +435,7 @@ kj::StringPtr SqliteDatabase::getCurrentQueryForDebug() { // Set up the regulator that will be used for authorizer callbacks while preparing this // statement. kj::Own SqliteDatabase::prepareSql( - Regulator& regulator, kj::StringPtr sqlCode, uint prepFlags, Multi multi) { + const Regulator& regulator, kj::StringPtr sqlCode, uint prepFlags, Multi multi) { KJ_ASSERT(currentRegulator == kj::none, "recursive prepareSql()?"); KJ_DEFER(currentRegulator = kj::none); currentRegulator = regulator; @@ -496,7 +496,8 @@ kj::Own SqliteDatabase::prepareSql( } } -SqliteDatabase::IngestResult SqliteDatabase::ingestSql(Regulator& regulator, kj::StringPtr sqlCode) { +SqliteDatabase::IngestResult SqliteDatabase::ingestSql( + const Regulator& regulator, kj::StringPtr sqlCode) { uint64_t rowsRead = 0; uint64_t rowsWritten = 0; uint64_t statementCount = 0; @@ -522,7 +523,8 @@ SqliteDatabase::IngestResult SqliteDatabase::ingestSql(Regulator& regulator, kj: return {.remainder = sqlCode, .rowsRead = rowsRead, .rowsWritten = rowsWritten, .statementCount = statementCount}; } -void SqliteDatabase::executeWithRegulator(Regulator& regulator, kj::FunctionParam func) { +void SqliteDatabase::executeWithRegulator( + const Regulator& regulator, kj::FunctionParam func) { // currentRegulator would only be set if we're running this method while running something else // with a regulator. I'm not sure what the ramifications are, so for now, we'll just assume that // we can only call executeWithRegulator when no regulator is currently set. @@ -536,7 +538,7 @@ void SqliteDatabase::executeWithRegulator(Regulator& regulator, kj::FunctionPara bool SqliteDatabase::isAuthorized(int actionCode, kj::Maybe param1, kj::Maybe param2, kj::Maybe dbName, kj::Maybe triggerName) { - Regulator& regulator = KJ_UNWRAP_OR(currentRegulator, { + const Regulator& regulator = KJ_UNWRAP_OR(currentRegulator, { // We're not currently preparing a statement, so we didn't expect the authorizer callback to // run. We blanket-deny in this case as a precaution. KJ_LOG(ERROR, "SQLite authorizer callback invoked at unexpected time", kj::getStackTrace()); @@ -786,7 +788,7 @@ bool SqliteDatabase::isAuthorized(int actionCode, // Temp databases have very restricted operations bool SqliteDatabase::isAuthorizedTemp(int actionCode, const kj::Maybe ¶m1, const kj::Maybe ¶m2, - Regulator ®ulator) { + const Regulator ®ulator) { switch (actionCode) { case SQLITE_READ : /* Table Name Column Name */ @@ -868,12 +870,13 @@ void SqliteDatabase::setupSecurity() { // (handled in BUILD.sqlite3) } -SqliteDatabase::Statement SqliteDatabase::prepare(Regulator& regulator, kj::StringPtr sqlCode) { +SqliteDatabase::Statement SqliteDatabase::prepare( + const Regulator& regulator, kj::StringPtr sqlCode) { return Statement(*this, regulator, prepareSql(regulator, sqlCode, SQLITE_PREPARE_PERSISTENT, SINGLE)); } -SqliteDatabase::Query::Query(SqliteDatabase& db, Regulator& regulator, Statement& statement, +SqliteDatabase::Query::Query(SqliteDatabase& db, const Regulator& regulator, Statement& statement, kj::ArrayPtr bindings) : db(db), regulator(regulator), statement(statement) { // If the statement was used for a previous query, then its row counters contain data from that @@ -882,7 +885,7 @@ SqliteDatabase::Query::Query(SqliteDatabase& db, Regulator& regulator, Statement init(bindings); } -SqliteDatabase::Query::Query(SqliteDatabase& db, Regulator& regulator, kj::StringPtr sqlCode, +SqliteDatabase::Query::Query(SqliteDatabase& db, const Regulator& regulator, kj::StringPtr sqlCode, kj::ArrayPtr bindings) : db(db), regulator(regulator), ownStatement(db.prepareSql(regulator, sqlCode, 0, MULTI)), diff --git a/src/workerd/util/sqlite.h b/src/workerd/util/sqlite.h index 30bb687f5e9..3161a130dca 100644 --- a/src/workerd/util/sqlite.h +++ b/src/workerd/util/sqlite.h @@ -33,7 +33,6 @@ class SqliteDatabase { class Statement; class Lock; class LockManager; - class Regulator; struct VfsOptions; struct IngestResult { @@ -51,13 +50,57 @@ class SqliteDatabase { // expected. operator sqlite3*() { return db; } +// Class which regulates a SQL query, especially to control how queries created in JavaScript +// application code are handled. +class Regulator { + +public: + // Returns whether the given name (which may be a table, index, view, etc.) is allowed to be + // accessed. Typically, this is used to deny access to names containing special prefixes + // indicating that they are privileged, like `_cf_`. + // + // This only applies to global names. Scoped names, such as column names, are not subject to + // authorization. + virtual bool isAllowedName(kj::StringPtr name) const { return true; } + + // Returns whether a given trigger or view name should be permitted to run as a side effect of a + // query running under this Regulator. This is a precaution to prevent application-defined + // triggers from executing under a privileged regulator. + // + // TODO(someday): In theory a trigger should run with the authority level under which it was + // created, but how do we track that? In practice we probably never expect triggers to run on + // trusted queries. + virtual bool isAllowedTrigger(kj::StringPtr name) const { return false; } + + // Report that an error occurred. `message` is the detail message constructed by SQLite. This + // function should typically throw an exception. If no exception is thrown, a simple KJ exception + // will be thrown after `onError()` returns. + // + // The purpose of this callback is to allow the JavaScript API bindings to throw a JSG exception. + // + // Note that SQLITE_MISUSE errors are NOT reported using `onError()` -- they will throw regular + // KJ exceptions in all cases. This is because SQLITE_MISUSE indicates a bug that could lead to + // undefined behavior. Such bugs are always in C++ code; JavaScript application code must be + // prohibited from causing such errors in the first place. + virtual void onError(kj::StringPtr message) const {} + + // Are BEGIN TRANSACTION and SAVEPOINT statements allowed? Note that if allowed, SAVEPOINT will + // also be subject to `isAllowedName()` for the savepoint name. If denied, the application will + // not be able to create any sort of transaction. + // + // In Durable Objects, we disallow these statements because the platform provides an explicit + // API for transactions that is safer (e.g. it automatically rolls back on throw). Also, the + // platform automatically wraps every entry into the isolate lock in a transaction. + virtual bool allowTransactions() const { return true; } +}; + // Use as the `Regulator&` for queries that are fully trusted. As a general rule, this should // be used if and only if the SQL query is a string literal. - static Regulator TRUSTED; + static constexpr Regulator TRUSTED; // Prepares the given SQL code as a persistent statement that can be used across several queries. // Don't use this for one-off queries; pass the code to the Query constructor. - Statement prepare(Regulator& regulator, kj::StringPtr sqlCode); + Statement prepare(const Regulator& regulator, kj::StringPtr sqlCode); // Convenience method to start a query. This is equivalent to `prepare(sqlCode).run(bindings...)` // except: @@ -66,7 +109,7 @@ class SqliteDatabase { // `Query` object are both associated with the last statement. This is particularly convenient // for doing database initialization such as creating several tables at once. template - Query run(Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings); + Query run(const Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings); template Statement prepare(const char (&sqlCode)[size]); @@ -99,16 +142,16 @@ class SqliteDatabase { // Helper to execute a chunk of SQL that may not be complete. // Executes every valid statement provided, and returns the remaining portion of the input // that was not processed. This is used for streaming SQL ingestion. - IngestResult ingestSql(Regulator& regulator, kj::StringPtr sqlCode); + IngestResult ingestSql(const Regulator& regulator, kj::StringPtr sqlCode); // Execute a function with the given regulator. - void executeWithRegulator(Regulator& regulator, kj::FunctionParam func); + void executeWithRegulator(const Regulator& regulator, kj::FunctionParam func); private: sqlite3* db; // Set while a query is compiling. - kj::Maybe currentRegulator; + kj::Maybe currentRegulator; // Set while a statement is executing. kj::Maybe currentStatement; @@ -126,7 +169,7 @@ class SqliteDatabase { // In MULTI mode, if `sqlCode` contains multiple statements, each statement before the last one // is executed immediately. The returned object represents the last statement. kj::Own prepareSql( - Regulator& regulator, kj::StringPtr sqlCode, uint prepFlags, Multi multi); + const Regulator& regulator, kj::StringPtr sqlCode, uint prepFlags, Multi multi); // Implements SQLite authorizer callback, see sqlite3_set_authorizer(). bool isAuthorized(int actionCode, @@ -136,55 +179,11 @@ class SqliteDatabase { // Implements SQLite authorizer for 'temp' DB bool isAuthorizedTemp(int actionCode, const kj::Maybe ¶m1, const kj::Maybe ¶m2, - Regulator ®ulator); + const Regulator ®ulator); void setupSecurity(); }; -// Class which regulates a SQL query, especially to control how queries created in JavaScript -// application code are handled. -class SqliteDatabase::Regulator { - -public: - // Returns whether the given name (which may be a table, index, view, etc.) is allowed to be - // accessed. Typically, this is used to deny access to names containing special prefixes - // indicating that they are privileged, like `_cf_`. - // - // This only applies to global names. Scoped names, such as column names, are not subject to - // authorization. - virtual bool isAllowedName(kj::StringPtr name) { return true; } - - // Returns whether a given trigger or view name should be permitted to run as a side effect of a - // query running under this Regulator. This is a precaution to prevent application-defined - // triggers from executing under a privileged regulator. - // - // TODO(someday): In theory a trigger should run with the authority level under which it was - // created, but how do we track that? In practice we probably never expect triggers to run on - // trusted queries. - virtual bool isAllowedTrigger(kj::StringPtr name) { return false; } - - // Report that an error occurred. `message` is the detail message constructed by SQLite. This - // function should typically throw an exception. If no exception is thrown, a simple KJ exception - // will be thrown after `onError()` returns. - // - // The purpose of this callback is to allow the JavaScript API bindings to throw a JSG exception. - // - // Note that SQLITE_MISUSE errors are NOT reported using `onError()` -- they will throw regular - // KJ exceptions in all cases. This is because SQLITE_MISUSE indicates a bug that could lead to - // undefined behavior. Such bugs are always in C++ code; JavaScript application code must be - // prohibited from causing such errors in the first place. - virtual void onError(kj::StringPtr message) {} - - // Are BEGIN TRANSACTION and SAVEPOINT statements allowed? Note that if allowed, SAVEPOINT will - // also be subject to `isAllowedName()` for the savepoint name. If denied, the application will - // not be able to create any sort of transaction. - // - // In Durable Objects, we disallow these statements because the platform provides an explicit - // API for transactions that is safer (e.g. it automatically rolls back on throw). Also, the - // platform automatically wraps every entry into the isolate lock in a transaction. - virtual bool allowTransactions() { return true; } -}; - // Represents a prepared SQL statement, which can be executed many times. class SqliteDatabase::Statement { public: @@ -206,10 +205,10 @@ class SqliteDatabase::Statement { private: SqliteDatabase& db; - Regulator& regulator; + const Regulator& regulator; kj::Own stmt; - Statement(SqliteDatabase& db, Regulator& regulator, kj::Own stmt) + Statement(SqliteDatabase& db, const Regulator& regulator, kj::Own stmt) : db(db), regulator(regulator), stmt(kj::mv(stmt)) {} friend class SqliteDatabase; @@ -292,24 +291,24 @@ class SqliteDatabase::Query { private: SqliteDatabase& db; - Regulator& regulator; + const Regulator& regulator; kj::Own ownStatement; // for one-off queries sqlite3_stmt* statement; bool done = false; friend class SqliteDatabase; - Query(SqliteDatabase& db, Regulator& regulator, Statement& statement, + Query(SqliteDatabase& db, const Regulator& regulator, Statement& statement, kj::ArrayPtr bindings); - Query(SqliteDatabase& db, Regulator& regulator, kj::StringPtr sqlCode, + Query(SqliteDatabase& db, const Regulator& regulator, kj::StringPtr sqlCode, kj::ArrayPtr bindings); template - Query(SqliteDatabase& db, Regulator& regulator, Statement& statement, Params&&... bindings) + Query(SqliteDatabase& db, const Regulator& regulator, Statement& statement, Params&&... bindings) : db(db), regulator(regulator), statement(statement) { bindAll(std::index_sequence_for(), kj::fwd(bindings)...); } template - Query(SqliteDatabase& db, Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings) + Query(SqliteDatabase& db, const Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings) : db(db), regulator(regulator), ownStatement(db.prepareSql(regulator, sqlCode, 0, MULTI)), statement(ownStatement) { @@ -584,7 +583,7 @@ class SqliteDatabase::Lock { template SqliteDatabase::Query SqliteDatabase::run( - Regulator& regulator, kj::StringPtr sqlCode, Params&&... params) { + const Regulator& regulator, kj::StringPtr sqlCode, Params&&... params) { return Query(*this, regulator, sqlCode, kj::fwd(params)...); }