From 2d816e8c2bca3e47e1dda67364594c73884bc277 Mon Sep 17 00:00:00 2001 From: Krzysztof Jackiewicz Date: Tue, 20 Jun 2023 17:52:03 +0200 Subject: [PATCH 1/1] Enable -Wshadow and fix warnings The flag is used in products an our code causes warnings/errors. Fix existing code and enable the flag to detect possible errors in future. Change-Id: I830696231f9a6f1b80d390f7bf3df4fff1814691 --- CMakeLists.txt | 4 +- misc/ckm_db_tool/ckm_db_merge.cpp | 4 +- misc/encryption_scheme/scheme-test.h | 14 ++-- src/include/ckm/ckm-type.h | 8 +-- src/manager/client-async/async-request.cpp | 8 +-- .../crypto/generic-backend/encryption-params.h | 6 +- src/manager/crypto/sw-backend/kbkdf.cpp | 4 +- src/manager/main/credentials.h | 4 +- src/manager/main/generic-socket-manager.h | 12 ++-- src/manager/main/service-messages.h | 20 +++--- src/manager/service/ckm-logic.cpp | 76 +++++++++++----------- src/manager/service/db-crypto.cpp | 25 ++++--- src/manager/service/db-crypto.h | 4 +- unit-tests/test_descriptor-set.cpp | 4 +- unit-tests/test_kbkdf.cpp | 18 +++-- unit-tests/test_log-provider.cpp | 2 +- 16 files changed, 110 insertions(+), 103 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1113a56..6af5d8c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -31,9 +31,9 @@ INCLUDE(FindPkgConfig) SET(CMAKE_C_FLAGS_PROFILING "-g -O0 -pg -Wp,-U_FORTIFY_SOURCE") SET(CMAKE_CXX_FLAGS_PROFILING "-g -std=c++17 -O0 -pg -Wp,-U_FORTIFY_SOURCE") SET(CMAKE_C_FLAGS_DEBUG "-g -O0 -ggdb -Wp,-U_FORTIFY_SOURCE") -SET(CMAKE_CXX_FLAGS_DEBUG "-g -std=c++17 -O0 -ggdb -Wp,-U_FORTIFY_SOURCE") +SET(CMAKE_CXX_FLAGS_DEBUG "-g -std=c++17 -O0 -ggdb -Wp,-U_FORTIFY_SOURCE -Wshadow") SET(CMAKE_C_FLAGS_RELEASE "-g -O2") -SET(CMAKE_CXX_FLAGS_RELEASE "-g -std=c++17 -O2") +SET(CMAKE_CXX_FLAGS_RELEASE "-g -std=c++17 -O2 -Wshadow") SET(CMAKE_C_FLAGS_COVERAGE "-g -O0 -ggdb --coverage -Wp,-U_FORTIFY_SOURCE") SET(CMAKE_CXX_FLAGS_COVERAGE "-g -std=c++17 -O0 -ggdb --coverage -Wp,-U_FORTIFY_SOURCE") diff --git a/misc/ckm_db_tool/ckm_db_merge.cpp b/misc/ckm_db_tool/ckm_db_merge.cpp index 44e4226..6e671ce 100644 --- a/misc/ckm_db_tool/ckm_db_merge.cpp +++ b/misc/ckm_db_tool/ckm_db_merge.cpp @@ -106,10 +106,10 @@ int mergeDatabase( if (migrate) { try { target.saveRow(e); - } catch (const CKM::DB::SqlConnection::Exception::Base &e) { + } catch (const CKM::DB::SqlConnection::Exception::Base &exc) { UI::error() << "sql exception, migration failed or object already exists in the database: " - << e.DumpToString() << endl; + << exc.DumpToString() << endl; } } } diff --git a/misc/encryption_scheme/scheme-test.h b/misc/encryption_scheme/scheme-test.h index 1ec178e..7679de4 100644 --- a/misc/encryption_scheme/scheme-test.h +++ b/misc/encryption_scheme/scheme-test.h @@ -41,10 +41,10 @@ struct Item { { } - Item(const CKM::Alias &alias, - const CKM::DataType::Type type, - const CKM::Policy &policy) - : alias(alias), type(type), policy(policy) + Item(const CKM::Alias &_alias, + const CKM::DataType::Type _type, + const CKM::Policy &_policy) + : alias(_alias), type(_type), policy(_policy) { } @@ -72,9 +72,9 @@ struct ItemFilter { { } - ItemFilter(CKM::DataType::Type typeFrom, CKM::DataType::Type typeTo) : - typeFrom(typeFrom), - typeTo(typeTo), + ItemFilter(CKM::DataType::Type _typeFrom, CKM::DataType::Type _typeTo) : + typeFrom(_typeFrom), + typeTo(_typeTo), exportableOnly(false), noPassword(false) { diff --git a/src/include/ckm/ckm-type.h b/src/include/ckm/ckm-type.h index 8653009..822da90 100644 --- a/src/include/ckm/ckm-type.h +++ b/src/include/ckm/ckm-type.h @@ -71,10 +71,10 @@ struct AliasInfo { BackendId backend; AliasInfo() : type(0), backend(BackendId::SW) {} - AliasInfo(const Alias& alias, int type, BackendId backend) : - alias(alias), - type(type), - backend(backend) + AliasInfo(const Alias& _alias, int _type, BackendId _backend) : + alias(_alias), + type(_type), + backend(_backend) { } }; diff --git a/src/manager/client-async/async-request.cpp b/src/manager/client-async/async-request.cpp index b55bb6c..03b02ae 100644 --- a/src/manager/client-async/async-request.cpp +++ b/src/manager/client-async/async-request.cpp @@ -27,14 +27,14 @@ namespace CKM { AsyncRequest::AsyncRequest(const ManagerAsync::ObserverPtr &o, std::string &&i, RawBuffer &&b, - int id, - int command) : + int _id, + int _command) : observer(o), interface(std::move(i)), buffer(std::move(b)), written(0), - id(id), - command(command) + id(_id), + command(_command) { } diff --git a/src/manager/crypto/generic-backend/encryption-params.h b/src/manager/crypto/generic-backend/encryption-params.h index 5fa8e9c..b226869 100644 --- a/src/manager/crypto/generic-backend/encryption-params.h +++ b/src/manager/crypto/generic-backend/encryption-params.h @@ -28,9 +28,9 @@ namespace Crypto { struct EncryptionParams { EncryptionParams() {} - EncryptionParams(RawBuffer iv, RawBuffer tag) : - iv(std::move(iv)), - tag(std::move(tag)) + EncryptionParams(RawBuffer _iv, RawBuffer _tag) : + iv(std::move(_iv)), + tag(std::move(_tag)) {} RawBuffer iv; diff --git a/src/manager/crypto/sw-backend/kbkdf.cpp b/src/manager/crypto/sw-backend/kbkdf.cpp index 9f579fd..e37f795 100644 --- a/src/manager/crypto/sw-backend/kbkdf.cpp +++ b/src/manager/crypto/sw-backend/kbkdf.cpp @@ -176,8 +176,8 @@ RawBuffer deriveKbkdfHmac(const RawBuffer& keyInput, lengthBits, counterLocation, rlenBits, - [&fixed](Prf& prf, size_t){ - prf.Update(fixed.data(), fixed.size()); + [&fixed](Prf& prf2, size_t){ + prf2.Update(fixed.data(), fixed.size()); }); } diff --git a/src/manager/main/credentials.h b/src/manager/main/credentials.h index 7246a29..173d3a4 100644 --- a/src/manager/main/credentials.h +++ b/src/manager/main/credentials.h @@ -28,8 +28,8 @@ namespace CKM { struct Credentials { Credentials() : clientUid(0) {} - Credentials(uid_t socketUid, const ClientId &client) - : clientUid(socketUid), client(client) {} + Credentials(uid_t socketUid, const ClientId &_client) + : clientUid(socketUid), client(_client) {} uid_t clientUid; ClientId client; diff --git a/src/manager/main/generic-socket-manager.h b/src/manager/main/generic-socket-manager.h index 6532b4b..bce5da8 100644 --- a/src/manager/main/generic-socket-manager.h +++ b/src/manager/main/generic-socket-manager.h @@ -57,13 +57,13 @@ struct GenericSocketService { typedef std::string ServiceHandlerPath; struct ServiceDescription { ServiceDescription(const char *path, - const char *privilege, - InterfaceID interfaceID = 0, - bool useSendMsg = false) : - privilege(privilege), - interfaceID(interfaceID), + const char *_privilege, + InterfaceID _interfaceID = 0, + bool _useSendMsg = false) : + privilege(_privilege), + interfaceID(_interfaceID), serviceHandlerPath(path), - useSendMsg(useSendMsg) {} + useSendMsg(_useSendMsg) {} std::string privilege; diff --git a/src/manager/main/service-messages.h b/src/manager/main/service-messages.h index 6b40a7f..fb4620a 100644 --- a/src/manager/main/service-messages.h +++ b/src/manager/main/service-messages.h @@ -35,7 +35,7 @@ namespace CKM { // inter-service communication message base class struct MsgBase { - explicit MsgBase(unsigned id) : id(id) {} + explicit MsgBase(unsigned _id) : id(_id) {} virtual ~MsgBase() {} unsigned id; @@ -43,13 +43,13 @@ struct MsgBase { // key request struct MsgKeyRequest : public MsgBase { - MsgKeyRequest(unsigned id, const Credentials &cred, const Name &name, - const ClientId &explicitOwner, const Password &password) : - MsgBase(id), - cred(cred), - name(name), - explicitOwner(explicitOwner), - password(password) + MsgKeyRequest(unsigned _id, const Credentials &_cred, const Name &_name, + const ClientId &_explicitOwner, const Password &_password) : + MsgBase(_id), + cred(_cred), + name(_name), + explicitOwner(_explicitOwner), + password(_password) {} Credentials cred; @@ -60,9 +60,9 @@ struct MsgKeyRequest : public MsgBase { // key response struct MsgKeyResponse : public MsgBase { - MsgKeyResponse(unsigned id, const Crypto::GObjShPtr &key, + MsgKeyResponse(unsigned _id, const Crypto::GObjShPtr &_key, int errorCode = CKM_API_SUCCESS) : - MsgBase(id), key(key), error(errorCode) {} + MsgBase(_id), key(_key), error(errorCode) {} Crypto::GObjShPtr key; int error; diff --git a/src/manager/service/ckm-logic.cpp b/src/manager/service/ckm-logic.cpp index 98619e0..f57f920 100644 --- a/src/manager/service/ckm-logic.cpp +++ b/src/manager/service/ckm-logic.cpp @@ -823,13 +823,13 @@ RawBuffer CKMLogic::getData( int retCode = tryRet([&] { Crypto::GObjUPtr obj; - int retCode = readDataHelper(true, cred, dataType, name, owner, - password, obj, objDataType); + int retCode2 = readDataHelper(true, cred, dataType, name, owner, + password, obj, objDataType); - if (retCode == CKM_API_SUCCESS) + if (retCode2 == CKM_API_SUCCESS) rowData = obj->getBinary(); - return retCode; + return retCode2; }); if (CKM_API_SUCCESS != retCode) @@ -879,36 +879,36 @@ RawBuffer CKMLogic::getPKCS12( // read private key (mandatory) Crypto::GObjUPtr keyObj; - int retCode = readDataHelper(true, cred, DataType::DB_KEY_FIRST, name, owner, - keyPassword, keyObj); + int retCode2 = readDataHelper(true, cred, DataType::DB_KEY_FIRST, name, owner, + keyPassword, keyObj); - if (retCode != CKM_API_SUCCESS) { - if (retCode != CKM_API_ERROR_NOT_EXPORTABLE) - return retCode; + if (retCode2 != CKM_API_SUCCESS) { + if (retCode2 != CKM_API_ERROR_NOT_EXPORTABLE) + return retCode2; } else { privKey = CKM::Key::create(keyObj->getBinary()); } // read certificate (mandatory) Crypto::GObjUPtr certObj; - retCode = readDataHelper(true, cred, DataType::CERTIFICATE, name, owner, - certPassword, certObj); + retCode2 = readDataHelper(true, cred, DataType::CERTIFICATE, name, owner, + certPassword, certObj); - if (retCode != CKM_API_SUCCESS) { - if (retCode != CKM_API_ERROR_NOT_EXPORTABLE) - return retCode; + if (retCode2 != CKM_API_SUCCESS) { + if (retCode2 != CKM_API_ERROR_NOT_EXPORTABLE) + return retCode2; } else { cert = CKM::Certificate::create(certObj->getBinary(), DataFormat::FORM_DER); } // read CA cert chain (optional) Crypto::GObjUPtrVector caChainObjs; - retCode = readDataHelper(true, cred, DataType::DB_CHAIN_FIRST, name, owner, - certPassword, caChainObjs); + retCode2 = readDataHelper(true, cred, DataType::DB_CHAIN_FIRST, name, owner, + certPassword, caChainObjs); - if (retCode != CKM_API_SUCCESS && retCode != CKM_API_ERROR_DB_ALIAS_UNKNOWN) { - if (retCode != CKM_API_ERROR_NOT_EXPORTABLE) - return retCode; + if (retCode2 != CKM_API_SUCCESS && retCode2 != CKM_API_ERROR_DB_ALIAS_UNKNOWN) { + if (retCode2 != CKM_API_ERROR_NOT_EXPORTABLE) + return retCode2; } else { for (auto &caCertObj : caChainObjs) caChain.push_back(CKM::Certificate::create(caCertObj->getBinary(), @@ -917,11 +917,11 @@ RawBuffer CKMLogic::getPKCS12( // if anything found, return it if (privKey || cert || caChain.size() > 0) - retCode = CKM_API_SUCCESS; + retCode2 = CKM_API_SUCCESS; // prepare response - if (retCode != CKM_API_SUCCESS) - return retCode; + if (retCode2 != CKM_API_SUCCESS) + return retCode2; output = PKCS12Serializable(std::move(privKey), std::move(cert), std::move(caChain)); return CKM_API_SUCCESS; @@ -1200,9 +1200,9 @@ RawBuffer CKMLogic::createKeyAES( try { retCode = tryRet([&] { - auto [dbOp, digest, retCode] = beginSaveAndGetHash(cred, name, owner); - if (retCode != CKM_API_SUCCESS) - return retCode; + auto [dbOp, digest, retCode2] = beginSaveAndGetHash(cred, name, owner); + if (retCode2 != CKM_API_SUCCESS) + return retCode2; // create key in store CryptoAlgorithm keyGenAlgorithm; @@ -1433,13 +1433,13 @@ RawBuffer CKMLogic::createSignature( try { retCode = tryRet([&] { Crypto::GObjUPtr obj; - int retCode = readDataHelper(false, cred, DataType::DB_KEY_FIRST, privateKeyName, - owner, password, obj); + int retCode2 = readDataHelper(false, cred, DataType::DB_KEY_FIRST, privateKeyName, + owner, password, obj); - if (retCode == CKM_API_SUCCESS) + if (retCode2 == CKM_API_SUCCESS) signature = obj->sign(cryptoAlg, message); - return retCode; + return retCode2; }); } catch (const std::exception &e) { LogError("STD exception " << e.what()); @@ -1628,19 +1628,19 @@ RawBuffer CKMLogic::exportWrappedKey( RawBuffer wrappedKey; auto retCode = tryRet([&] { - auto retCode = readDataHelper(false, cred, DataType::DB_KEY_FIRST, wrappingKeyName, - wrappingKeyOwner, wrappingKeyPassword, wrappingKey); - if (retCode != CKM_API_SUCCESS) - return retCode; + auto retCode2 = readDataHelper(false, cred, DataType::DB_KEY_FIRST, wrappingKeyName, + wrappingKeyOwner, wrappingKeyPassword, wrappingKey); + if (retCode2 != CKM_API_SUCCESS) + return retCode2; - retCode = readRowHelper(false, cred, DataType::DB_KEY_FIRST, keyName, - keyOwner, keyPassword, wrappedKeyRow, wrappedKeyType); - if (retCode != CKM_API_SUCCESS) - return retCode; + retCode2 = readRowHelper(false, cred, DataType::DB_KEY_FIRST, keyName, + keyOwner, keyPassword, wrappedKeyRow, wrappedKeyType); + if (retCode2 != CKM_API_SUCCESS) + return retCode2; wrappedKey = wrappingKey->wrap(params, wrappedKeyRow, keyPassword); - return retCode; + return retCode2; }); return SerializeMessage(msgID, retCode, wrappedKeyType, wrappedKey); diff --git a/src/manager/service/db-crypto.cpp b/src/manager/service/db-crypto.cpp index ceccd69..3502121 100644 --- a/src/manager/service/db-crypto.cpp +++ b/src/manager/service/db-crypto.cpp @@ -264,8 +264,8 @@ bool Crypto::getDBVersion(int &schemaVersion) Transaction transaction(this); if (m_connection->CheckTableExist("SCHEMA_INFO")) { - SchemaInfo SchemaInfo(m_connection.get()); - if (SchemaInfo.getVersionInfo(schemaVersion)) { + SchemaInfo info(m_connection.get()); + if (info.getVersionInfo(schemaVersion)) { LogDebug("Current DB version: " << schemaVersion); return true; } @@ -315,8 +315,8 @@ void Crypto::initDatabase() } // update DB version info - SchemaInfo SchemaInfo(m_connection.get()); - SchemaInfo.setVersionInfo(); + SchemaInfo info(m_connection.get()); + info.setVersionInfo(); transaction.commit(); } return; @@ -357,8 +357,8 @@ void Crypto::createDBSchema() "Can not create the database schema: no initialization script"); m_connection->ExecCommand((*script).c_str()); - SchemaInfo SchemaInfo(m_connection.get()); - SchemaInfo.setVersionInfo(); + SchemaInfo info(m_connection.get()); + info.setVersionInfo(); transaction.commit(); } @@ -618,14 +618,14 @@ void Crypto::getRows( " name ", name, " with owner label ", owner); } -void Crypto::listInfos(const ClientId &owner, +void Crypto::listInfos(const ClientId &accessor, AliasInfoVector &aliasInfoVector, DataType type) { - listInfos(owner, aliasInfoVector, type, type); + listInfos(accessor, aliasInfoVector, type, type); } -void Crypto::listInfos(const ClientId &owner, +void Crypto::listInfos(const ClientId &accessor, AliasInfoVector &aliasInfoVector, DataType typeRangeStart, DataType typeRangeStop) @@ -636,9 +636,8 @@ void Crypto::listInfos(const ClientId &owner, m_connection->PrepareDataCommand(DB_CMD_INFO_SELECT_BY_TYPE_AND_PERMISSION); selectCommand->BindInteger(1, typeRangeStart); selectCommand->BindInteger(2, typeRangeStop); - selectCommand->BindString(104, owner.c_str()); - selectCommand->BindInteger(4, - static_cast(Permission::READ | Permission::REMOVE)); + selectCommand->BindString(104, accessor.c_str()); + selectCommand->BindInteger(4, static_cast(Permission::READ | Permission::REMOVE)); while (selectCommand->Step()) { ClientId owner = selectCommand->GetColumnString(0); @@ -674,7 +673,7 @@ void Crypto::listInfos(const ClientId &owner, "Couldn't list names of type <", typeRangeStart, ",", typeRangeStop, ">", - " accessible to client ", owner); + " accessible to client ", accessor); } void Crypto::saveKey( diff --git a/src/manager/service/db-crypto.h b/src/manager/service/db-crypto.h index a6f1a1a..83b13a8 100644 --- a/src/manager/service/db-crypto.h +++ b/src/manager/service/db-crypto.h @@ -99,12 +99,12 @@ public: RowVector &output); void listInfos( - const ClientId &owner, + const ClientId &accessor, AliasInfoVector &aliasInfoVector, DataType type); void listInfos( - const ClientId &owner, + const ClientId &accessor, AliasInfoVector &aliasInfoVector, DataType typeRangeStart, DataType typeRangeStop); diff --git a/unit-tests/test_descriptor-set.cpp b/unit-tests/test_descriptor-set.cpp index 5aad04b..22bc578 100644 --- a/unit-tests/test_descriptor-set.cpp +++ b/unit-tests/test_descriptor-set.cpp @@ -261,9 +261,9 @@ POSITIVE_TEST_CASE(T090_WriteAfterRead) readFd(desc, fd[0], revents); descriptors.remove(desc); - descriptors.add(fd2[1], POLLOUT, [&](int desc2, short revents) { + descriptors.add(fd2[1], POLLOUT, [&](int desc2, short revents2) { callback2 = true; - writeFd(desc2, fd2[1], revents); + writeFd(desc2, fd2[1], revents2); descriptors.remove(desc2); }); }); diff --git a/unit-tests/test_kbkdf.cpp b/unit-tests/test_kbkdf.cpp index 36a9e53..fa7ebd7 100644 --- a/unit-tests/test_kbkdf.cpp +++ b/unit-tests/test_kbkdf.cpp @@ -84,7 +84,7 @@ POSITIVE_TEST_CASE(kbkdf_test_vectors) // test case parameters size_t length; - RawBuffer key; + RawBuffer keyBuffer; size_t fixedLen; RawBuffer fixed; size_t labelLen; @@ -121,7 +121,7 @@ POSITIVE_TEST_CASE(kbkdf_test_vectors) rlen = 0; }}, {"L", [&](const std::string& value) { length = std::stoul(value); }}, - {"KI", [&](const std::string& value) { key = fromHex(value); }}, + {"KI", [&](const std::string& value) { keyBuffer = fromHex(value); }}, {"FixedInputDataByteLen", [&](const std::string& value) { fixedLen = std::stoul(value); }}, {"FixedInputData", [&](const std::string& value) { fixed = fromHex(value); }}, {"DataBeforeCtrLen", [&](const std::string& value) { labelLen = std::stoul(value); }}, @@ -139,14 +139,22 @@ POSITIVE_TEST_CASE(kbkdf_test_vectors) if (fixedLen > 0) { BOOST_REQUIRE(fixedLen == fixed.size()); - output = deriveKbkdfHmac(key, length, md, ctr, rlen, fixed); + output = deriveKbkdfHmac(keyBuffer, length, md, ctr, rlen, fixed); } else { BOOST_REQUIRE(labelLen > 0); BOOST_REQUIRE(labelLen == label.size()); BOOST_REQUIRE(contextLen > 0); BOOST_REQUIRE(contextLen == context.size()); - output = deriveKbkdfHmac(key, length, md, ctr, rlen, 0, label, context, false); + output = deriveKbkdfHmac(keyBuffer, + length, + md, + ctr, + rlen, + 0, + label, + context, + false); } BOOST_REQUIRE(output == expectedOutput); @@ -158,7 +166,7 @@ POSITIVE_TEST_CASE(kbkdf_test_vectors) labelLen = 0; context.clear(); contextLen = 0; - key.clear(); + keyBuffer.clear(); length = 0; }}, }; diff --git a/unit-tests/test_log-provider.cpp b/unit-tests/test_log-provider.cpp index 8a8c460..f98aefe 100644 --- a/unit-tests/test_log-provider.cpp +++ b/unit-tests/test_log-provider.cpp @@ -67,7 +67,7 @@ constexpr AbstractLogProvider::LogLevel defaultLogLevel() class Env { public: - explicit Env(const std::string& name) : name(name) + explicit Env(const std::string& _name) : name(_name) { char* val = getenv(name.c_str()); if (val) -- 2.7.4