From 3c548b2a442ff6b196ed4c0546265c19a4c29010 Mon Sep 17 00:00:00 2001 From: Hannes Rantzsch Date: Sun, 27 Aug 2023 16:48:51 +0200 Subject: [PATCH 1/3] ScopedCFRef helper to manage CF objects --- src/keychain_mac.cpp | 173 ++++++++++++++++++++----------------------- 1 file changed, 81 insertions(+), 92 deletions(-) diff --git a/src/keychain_mac.cpp b/src/keychain_mac.cpp index 76b51a3..1296586 100644 --- a/src/keychain_mac.cpp +++ b/src/keychain_mac.cpp @@ -30,6 +30,7 @@ #include "keychain.h" namespace { + /*! \brief Converts a CFString to a std::string * * This either uses CFStringGetCStringPtr or (if that fails) CFStringGetCString. @@ -101,67 +102,80 @@ void updateError(keychain::Error &err, OSStatus status) { } } -void handleCFCreateFailure(keychain::Error &err, - const std::string &errorMessage) { +void setGenericError(keychain::Error &err, const std::string &errorMessage) { + err = keychain::Error{}; err.message = errorMessage; err.type = keychain::ErrorType::GenericError; err.code = -1; } -CFStringRef createCFStringWithCString(const std::string &str, - keychain::Error &err) { - CFStringRef result = CFStringCreateWithCString( - kCFAllocatorDefault, str.c_str(), kCFStringEncodingUTF8); - if (result == NULL) { - handleCFCreateFailure(err, "Failed to create CFString"); +/*! \brief Helper to manage the lifetime of CFObjects */ +template struct ScopedCFRef { + public: + explicit ScopedCFRef(T &&ref) : _ref(std::move(ref)) {} + ~ScopedCFRef() { + if (_ref) + CFRelease(_ref); } + + ScopedCFRef(ScopedCFRef &&) noexcept = default; + ScopedCFRef(const ScopedCFRef &) = delete; + ScopedCFRef &operator=(const ScopedCFRef &) = delete; + ScopedCFRef &operator=(ScopedCFRef &&) = delete; + + const T &getCFRef() const { return _ref; } + operator bool() const { return _ref != nullptr; } + + private: + T _ref; +}; + +ScopedCFRef createCFStringWithCString(const std::string &str, + keychain::Error &err) { + auto result = ScopedCFRef(CFStringCreateWithCString( + kCFAllocatorDefault, str.c_str(), kCFStringEncodingUTF8)); + if (!result) + setGenericError(err, "Failed to create CFString"); return result; } -CFMutableDictionaryRef createCFMutableDictionary(keychain::Error &err) { - CFMutableDictionaryRef result = +ScopedCFRef +createCFMutableDictionary(keychain::Error &err) { + auto result = ScopedCFRef( CFDictionaryCreateMutable(kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks, - &kCFTypeDictionaryValueCallBacks); - if (result == NULL) { - handleCFCreateFailure(err, "Failed to create CFMutableDictionary"); - } + &kCFTypeDictionaryValueCallBacks)); + if (!result) + setGenericError(err, "Failed to create CFMutableDictionary"); return result; } -CFDataRef createCFData(const std::string &data, keychain::Error &err) { - CFDataRef result = +ScopedCFRef createCFData(const std::string &data, + keychain::Error &err) { + auto result = ScopedCFRef( CFDataCreate(kCFAllocatorDefault, reinterpret_cast(data.c_str()), - data.length()); - if (result == NULL) { - handleCFCreateFailure(err, "Failed to create CFData"); - } + data.length())); + if (!result) + setGenericError(err, "Failed to create CFData"); return result; } -CFMutableDictionaryRef createQuery(const std::string &serviceName, - const std::string &user, - keychain::Error &err) { - CFStringRef cfServiceName = createCFStringWithCString(serviceName, err); - CFStringRef cfUser = createCFStringWithCString(user, err); - CFMutableDictionaryRef query = createCFMutableDictionary(err); - - if (err.type != keychain::ErrorType::NoError) { - if (cfServiceName) - CFRelease(cfServiceName); - if (cfUser) - CFRelease(cfUser); - return NULL; - } +ScopedCFRef createQuery(const std::string &serviceName, + const std::string &user, + keychain::Error &err) { + const auto cfServiceName = createCFStringWithCString(serviceName, err); + const auto cfUser = createCFStringWithCString(user, err); + auto query = createCFMutableDictionary(err); - CFDictionaryAddValue(query, kSecClass, kSecClassGenericPassword); - CFDictionaryAddValue(query, kSecAttrAccount, cfUser); - CFDictionaryAddValue(query, kSecAttrService, cfServiceName); + if (err.type != keychain::ErrorType::NoError) + return query; - CFRelease(cfServiceName); - CFRelease(cfUser); + CFDictionaryAddValue(query.getCFRef(), kSecClass, kSecClassGenericPassword); + CFDictionaryAddValue(query.getCFRef(), kSecAttrAccount, cfUser.getCFRef()); + CFDictionaryAddValue( + query.getCFRef(), kSecAttrService, cfServiceName.getCFRef()); return query; } @@ -175,89 +189,64 @@ void setPassword(const std::string &package, const std::string &service, Error &err) { err = Error{}; const auto serviceName = makeServiceName(package, service); - CFDataRef cfPassword = createCFData(password, err); - CFMutableDictionaryRef query = createQuery(serviceName, user, err); + const auto cfPassword = createCFData(password, err); + auto query = createQuery(serviceName, user, err); - if (err.type != keychain::ErrorType::NoError) { + if (err.type != keychain::ErrorType::NoError) return; - } - - CFDictionaryAddValue(query, kSecValueData, cfPassword); - OSStatus status = SecItemAdd(query, NULL); + CFDictionaryAddValue( + query.getCFRef(), kSecValueData, cfPassword.getCFRef()); + OSStatus status = SecItemAdd(query.getCFRef(), NULL); if (status == errSecDuplicateItem) { // password exists -- override - CFMutableDictionaryRef attributesToUpdate = - createCFMutableDictionary(err); - if (err.type != keychain::ErrorType::NoError) { - CFRelease(cfPassword); - CFRelease(query); + auto attributesToUpdate = createCFMutableDictionary(err); + if (err.type != keychain::ErrorType::NoError) return; - } - CFDictionaryAddValue(attributesToUpdate, kSecValueData, cfPassword); - status = SecItemUpdate(query, attributesToUpdate); - - CFRelease(attributesToUpdate); - } - - if (status != errSecSuccess) { - updateError(err, status); + CFDictionaryAddValue(attributesToUpdate.getCFRef(), + kSecValueData, + cfPassword.getCFRef()); + status = SecItemUpdate(query.getCFRef(), attributesToUpdate.getCFRef()); } - CFRelease(cfPassword); - CFRelease(query); + updateError(err, status); } std::string getPassword(const std::string &package, const std::string &service, const std::string &user, Error &err) { err = Error{}; - std::string password; const auto serviceName = makeServiceName(package, service); - CFMutableDictionaryRef query = createQuery(serviceName, user, err); - - if (err.type != keychain::ErrorType::NoError) { - return password; - } + auto query = createQuery(serviceName, user, err); - CFDictionaryAddValue(query, kSecReturnData, kCFBooleanTrue); + if (err.type != keychain::ErrorType::NoError) + return ""; - CFTypeRef result = NULL; - OSStatus status = SecItemCopyMatching(query, &result); + CFDictionaryAddValue(query.getCFRef(), kSecReturnData, kCFBooleanTrue); - if (status != errSecSuccess) { - updateError(err, status); - } else if (result != NULL) { - CFDataRef cfPassword = (CFDataRef)result; - password = std::string( - reinterpret_cast(CFDataGetBytePtr(cfPassword)), - CFDataGetLength(cfPassword)); - CFRelease(result); - } + CFTypeRef result = nullptr; + updateError(err, SecItemCopyMatching(query.getCFRef(), &result)); + const auto cfPassword = ScopedCFRef((CFDataRef)result); - CFRelease(query); + if (!cfPassword || err.type != keychain::ErrorType::NoError) + return ""; - return password; + return std::string( + reinterpret_cast(CFDataGetBytePtr(cfPassword.getCFRef())), + CFDataGetLength(cfPassword.getCFRef())); } void deletePassword(const std::string &package, const std::string &service, const std::string &user, Error &err) { err = Error{}; const auto serviceName = makeServiceName(package, service); - CFMutableDictionaryRef query = createQuery(serviceName, user, err); + const auto query = createQuery(serviceName, user, err); - if (err.type != keychain::ErrorType::NoError) { + if (err.type != keychain::ErrorType::NoError) return; - } - - OSStatus status = SecItemDelete(query); - - if (status != errSecSuccess) { - updateError(err, status); - } - CFRelease(query); + updateError(err, SecItemDelete(query.getCFRef())); } } // namespace keychain From af24272a78a4d64ad6c80563aadad72118f413a4 Mon Sep 17 00:00:00 2001 From: Hannes Rantzsch Date: Sun, 22 Oct 2023 17:23:51 +0200 Subject: [PATCH 2/3] address review comments part I --- src/keychain_mac.cpp | 62 +++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/src/keychain_mac.cpp b/src/keychain_mac.cpp index 1296586..a11522f 100644 --- a/src/keychain_mac.cpp +++ b/src/keychain_mac.cpp @@ -23,6 +23,7 @@ * */ +#include #include #include @@ -110,23 +111,39 @@ void setGenericError(keychain::Error &err, const std::string &errorMessage) { } /*! \brief Helper to manage the lifetime of CFObjects */ -template struct ScopedCFRef { +template ::value>::type> +class ScopedCFRef { public: - explicit ScopedCFRef(T &&ref) : _ref(std::move(ref)) {} - ~ScopedCFRef() { - if (_ref) - CFRelease(_ref); + explicit ScopedCFRef(T ref) : _ref(ref) {} + ~ScopedCFRef() { _release(); } + + ScopedCFRef(ScopedCFRef &&other) noexcept : _ref(other._ref) { + other._ref = nullptr; + } + ScopedCFRef &operator=(ScopedCFRef &&other) { + if (this != &other) { + _release(); + _ref = other._ref; + other._ref = nullptr; + } + return *this; } - ScopedCFRef(ScopedCFRef &&) noexcept = default; ScopedCFRef(const ScopedCFRef &) = delete; ScopedCFRef &operator=(const ScopedCFRef &) = delete; - ScopedCFRef &operator=(ScopedCFRef &&) = delete; - const T &getCFRef() const { return _ref; } + const T get() const { return _ref; } operator bool() const { return _ref != nullptr; } private: + void _release() { + if (_ref != nullptr) { + CFRelease(_ref); + _ref = nullptr; + } + } + T _ref; }; @@ -172,10 +189,9 @@ ScopedCFRef createQuery(const std::string &serviceName, if (err.type != keychain::ErrorType::NoError) return query; - CFDictionaryAddValue(query.getCFRef(), kSecClass, kSecClassGenericPassword); - CFDictionaryAddValue(query.getCFRef(), kSecAttrAccount, cfUser.getCFRef()); - CFDictionaryAddValue( - query.getCFRef(), kSecAttrService, cfServiceName.getCFRef()); + CFDictionaryAddValue(query.get(), kSecClass, kSecClassGenericPassword); + CFDictionaryAddValue(query.get(), kSecAttrAccount, cfUser.get()); + CFDictionaryAddValue(query.get(), kSecAttrService, cfServiceName.get()); return query; } @@ -195,9 +211,8 @@ void setPassword(const std::string &package, const std::string &service, if (err.type != keychain::ErrorType::NoError) return; - CFDictionaryAddValue( - query.getCFRef(), kSecValueData, cfPassword.getCFRef()); - OSStatus status = SecItemAdd(query.getCFRef(), NULL); + CFDictionaryAddValue(query.get(), kSecValueData, cfPassword.get()); + OSStatus status = SecItemAdd(query.get(), NULL); if (status == errSecDuplicateItem) { // password exists -- override @@ -205,10 +220,9 @@ void setPassword(const std::string &package, const std::string &service, if (err.type != keychain::ErrorType::NoError) return; - CFDictionaryAddValue(attributesToUpdate.getCFRef(), - kSecValueData, - cfPassword.getCFRef()); - status = SecItemUpdate(query.getCFRef(), attributesToUpdate.getCFRef()); + CFDictionaryAddValue( + attributesToUpdate.get(), kSecValueData, cfPassword.get()); + status = SecItemUpdate(query.get(), attributesToUpdate.get()); } updateError(err, status); @@ -223,18 +237,18 @@ std::string getPassword(const std::string &package, const std::string &service, if (err.type != keychain::ErrorType::NoError) return ""; - CFDictionaryAddValue(query.getCFRef(), kSecReturnData, kCFBooleanTrue); + CFDictionaryAddValue(query.get(), kSecReturnData, kCFBooleanTrue); CFTypeRef result = nullptr; - updateError(err, SecItemCopyMatching(query.getCFRef(), &result)); + updateError(err, SecItemCopyMatching(query.get(), &result)); const auto cfPassword = ScopedCFRef((CFDataRef)result); if (!cfPassword || err.type != keychain::ErrorType::NoError) return ""; return std::string( - reinterpret_cast(CFDataGetBytePtr(cfPassword.getCFRef())), - CFDataGetLength(cfPassword.getCFRef())); + reinterpret_cast(CFDataGetBytePtr(cfPassword.get())), + CFDataGetLength(cfPassword.get())); } void deletePassword(const std::string &package, const std::string &service, @@ -246,7 +260,7 @@ void deletePassword(const std::string &package, const std::string &service, if (err.type != keychain::ErrorType::NoError) return; - updateError(err, SecItemDelete(query.getCFRef())); + updateError(err, SecItemDelete(query.get())); } } // namespace keychain From 6708591d44a1d23a29dfb8fec8b7d17e501693ef Mon Sep 17 00:00:00 2001 From: Hannes Rantzsch Date: Wed, 31 Jan 2024 15:14:50 +0100 Subject: [PATCH 3/3] Add a docstring for ScopedCFRef --- src/keychain_mac.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/keychain_mac.cpp b/src/keychain_mac.cpp index a11522f..1b84113 100644 --- a/src/keychain_mac.cpp +++ b/src/keychain_mac.cpp @@ -110,7 +110,14 @@ void setGenericError(keychain::Error &err, const std::string &errorMessage) { err.code = -1; } -/*! \brief Helper to manage the lifetime of CFObjects */ +/*! \brief Helper to manage the lifetime of CF-Objects + * + * This helper will CFRelease the managed CF-Object when it goes out of scope. + * It assumes ownership of the managed object, so users should own the object in + * terms of the Core Foundation "Create Rule" when passing it to the + * ScopedCFRef. Consequently, the object should also not be released by anyone + * else, at least not without calling CFRetain first. + * */ template ::value>::type> class ScopedCFRef {