From 58be75bc938206df864b102809ec74b66ff141b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Kr=C3=BCmpelmann?= Date: Thu, 22 Oct 2026 18:59:09 +0200 Subject: [PATCH] fix(codeql): resolve the three open code-scanning alerts - cpp/declaration-hides-parameter: the lambda pack in SharedMemoryRepository::construct() has no homonymous parameter; renaming only changed the reported name. Unpack the tuple via constructFromTuple() and std::index_sequence_for instead of std::apply with a variadic lambda. - cpp/path-injection: export_repository writes to the operator's own --file argument in their own process, so no privilege boundary exists. Keep the sink with a justified // codeql[cpp/path-injection] suppression and report a failed open instead of ignoring it. - cpp/poorly-documented-function: document parseDatabaseEntry() and its validation sections. Docs table and the false-positive rationale are updated as well. --- docs/code-scanning.md | 20 +++++++++++ .../DatabaseConnectionComponent.cpp | 33 +++++++++++++++++++ .../components/SharedMemoryCliComponent.cpp | 8 +++++ .../lintel/core/shm/SharedMemoryRepository.h | 21 ++++++++++-- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/docs/code-scanning.md b/docs/code-scanning.md index 20928832..6d6d0ecf 100644 --- a/docs/code-scanning.md +++ b/docs/code-scanning.md @@ -162,6 +162,26 @@ codebase. | First `manual` run (broken build, 0 results) | 300 | stale: 285 `unused-static-function` (Catch2), 90 `include-non-header` (unity amalgamations) | | After query-filters + unity off | ~10 real findings | 4 `large-parameter`, 3 `loop-variable-changed`, 1 `long-switch`, 1 `array-in-interface`, 1 `cleartext-storage-database` (FP) | | After A2 (`lintel::di`) + parameterized test | 0 | only suppressed findings remain | +| After the SHM registry / no-lint cleanup runs | 3 | 1 `path-injection` (suppressed at the sink), 1 `declaration-hides-parameter` (false positive, lambda pack replaced by an index sequence), 1 `poorly-documented-function` (documented) | + +## `declaration-hides-parameter` on the SHM payload constructor + +`SharedMemoryRepository::construct()` expands the stored constructor +arguments with `std::apply`. CodeQL reported the lambda's parameter pack as +hiding a parameter of the same name even though no such parameter exists +anywhere in the header; renaming the pack only moved the reported name +(`constructorArgs` → `ctorArgs`, still open). The lambda was therefore replaced +by `constructFromTuple()` plus `std::index_sequence_for`, which +unpacks the tuple without introducing a nested parameter pack. + +## `path-injection` on the CLI export target + +`export_repository --file ` writes to a path the operator passed on the +command line of their own process. Like `examples/named_pipe_server.cpp`, the +sink stays but carries a `// codeql[cpp/path-injection]` suppression with the +reason: there is no privilege boundary between the caller and the file, so no +sanitizer the rule would accept can exist. The code additionally reports a +failed open instead of silently writing nothing. The ~10 remaining notes are real and were fixed in code rather than dismissed: the `large-parameter` call sites pass by `const &`, the `loop-variable-changed` diff --git a/src/core/configuration/DatabaseConnectionComponent.cpp b/src/core/configuration/DatabaseConnectionComponent.cpp index cae2cee4..5c441e63 100644 --- a/src/core/configuration/DatabaseConnectionComponent.cpp +++ b/src/core/configuration/DatabaseConnectionComponent.cpp @@ -19,12 +19,33 @@ DatabaseConnectionComponent::DatabaseConnectionComponent( : Component(shape.CONFIG_ROOT), m_environmentConfiguration(std::move(environmentConfiguration)) {} +/** + * Parse one ``/`` element into a DatabaseConnectionEntry. + * + * Required attributes are validated in the order the entry needs them, so the + * first error reported refers to the first missing or invalid value. SQLite + * connections additionally resolve a leading `~` against the configured home + * directory and reject `poolSize > 1` unless multiple connections are allowed + * explicitly. + * + * @param propertyElement XML element carrying the connection attributes + * @param lineOffset offset added to the XML line number for diagnostics + * @param configRoot configuration root used in exception messages + * @param envConfig environment configuration resolving the home directory + * @param foundConnectionWithDefault set to true when this entry is the default; + * a second default entry is rejected + * @return the parsed entry + * @throws ConfigurationException when a required attribute is missing, a value + * is invalid, or a second default connection is declared + */ std::shared_ptr DatabaseConnectionComponent::parseDatabaseEntry(tinyxml2::XMLElement *propertyElement, int32_t lineOffset, const std::string &configRoot, const std::shared_ptr &envConfig, bool &foundConnectionWithDefault) { + // Attributes are optional at this point; each one is validated at the place + // where it is consumed below. const char *name = propertyElement->Attribute(shape.CONNECTION_NAME); const char *connection = propertyElement->Attribute(shape.CONNECTION_CONNECTION); const char *password = propertyElement->Attribute(shape.CONNECTION_PASSWORD); @@ -39,9 +60,12 @@ std::shared_ptr DatabaseConnectionComponent::parseDatabaseEntry(tinyxml2: int32_t lineNumber = propertyElement->GetLineNum() + lineOffset - 1; + // The type decides how the remaining attributes are interpreted, so it must + // be present before anything else is checked. if (typeName == nullptr) { throw ConfigurationException(configRoot, "type string is null", lineNumber); } std::string connectionString = connection != nullptr ? connection : ""; + // At most one connection may be marked as default. bool isDefault = false; if (defaultName != nullptr) { isDefault = std::strcmp(defaultName, "true") == 0 || std::strcmp(defaultName, "t") == 0; @@ -57,6 +81,8 @@ std::shared_ptr DatabaseConnectionComponent::parseDatabaseEntry(tinyxml2: if (type == db::ConnectionType::UNDEFINED) { throw ConfigurationException(configRoot, "type " + std::string(typeName) + " is not valid", lineNumber); } + // SQLite stores the database as a file, so a leading `~` has to be expanded + // relative to the environment's home directory instead of the process CWD. if (type == db::ConnectionType::SQLite) { std::string tmpPath = connection; if (!tmpPath.empty() && tmpPath[0] == '~') { @@ -70,14 +96,19 @@ std::shared_ptr DatabaseConnectionComponent::parseDatabaseEntry(tinyxml2: if (name == nullptr) { throw ConfigurationException(configRoot, "name string is null", lineNumber); } + // Numeric attributes are parsed only when present; -1 means "not set" and is + // passed through to the entry as such. if (portTemp != nullptr) { port = std::stoi(portTemp); } + // Missing connection details default to the empty string instead of failing: + // SQLite needs none of them, and the server backends validate on connect. if (password == nullptr) { password = ""; } if (userName == nullptr) { userName = ""; } if (databaseName == nullptr) { databaseName = ""; } + // Pool settings are optional; a present value must not be negative. int32_t poolSize = -1; if (poolSizeTemp != nullptr) { poolSize = std::stoi(poolSizeTemp); @@ -96,6 +127,8 @@ std::shared_ptr DatabaseConnectionComponent::parseDatabaseEntry(tinyxml2: } } + // SQLite serializes writers through a single file lock; a larger pool only + // helps when the operator opted in explicitly. if (type == db::ConnectionType::SQLite) { const int32_t effectivePoolSize = poolSize >= 0 ? poolSize : DatabaseConnectionEntry::kDefaultSqlitePoolSize; const bool allowMultipleConnections = diff --git a/src/features/cli/components/SharedMemoryCliComponent.cpp b/src/features/cli/components/SharedMemoryCliComponent.cpp index 72ace286..06b2eec3 100644 --- a/src/features/cli/components/SharedMemoryCliComponent.cpp +++ b/src/features/cli/components/SharedMemoryCliComponent.cpp @@ -6,6 +6,7 @@ #include #include #include +#include namespace lintel { SharedMemoryCliComponent::SharedMemoryCliComponent(std::shared_ptr sharedMemoryApi) @@ -144,7 +145,14 @@ void SharedMemoryCliComponent::handleExportRepository() std::filesystem::path path(file); bool parentPathExits = std::filesystem::exists(path.parent_path()); if (!parentPathExits) { std::filesystem::create_directories(path.parent_path()); } + // codeql[cpp/path-injection] The export target is the operator's own + // --file argument processed in the operator's own process, so it grants no + // access that the caller does not already have. Every other component + // (segment, repository, UUID) is resolved by the service; this output path + // is the single deliberate user choice. There is no privileged boundary to + // enforce here, hence no sanitizer the rule could accept. std::ofstream out = std::ofstream(path, std::ofstream::trunc | std::ofstream::out); + if (!out.is_open()) { throw std::runtime_error("cannot open export file " + path.string()); } out << result; std::cout << "Exported SharedMemory to >" << path.filename() << "<\n"; } else { diff --git a/src/include/lintel/core/shm/SharedMemoryRepository.h b/src/include/lintel/core/shm/SharedMemoryRepository.h index d47e6a71..470eb7d5 100644 --- a/src/include/lintel/core/shm/SharedMemoryRepository.h +++ b/src/include/lintel/core/shm/SharedMemoryRepository.h @@ -267,14 +267,31 @@ namespace shm { [[nodiscard]] const std::shared_ptr &getSegment() const { return m_cell.getSegment(); } private: + /** Placement-construct the payload from the stored constructor arguments. + * + * Unpacking through an index sequence instead of a variadic lambda keeps + * the argument names out of the scope of construct(): CodeQL reports the + * lambda's parameter pack as hiding the repository's constructor + * parameter pack, regardless of how it is named. + * @tparam Arguments decayed tuple type of the constructor arguments + * @param payload raw payload memory of the cell + * @param arguments stored constructor arguments + * @param indices index sequence selecting the tuple elements */ + template + static void constructFromTuple(void *payload, const Arguments &arguments, std::index_sequence) + { + ::new (payload) T(std::get(arguments)...); + } + /** Payload constructor handed to the cell as a plain function pointer. * @param payload raw payload memory of the cell * @param initArgument the tuple of constructor arguments this repository * built at construction time */ template static void construct(void *payload, ShmCell::InitArgument initArgument) { - const auto &arguments = *static_cast...> *>(initArgument.value); - std::apply([payload](const auto &...ctorArgs) { ::new (payload) T(ctorArgs...); }, arguments); + using Arguments = std::tuple...>; + const auto &arguments = *static_cast(initArgument.value); + constructFromTuple(payload, arguments, std::index_sequence_for{}); } ShmCell m_cell;