From 6f31ec71d5b7bf12609e0147cdb91a79d10c7691 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 08:43:42 +0200 Subject: [PATCH 01/67] Bug fix. --- src/libcommonserver/io/iohelper_mac.cpp | 2 +- src/libcommonserver/io/iohelper_win.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index b862da0e7..d1f7f9c6c 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -230,7 +230,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex if (!exists) { return true; } - const bool isSymlink = itemType.linkType = LinkTypeSymlink; + const bool isSymlink = itemType.linkType == LinkTypeSymlink; std::error_code ec; std::filesystem::perms perms = diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index f44ae67f4..301228927 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -420,7 +420,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex if (!exists) { return true; } - const bool isSymlink = itemType.linkType = LinkTypeSymlink; + const bool isSymlink = itemType.linkType == LinkTypeSymlink; std::error_code ec; std::filesystem::perms perms = isSymlink ? std::filesystem::symlink_status(path, ec).permissions() From 3ff0ce2791bf9d813bf027eda6ce68f0da42b7ad Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 10:48:48 +0200 Subject: [PATCH 02/67] Add method definition. --- src/libcommonserver/io/iohelper.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index a669acd5c..b437294aa 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -258,6 +258,17 @@ struct IoHelper { // TODO: docstring and unit tests static bool getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept; + //! Set the rights of the item indicated by `path`. + /*! + \param path is the file system path of the item. + \param read is a boolean indicating whether the item should be readable. + \param write is a boolean indicating whether the item should be writable. + \param exec is a boolean indicating whether the item should be executable. + \param ioError holds the error returned when an underlying OS API call fails. + \return true if no unexpected error occurred, false otherwise. + */ + static bool setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; + protected: // These functions default to the std::filesystem functions. // They can be modified in tests. From 09299f303a067adc96eb50125cd0f2e8697cd7e6 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 10:49:48 +0200 Subject: [PATCH 03/67] Add description to getRights method definition. --- src/libcommonserver/io/iohelper.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index b437294aa..968a1640a 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -255,7 +255,15 @@ struct IoHelper { */ static bool checkIfFileIsDehydrated(const SyncPath &path, bool &isDehydrated, IoError &ioError) noexcept; - // TODO: docstring and unit tests + //! Get the rights of the item indicated by `path`. + /*! + \param path is the file system path of the item. + \param read is a boolean indicating whether the item is readable. + \param write is a boolean indicating whether the item is writable. + \param exec is a boolean indicating whether the item is executable. + \param exists is a boolean indicating whether the item exists. + \return true if no unexpected error occurred, false otherwise. + */ static bool getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept; //! Set the rights of the item indicated by `path`. From 9846184c6ce898b1552a9c9b18b72778b597f14d Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 10:51:08 +0200 Subject: [PATCH 04/67] Add Linux implementation of setRights. --- src/libcommonserver/io/iohelper_linux.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/libcommonserver/io/iohelper_linux.cpp b/src/libcommonserver/io/iohelper_linux.cpp index 88c9c3810..77521215b 100644 --- a/src/libcommonserver/io/iohelper_linux.cpp +++ b/src/libcommonserver/io/iohelper_linux.cpp @@ -141,5 +141,28 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } +bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + ioError = IoErrorSuccess; + + std::filesystem::perms perms = std::filesystem::perms::none; + if (read) { + perms |= std::filesystem::perms::owner_read; + } + if (write) { + perms |= std::filesystem::perms::owner_write; + } + if (exec) { + perms |= std::filesystem::perms::owner_exec; + } + + std::error_code ec; + std::filesystem::permissions(path, perms, ec); + if (ec.value() != 0) { + ioError = posixError2ioError(ec.value()); + return _isExpectedError(ioError); + } + + return true; +} } // namespace KDC From b65de93c191710b16868c1c53d0c0923d8ad7d27 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 11:16:49 +0200 Subject: [PATCH 05/67] Add implementation for macOS. --- src/libcommonserver/io/iohelper_mac.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index d1f7f9c6c..c3f099d13 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -253,4 +253,28 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } +bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + ioError = IoErrorSuccess; + + std::filesystem::perms perms = std::filesystem::perms::none; + if (read) { + perms |= std::filesystem::perms::owner_read; + } + if (write) { + perms |= std::filesystem::perms::owner_write; + } + if (exec) { + perms |= std::filesystem::perms::owner_exec; + } + + std::error_code ec; + std::filesystem::permissions(path, perms, ec); + if (ec.value() != 0) { + ioError = posixError2ioError(ec.value()); + return _isExpectedError(ioError); + } + + return true; +} + } // namespace KDC From 2aac874a84a203a30f3ed4e2c0eea88683e2c08e Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 11:17:47 +0200 Subject: [PATCH 06/67] Add Windows implementation. --- src/libcommonserver/io/iohelper.h | 2 +- src/libcommonserver/io/iohelper_win.cpp | 109 +++++++++++++++++++++++- 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 968a1640a..994925490 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -32,7 +32,7 @@ struct IoHelper { inline static void setLogger(log4cplus::Logger logger) { _logger = logger; } #ifdef _WIN32 - static int _getRightsMethod; + static int _getAndSetRightsMethod; #endif static IoError stdError2ioError(int error) noexcept; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 301228927..d1ca9c22a 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -89,7 +89,7 @@ time_t FileTimeToUnixTime(LARGE_INTEGER filetime, DWORD *remainder) { } } // namespace -int IoHelper::_getRightsMethod = 0; +int IoHelper::_getAndSetRightsMethod = 0; bool IoHelper::fileExists(const std::error_code &ec) noexcept { return (ec.value() != ERROR_FILE_NOT_FOUND) && (ec.value() != ERROR_PATH_NOT_FOUND) && (ec.value() != ERROR_INVALID_DRIVE); @@ -337,7 +337,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex IoError ioError = IoErrorSuccess; for (;;) { - switch (_getRightsMethod) { + switch (_getAndSetRightsMethod) { case 0: { if (Utility::_trustee.ptstrName) { WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; @@ -397,7 +397,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } // Try next method - IoHelper::_getRightsMethod++; + IoHelper::_getAndSetRightsMethod++; Utility::_trustee = {0}; LocalFree(Utility::_psid); Utility::_psid = NULL; @@ -449,6 +449,109 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } +bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + ioError = IoErrorSuccess; + + for (;;) { + switch (_getAndSetRightsMethod) { + case 0: { + if (Utility::_trustee.ptstrName) { + std::unique_ptr pSecurityDescriptor; + if (!InitializeSecurityDescriptor(*pSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { + ioError = dWordError2ioError(GetLastError()); + LOG_WARN(logger(), L"Error in InitializeSecurityDescriptor - path=" + << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } + + EXPLICIT_ACCESS ExplicitAccess[1]; + ZeroMemory(ExplicitAccess, sizeof(ExplicitAccess)); + + DWORD permission = READ_CONTROL | WRITE_DAC | WRITE_OWNER; // We need to keep these permissions to be able to + // change the permissions later + + if (read) permission |= GENERIC_READ; + if (write) permission |= GENERIC_WRITE | DELETE; + if (exec) permission |= GENERIC_EXECUTE; + + BuildExplicitAccessWithName(&ExplicitAccess[0], Utility::_trustee.ptstrName, permission, SET_ACCESS, + NO_INHERITANCE); + + PACL pACL_old = NULL; // Current ACL + PACL pACL_new = NULL; // New ACL + LPCWSTR pathw = Path2WStr(path).c_str(); + + DWORD ValueReturned = GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, + &pACL_old, NULL, pSecurityDescriptor.get()); + + DWORD result = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); + + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() + << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } + + if (!IsValidAcl(pACL_new)) { + ioError = IoErrorUnknown; + LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); + return false; // Invalid ACL, Failed to set permissions. + } + + result = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, + NULL); + + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() + << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } + + return true; + } + // Try next method + IoHelper::_getAndSetRightsMethod++; + Utility::_trustee = {0}; + LocalFree(Utility::_psid); + Utility::_psid = NULL; + + LOG_WARN(logger(), L"Trustee is not initialized, using fallback method"); + break; + } + case 1: { + // Fallback method only applied read only flag wich is not meaningful on Windows + std::filesystem::perms perms = std::filesystem::perms::none; + if (read) { + perms |= std::filesystem::perms::owner_read; + } + if (write) { + perms |= std::filesystem::perms::owner_write; + } + if (exec) { + perms |= std::filesystem::perms::owner_exec; + } + + std::error_code ec; + std::filesystem::permissions(path, perms, ec); + if (ec.value() != 0) { + ioError = posixError2ioError(ec.value()); + return _isExpectedError(ioError); + } + + return true; + + break; + } + } + } + + return true; +} + + bool IoHelper::checkIfIsJunction(const SyncPath &path, bool &isJunction, IoError &ioError) noexcept { isJunction = false; ioError = IoErrorSuccess; From d28cf25516b780937889ab55c88d6b33a6be71a2 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 11:35:45 +0200 Subject: [PATCH 07/67] Put MacOS and Linux implementation in the common ioHelper file to avoid code duplication. --- src/libcommonserver/io/iohelper.cpp | 31 +++++++++++++++++++++++ src/libcommonserver/io/iohelper.h | 2 ++ src/libcommonserver/io/iohelper_linux.cpp | 24 ------------------ src/libcommonserver/io/iohelper_mac.cpp | 24 ------------------ src/libcommonserver/io/iohelper_win.cpp | 1 - 5 files changed, 33 insertions(+), 49 deletions(-) diff --git a/src/libcommonserver/io/iohelper.cpp b/src/libcommonserver/io/iohelper.cpp index 5200c9656..b23b39fdd 100644 --- a/src/libcommonserver/io/iohelper.cpp +++ b/src/libcommonserver/io/iohelper.cpp @@ -507,4 +507,35 @@ bool IoHelper::createSymlink(const SyncPath &targetPath, const SyncPath &path, I return ioError == IoErrorSuccess; } + +#ifndef _WIN32 +bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + return _setRightsUnix(path, read, write, exec, ioError); +} +#endif + +bool IoHelper::_setRightsUnix(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + ioError = IoErrorSuccess; + std::filesystem::perms perms = std::filesystem::perms::none; + if (read) { + perms |= std::filesystem::perms::owner_read; + } + if (write) { + perms |= std::filesystem::perms::owner_write; + } + if (exec) { + perms |= std::filesystem::perms::owner_exec; + } + + std::error_code ec; + std::filesystem::permissions(path, perms, ec); + if (ec.value() != 0) { + ioError = posixError2ioError(ec.value()); + return _isExpectedError(ioError); + } + + return true; +} + + } // namespace KDC diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 994925490..d413cfa09 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -302,6 +302,8 @@ struct IoHelper { #endif static bool _setTargetType(ItemType &itemType) noexcept; static bool _checkIfIsHiddenFile(const SyncPath &path, bool &isHidden, IoError &ioError) noexcept; + + static bool _setRightsUnix(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; }; } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_linux.cpp b/src/libcommonserver/io/iohelper_linux.cpp index 77521215b..9529cdcef 100644 --- a/src/libcommonserver/io/iohelper_linux.cpp +++ b/src/libcommonserver/io/iohelper_linux.cpp @@ -141,28 +141,4 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } -bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - ioError = IoErrorSuccess; - - std::filesystem::perms perms = std::filesystem::perms::none; - if (read) { - perms |= std::filesystem::perms::owner_read; - } - if (write) { - perms |= std::filesystem::perms::owner_write; - } - if (exec) { - perms |= std::filesystem::perms::owner_exec; - } - - std::error_code ec; - std::filesystem::permissions(path, perms, ec); - if (ec.value() != 0) { - ioError = posixError2ioError(ec.value()); - return _isExpectedError(ioError); - } - - return true; -} - } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index c3f099d13..d1f7f9c6c 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -253,28 +253,4 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } -bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - ioError = IoErrorSuccess; - - std::filesystem::perms perms = std::filesystem::perms::none; - if (read) { - perms |= std::filesystem::perms::owner_read; - } - if (write) { - perms |= std::filesystem::perms::owner_write; - } - if (exec) { - perms |= std::filesystem::perms::owner_exec; - } - - std::error_code ec; - std::filesystem::permissions(path, perms, ec); - if (ec.value() != 0) { - ioError = posixError2ioError(ec.value()); - return _isExpectedError(ioError); - } - - return true; -} - } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index d1ca9c22a..7415f77b5 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -551,7 +551,6 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, return true; } - bool IoHelper::checkIfIsJunction(const SyncPath &path, bool &isJunction, IoError &ioError) noexcept { isJunction = false; ioError = IoErrorSuccess; From ea2c4cf675cb6c069d86bcd5784bec3b741c71c2 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 22 Apr 2024 11:47:10 +0200 Subject: [PATCH 08/67] Reduce code duplication and cognitive complexity. --- src/libcommonserver/io/iohelper_win.cpp | 126 +++++++++--------------- 1 file changed, 45 insertions(+), 81 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 7415f77b5..2d9a7f9d8 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -452,100 +452,64 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { ioError = IoErrorSuccess; - for (;;) { - switch (_getAndSetRightsMethod) { - case 0: { - if (Utility::_trustee.ptstrName) { - std::unique_ptr pSecurityDescriptor; - if (!InitializeSecurityDescriptor(*pSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { - ioError = dWordError2ioError(GetLastError()); - LOG_WARN(logger(), L"Error in InitializeSecurityDescriptor - path=" - << Path2WStr(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); - } - - EXPLICIT_ACCESS ExplicitAccess[1]; - ZeroMemory(ExplicitAccess, sizeof(ExplicitAccess)); - - DWORD permission = READ_CONTROL | WRITE_DAC | WRITE_OWNER; // We need to keep these permissions to be able to - // change the permissions later - - if (read) permission |= GENERIC_READ; - if (write) permission |= GENERIC_WRITE | DELETE; - if (exec) permission |= GENERIC_EXECUTE; + if (Utility::_trustee.ptstrName) { + std::unique_ptr pSecurityDescriptor; + if (!InitializeSecurityDescriptor(*pSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { + ioError = dWordError2ioError(GetLastError()); + LOG_WARN(logger(), L"Error in InitializeSecurityDescriptor - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } - BuildExplicitAccessWithName(&ExplicitAccess[0], Utility::_trustee.ptstrName, permission, SET_ACCESS, - NO_INHERITANCE); + EXPLICIT_ACCESS ExplicitAccess[1]; + ZeroMemory(ExplicitAccess, sizeof(ExplicitAccess)); - PACL pACL_old = NULL; // Current ACL - PACL pACL_new = NULL; // New ACL - LPCWSTR pathw = Path2WStr(path).c_str(); + DWORD permission = READ_CONTROL | WRITE_DAC | WRITE_OWNER; // We need to keep these permissions to be able to + // change the permissions later - DWORD ValueReturned = GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, - &pACL_old, NULL, pSecurityDescriptor.get()); + if (read) permission |= GENERIC_READ; + if (write) permission |= GENERIC_WRITE | DELETE; + if (exec) permission |= GENERIC_EXECUTE; - DWORD result = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); + BuildExplicitAccessWithName(&ExplicitAccess[0], Utility::_trustee.ptstrName, permission, SET_ACCESS, NO_INHERITANCE); - if (result != ERROR_SUCCESS) { - ioError = dWordError2ioError(result); - LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() - << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); - } + PACL pACL_old = NULL; // Current ACL + PACL pACL_new = NULL; // New ACL + LPCWSTR pathw = Path2WStr(path).c_str(); - if (!IsValidAcl(pACL_new)) { - ioError = IoErrorUnknown; - LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); - return false; // Invalid ACL, Failed to set permissions. - } + DWORD ValueReturned = GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pACL_old, NULL, + pSecurityDescriptor.get()); - result = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, - NULL); + DWORD result = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); - if (result != ERROR_SUCCESS) { - ioError = dWordError2ioError(result); - LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() - << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); - } + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } - return true; - } - // Try next method - IoHelper::_getAndSetRightsMethod++; - Utility::_trustee = {0}; - LocalFree(Utility::_psid); - Utility::_psid = NULL; + if (!IsValidAcl(pACL_new)) { + ioError = IoErrorUnknown; + LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); + return false; // Invalid ACL, Failed to set permissions. + } - LOG_WARN(logger(), L"Trustee is not initialized, using fallback method"); - break; - } - case 1: { - // Fallback method only applied read only flag wich is not meaningful on Windows - std::filesystem::perms perms = std::filesystem::perms::none; - if (read) { - perms |= std::filesystem::perms::owner_read; - } - if (write) { - perms |= std::filesystem::perms::owner_write; - } - if (exec) { - perms |= std::filesystem::perms::owner_exec; - } + result = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, NULL); - std::error_code ec; - std::filesystem::permissions(path, perms, ec); - if (ec.value() != 0) { - ioError = posixError2ioError(ec.value()); - return _isExpectedError(ioError); - } + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + return _isExpectedError(ioError); + } - return true; + return true; - break; - } - } + } else { + // Fallback method only applied read only flag wich is not meaningful on Windows + LOG_WARN(logger(), L"Trustee is not initialized, using fallback method"); + return _setRightsUnix(path, read, write, exec, ioError); } return true; From 13799f23accbe42350052384edda4ec0e0da75b3 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 23 Apr 2024 14:20:59 +0200 Subject: [PATCH 09/67] Windows implementation. --- src/libcommonserver/io/iohelper.h | 10 +- src/libcommonserver/io/iohelper_win.cpp | 327 +++++++++++++----------- 2 files changed, 191 insertions(+), 146 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index d413cfa09..1e85235cb 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -20,6 +20,9 @@ #include "libcommon/utility/types.h" #include "libcommonserver/log/log.h" +#ifdef _WIN32 +#include +#endif namespace KDC { @@ -256,7 +259,7 @@ struct IoHelper { static bool checkIfFileIsDehydrated(const SyncPath &path, bool &isDehydrated, IoError &ioError) noexcept; //! Get the rights of the item indicated by `path`. - /*! + /*! \param path is the file system path of the item. \param read is a boolean indicating whether the item is readable. \param write is a boolean indicating whether the item is writable. @@ -304,6 +307,11 @@ struct IoHelper { static bool _checkIfIsHiddenFile(const SyncPath &path, bool &isHidden, IoError &ioError) noexcept; static bool _setRightsUnix(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; + +#ifdef _WIN32 + static bool _setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, + IoError &ioError) noexcept; +#endif }; } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 2d9a7f9d8..d565a21ab 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -336,182 +336,219 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex exists = false; IoError ioError = IoErrorSuccess; - for (;;) { - switch (_getAndSetRightsMethod) { - case 0: { - if (Utility::_trustee.ptstrName) { - WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; - lstrcpyW(szFilePath, path.native().c_str()); - - // Get security info - PACL pfileACL = NULL; - PSECURITY_DESCRIPTOR psecDesc = NULL; - DWORD result = GetNamedSecurityInfoW(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, - &pfileACL, NULL, &psecDesc); - ioError = dWordError2ioError(result); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; - } - - if (ioError == IoErrorAccessDenied) { - read = false; - write = false; - exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return true; - } - - LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW - path=" << szFilePath << L" result=" << result); - } else { - // Get rights for trustee - ACCESS_MASK rights = 0; - result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - ioError = dWordError2ioError(result); - LocalFree(psecDesc); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; - } - - if (ioError == IoErrorAccessDenied) { - read = false; - write = false; - exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return true; - } - - LOGW_WARN(logger(), - L"Error in GetEffectiveRightsFromAcl - path=" << szFilePath << L" result=" << result); - } else { - exists = true; - read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; - write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; - exec = (rights & FILE_GENERIC_EXECUTE) == FILE_GENERIC_EXECUTE; - return true; - } - } - } - // Try next method - IoHelper::_getAndSetRightsMethod++; - Utility::_trustee = {0}; - LocalFree(Utility::_psid); - Utility::_psid = NULL; - break; + if (Utility::_trustee.ptstrName) { + WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; + lstrcpyW(szFilePath, path.native().c_str()); + + // Get security info + PACL pfileACL = NULL; + PSECURITY_DESCRIPTOR psecDesc = NULL; + DWORD result = + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + LocalFree(psecDesc); + ioError = dWordError2ioError(result); + if (ioError != IoErrorSuccess) { + exists = ioError != IoErrorNoSuchFileOrDirectory; + + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); } - case 1: { - // Fallback method - // !!! When Deny Full control to file/directory => returns exists == false !!! - - ItemType itemType; - const bool success = getItemType(path, itemType); - ioError = itemType.ioError; - if (!success) { - LOGW_WARN(logger(), - L"Failed to check if the item is a symlink - " << Utility::formatIoError(path, ioError).c_str()); - return false; - } + LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW - path=" << szFilePath << L" result=" << result); + return _isExpectedError(ioError); + + } else { + // Get rights for trustee + ACCESS_MASK rights = 0; + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + ioError = dWordError2ioError(result); + + if (ioError != IoErrorSuccess) { exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); } - const bool isSymlink = itemType.linkType == LinkTypeSymlink; - - std::error_code ec; - std::filesystem::perms perms = isSymlink ? std::filesystem::symlink_status(path, ec).permissions() - : std::filesystem::status(path, ec).permissions(); - ioError = stdError2ioError(ec); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; + LOGW_WARN(logger(), L"Error in GetEffectiveRightsFromAcl - path=" << szFilePath << L" result=" << result); + return _isExpectedError(ioError); + } else { + bool readCtrl = + (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) + if (!readCtrl) { + _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError); // Force read control + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, + &psecDesc); + LocalFree(psecDesc); + GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, + ioError); // Revoke read control after reading the permissions + readCtrl = (rights & READ_CONTROL) == READ_CONTROL; + if (!readCtrl) { + ioError = IoErrorAccessDenied; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + return _isExpectedError(ioError); } - - LOGW_WARN(logger(), L"Failed to get permissions - " << Utility::formatStdError(path, ec).c_str()); - return false; } exists = true; - read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); - write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); - exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); + read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; + write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; + exec = (rights & FILE_GENERIC_EXECUTE) == FILE_GENERIC_EXECUTE; return true; - break; } } - } - - return true; -} - -bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - ioError = IoErrorSuccess; + } else { + // Fallback method + // !!! When Deny Full control to file/directory => returns exists == false !!! + + ItemType itemType; + const bool success = getItemType(path, itemType); + ioError = itemType.ioError; + if (!success) { + LOGW_WARN(logger(), L"Failed to check if the item is a symlink - " << Utility::formatIoError(path, ioError).c_str()); + return false; + } - if (Utility::_trustee.ptstrName) { - std::unique_ptr pSecurityDescriptor; - if (!InitializeSecurityDescriptor(*pSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { - ioError = dWordError2ioError(GetLastError()); - LOG_WARN(logger(), L"Error in InitializeSecurityDescriptor - path=" << Path2WStr(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); + exists = ioError != IoErrorNoSuchFileOrDirectory; + if (!exists) { + return true; } + const bool isSymlink = itemType.linkType == LinkTypeSymlink; + + std::error_code ec; + std::filesystem::perms perms = + isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); + ioError = stdError2ioError(ec); + if (ioError != IoErrorSuccess) { + exists = ioError != IoErrorNoSuchFileOrDirectory; + if (!exists) { + return true; + } - EXPLICIT_ACCESS ExplicitAccess[1]; - ZeroMemory(ExplicitAccess, sizeof(ExplicitAccess)); + LOGW_WARN(logger(), L"Failed to get permissions - " << Utility::formatStdError(path, ec).c_str()); + return false; + } - DWORD permission = READ_CONTROL | WRITE_DAC | WRITE_OWNER; // We need to keep these permissions to be able to - // change the permissions later + exists = true; + read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); + write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); + exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); + return true; + } - if (read) permission |= GENERIC_READ; - if (write) permission |= GENERIC_WRITE | DELETE; - if (exec) permission |= GENERIC_EXECUTE; - BuildExplicitAccessWithName(&ExplicitAccess[0], Utility::_trustee.ptstrName, permission, SET_ACCESS, NO_INHERITANCE); + return true; +} - PACL pACL_old = NULL; // Current ACL - PACL pACL_new = NULL; // New ACL - LPCWSTR pathw = Path2WStr(path).c_str(); +bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { + if (Utility::_trustee.ptstrName) { + ioError = IoErrorSuccess; + DWORD grantedPermission = WRITE_DAC; + DWORD deniedPermission = 0; - DWORD ValueReturned = GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pACL_old, NULL, - pSecurityDescriptor.get()); + if (!read) { + deniedPermission |= FILE_GENERIC_READ & ~READ_CONTROL & ~SYNCHRONIZE; + } else { + grantedPermission |= FILE_GENERIC_READ; + } - DWORD result = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); + if (!write) { + deniedPermission |= FILE_GENERIC_WRITE & ~READ_CONTROL & ~SYNCHRONIZE; + } else { + grantedPermission |= FILE_GENERIC_WRITE; + } - if (result != ERROR_SUCCESS) { - ioError = dWordError2ioError(result); - LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); + if (!exec) { + deniedPermission |= FILE_GENERIC_EXECUTE & ~READ_CONTROL & ~SYNCHRONIZE & ~FILE_READ_ATTRIBUTES; + } else { + grantedPermission |= FILE_GENERIC_EXECUTE; } - if (!IsValidAcl(pACL_new)) { - ioError = IoErrorUnknown; - LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); - return false; // Invalid ACL, Failed to set permissions. + bool res = false; + res = _setRightsWindows(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError); + if (!res) { + return false; } + res = _setRightsWindows(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError); + return res; + } else { + return _setRightsUnix(path, read, write, exec, ioError); + } +} - result = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, NULL); +bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError) noexcept { + PACL pACL_old = NULL; // Current ACL + PACL pACL_new = NULL; // New ACL + PSECURITY_DESCRIPTOR pSecurityDescriptor = NULL; + EXPLICIT_ACCESS ExplicitAccess[1]; + ZeroMemory(&ExplicitAccess[0], sizeof(ExplicitAccess)); + + ExplicitAccess[0].grfAccessPermissions = permission; + ExplicitAccess[0].grfAccessMode = accessMode; + ExplicitAccess[0].grfInheritance = NO_INHERITANCE; + ExplicitAccess[0].Trustee.pMultipleTrustee = NULL; + ExplicitAccess[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + ExplicitAccess[0].Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; + ExplicitAccess[0].Trustee.TrusteeType = Utility::_trustee.TrusteeType; + ExplicitAccess[0].Trustee.ptstrName = Utility::_trustee.ptstrName; + + LPCWSTR pathw = Path2WStr(path).c_str(); + DWORD ValueReturned = + GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pACL_old, NULL, &pSecurityDescriptor); + + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger(), L"Error in GetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return _isExpectedError(ioError); + } - if (result != ERROR_SUCCESS) { - ioError = dWordError2ioError(result); - LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - return _isExpectedError(ioError); - } + ValueReturned = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return _isExpectedError(ioError); + } - return true; + if (!IsValidAcl(pACL_new)) { + ioError = IoErrorUnknown; + LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); - } else { - // Fallback method only applied read only flag wich is not meaningful on Windows - LOG_WARN(logger(), L"Trustee is not initialized, using fallback method"); - return _setRightsUnix(path, read, write, exec, ioError); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return false; // Invalid ACL, Failed to set permissions. } + ValueReturned = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, NULL); + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return _isExpectedError(ioError); + } + + + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return true; } From 40be3d45a3e33e4c9108b6bc503442e2ea7a818b Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 23 Apr 2024 14:53:56 +0200 Subject: [PATCH 10/67] Sentry compliant. --- src/libcommonserver/io/iohelper.h | 2 +- src/libcommonserver/io/iohelper_win.cpp | 109 +++++++++++++----------- 2 files changed, 62 insertions(+), 49 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 1e85235cb..46ecf7970 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -21,7 +21,7 @@ #include "libcommon/utility/types.h" #include "libcommonserver/log/log.h" #ifdef _WIN32 -#include +#include #endif namespace KDC { diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index d565a21ab..73019fa15 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -337,6 +337,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex IoError ioError = IoErrorSuccess; + // Preferred method if (Utility::_trustee.ptstrName) { WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; lstrcpyW(szFilePath, path.native().c_str()); @@ -360,50 +361,54 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW - path=" << szFilePath << L" result=" << result); return _isExpectedError(ioError); + } - } else { - // Get rights for trustee - ACCESS_MASK rights = 0; - result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - ioError = dWordError2ioError(result); - - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (ioError == IoErrorAccessDenied) { - read = false; - write = false; - exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - } - LOGW_WARN(logger(), L"Error in GetEffectiveRightsFromAcl - path=" << szFilePath << L" result=" << result); + + // Get rights for trustee + ACCESS_MASK rights = 0; + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + ioError = dWordError2ioError(result); + + if (ioError != IoErrorSuccess) { + exists = ioError != IoErrorNoSuchFileOrDirectory; + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + } + LOGW_WARN(logger(), L"Error in GetEffectiveRightsFromAcl - path=" << szFilePath << L" result=" << result); + return _isExpectedError(ioError); + } + + bool readCtrl = + (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) + + if (!readCtrl) { + _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError); // Force read control + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + LocalFree(psecDesc); + GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, + ioError); // Revoke read control after reading the permissions + readCtrl = (rights & READ_CONTROL) == READ_CONTROL; + if (!readCtrl) { + ioError = IoErrorAccessDenied; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); return _isExpectedError(ioError); - } else { - bool readCtrl = - (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) - if (!readCtrl) { - _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError); // Force read control - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, - &psecDesc); - LocalFree(psecDesc); - GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, - ioError); // Revoke read control after reading the permissions - readCtrl = (rights & READ_CONTROL) == READ_CONTROL; - if (!readCtrl) { - ioError = IoErrorAccessDenied; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return _isExpectedError(ioError); - } - } - - exists = true; - read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; - write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; - exec = (rights & FILE_GENERIC_EXECUTE) == FILE_GENERIC_EXECUTE; - return true; } } - } else { + + exists = true; + read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; + write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; + exec = (rights & FILE_GENERIC_EXECUTE) == FILE_GENERIC_EXECUTE; + return true; + + + } + // Fallback method + else { // Fallback method // !!! When Deny Full control to file/directory => returns exists == false !!! @@ -483,24 +488,31 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError) noexcept { - PACL pACL_old = NULL; // Current ACL - PACL pACL_new = NULL; // New ACL - PSECURITY_DESCRIPTOR pSecurityDescriptor = NULL; + PACL pACL_old = nullptr; // Current ACL + PACL pACL_new = nullptr; // New ACL + PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; EXPLICIT_ACCESS ExplicitAccess[1]; ZeroMemory(&ExplicitAccess[0], sizeof(ExplicitAccess)); ExplicitAccess[0].grfAccessPermissions = permission; ExplicitAccess[0].grfAccessMode = accessMode; ExplicitAccess[0].grfInheritance = NO_INHERITANCE; - ExplicitAccess[0].Trustee.pMultipleTrustee = NULL; + ExplicitAccess[0].Trustee.pMultipleTrustee = nullptr; ExplicitAccess[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; ExplicitAccess[0].Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; ExplicitAccess[0].Trustee.TrusteeType = Utility::_trustee.TrusteeType; ExplicitAccess[0].Trustee.ptstrName = Utility::_trustee.ptstrName; - LPCWSTR pathw = Path2WStr(path).c_str(); - DWORD ValueReturned = - GetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pACL_old, NULL, &pSecurityDescriptor); + LPCWSTR pathw_c = Path2WStr(path).c_str(); + + std::unique_ptr pathw_ptr(new WCHAR[wcslen(pathw_c) + 1]); + LPWSTR pathw = pathw_ptr.get(); + wcscpy(pathw, pathw_c); + + + + DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, + nullptr, &pSecurityDescriptor); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); @@ -533,7 +545,8 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ return false; // Invalid ACL, Failed to set permissions. } - ValueReturned = SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, pACL_new, NULL); + ValueReturned = + SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" From 8e5111b751e1eadeb3cf8cdcabce354d2bb53ae6 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 23 Apr 2024 15:12:20 +0200 Subject: [PATCH 11/67] Sentry compliance. --- src/libcommonserver/io/iohelper_win.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 73019fa15..9ce92aa3f 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -504,10 +504,11 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ ExplicitAccess[0].Trustee.ptstrName = Utility::_trustee.ptstrName; LPCWSTR pathw_c = Path2WStr(path).c_str(); - - std::unique_ptr pathw_ptr(new WCHAR[wcslen(pathw_c) + 1]); + size_t pathw_len = wcslen(pathw_c); + std::unique_ptr pathw_ptr(new WCHAR[pathw_len + 1]); LPWSTR pathw = pathw_ptr.get(); wcscpy(pathw, pathw_c); + pathw[pathw_len] = '\0'; From 63a7e9e1b4e43ddaabfb26036a095fd88859d66b Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 23 Apr 2024 15:12:20 +0200 Subject: [PATCH 12/67] Sentry compliance. --- src/libcommonserver/io/iohelper_win.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 73019fa15..767075ae9 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -504,11 +504,12 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ ExplicitAccess[0].Trustee.ptstrName = Utility::_trustee.ptstrName; LPCWSTR pathw_c = Path2WStr(path).c_str(); + size_t pathw_len = Path2WStr(path).length(); - std::unique_ptr pathw_ptr(new WCHAR[wcslen(pathw_c) + 1]); + std::unique_ptr pathw_ptr(new WCHAR[pathw_len + 1]); + Path2WStr(path).copy(pathw_ptr.get(), pathw_len); LPWSTR pathw = pathw_ptr.get(); - wcscpy(pathw, pathw_c); - + pathw[pathw_len] = L'\0'; DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, From abf61b6f0a3eb206fa25fdc9972db8213491149a Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 23 Apr 2024 15:47:36 +0200 Subject: [PATCH 13/67] Sentry compliance. --- src/libcommonserver/io/iohelper.h | 2 +- src/libcommonserver/io/iohelper_win.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 46ecf7970..fd95a646a 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -21,7 +21,7 @@ #include "libcommon/utility/types.h" #include "libcommonserver/log/log.h" #ifdef _WIN32 -#include +#include #endif namespace KDC { diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 767075ae9..f2537855e 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -39,7 +39,7 @@ #include #include #include -#include +#include #define SECURITY_WIN32 #include @@ -506,7 +506,7 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ LPCWSTR pathw_c = Path2WStr(path).c_str(); size_t pathw_len = Path2WStr(path).length(); - std::unique_ptr pathw_ptr(new WCHAR[pathw_len + 1]); + std::unique_ptr pathw_ptr = std::make_unique(pathw_len + 1); Path2WStr(path).copy(pathw_ptr.get(), pathw_len); LPWSTR pathw = pathw_ptr.get(); pathw[pathw_len] = L'\0'; @@ -547,7 +547,7 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ } ValueReturned = - SetNamedSecurityInfo((LPWSTR)pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); + SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" From 8d00f136bbaaff4c610219a7fd177d440972604b Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 08:33:45 +0200 Subject: [PATCH 14/67] Apply review suggestion. --- src/libcommonserver/io/iohelper.cpp | 2 +- src/server/appserver.cpp | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libcommonserver/io/iohelper.cpp b/src/libcommonserver/io/iohelper.cpp index b23b39fdd..88da61efe 100644 --- a/src/libcommonserver/io/iohelper.cpp +++ b/src/libcommonserver/io/iohelper.cpp @@ -529,7 +529,7 @@ bool IoHelper::_setRightsUnix(const SyncPath &path, bool read, bool write, bool std::error_code ec; std::filesystem::permissions(path, perms, ec); - if (ec.value() != 0) { + if (ec) { ioError = posixError2ioError(ec.value()); return _isExpectedError(ioError); } diff --git a/src/server/appserver.cpp b/src/server/appserver.cpp index 93855a6da..1cf49e2cc 100644 --- a/src/server/appserver.cpp +++ b/src/server/appserver.cpp @@ -352,8 +352,6 @@ AppServer::AppServer(int &argc, char **argv) connect(&_restartSyncsTimer, &QTimer::timeout, this, &AppServer::onRestartSyncs); _restartSyncsTimer.start(RESTART_SYNCS_INTERVAL); - SyncPath test = "C:/Users/Herve/Documents/test"; - IoHelper::setRights(test, false, false, false, ioError); } AppServer::~AppServer() { From a161da59f229ec2b4ad8a4c3a721a98aad0a4584 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 08:43:25 +0200 Subject: [PATCH 15/67] Replace Path2WStr by Utility::formatSyncPath in log generation. --- src/libcommonserver/io/iohelper_win.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index f2537855e..21ab6144f 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -106,7 +106,7 @@ bool IoHelper::getNodeId(const SyncPath &path, NodeId &nodeId) noexcept { FILE_FLAG_BACKUP_SEMANTICS, NULL); if (hParent == INVALID_HANDLE_VALUE) { - LOGW_INFO(Log::instance()->getLogger(), L"Error in CreateFileW - path=" << Path2WStr(path.parent_path()).c_str()); + LOGW_INFO(Log::instance()->getLogger(), L"Error in CreateFileW: " << Utility::formatSyncPath(path.parent_path()).c_str()); return false; } @@ -129,7 +129,7 @@ bool IoHelper::getNodeId(const SyncPath &path, NodeId &nodeId) noexcept { (PZW_QUERY_DIRECTORY_FILE)GetProcAddress(GetModuleHandle(L"ntdll.dll"), "ZwQueryDirectoryFile"); if (zwQueryDirectoryFile == 0) { - LOG_WARN(Log::instance()->getLogger(), L"Error in GetProcAddress - path=" << Path2WStr(path.parent_path()).c_str()); + LOG_WARN(Log::instance()->getLogger(), L"Error in GetProcAddress: " << Utility::formatSyncPath(path.parent_path()).c_str()); return false; } @@ -175,12 +175,12 @@ bool IoHelper::getFileStat(const SyncPath &path, FileStat *buf, bool &exists, Io retry = true; Utility::msleep(10); LOGW_DEBUG(Log::instance()->getLogger(), - L"Retrying to get handle - path=" << Path2WStr(path.parent_path()).c_str()); + L"Retrying to get handle: " << Utility::formatSyncPath(path.parent_path()).c_str()); counter--; continue; } - LOG_WARN(logger(), L"Error in CreateFileW - path=" << Path2WStr(path.parent_path()).c_str()); + LOG_WARN(logger(), L"Error in CreateFileW: " << Utility::formatSyncPath(path.parent_path()).c_str()); ioError = dWordError2ioError(GetLastError()); exists = false; @@ -223,7 +223,7 @@ bool IoHelper::getFileStat(const SyncPath &path, FileStat *buf, bool &exists, Io (!isNtfs && dwError != 0)) { // On FAT32 file system, NT_SUCCESS will return false even if it is a success, therefore we // also check GetLastError LOGW_DEBUG(Log::instance()->getLogger(), - L"Error in zwQueryDirectoryFile - path=" << Path2WStr(path.parent_path()).c_str()); + L"Error in zwQueryDirectoryFile: " << Utility::formatSyncPath(path.parent_path()).c_str()); CloseHandle(hParent); exists = false; ioError = dWordError2ioError(dwError); @@ -517,7 +517,7 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in GetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" + LOG_WARN(logger(), L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -528,7 +528,7 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ ValueReturned = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in SetEntriesInAcl - path=" << Path2WStr(path).c_str() << L" error=" + LOG_WARN(logger(), L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -538,19 +538,19 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ if (!IsValidAcl(pACL_new)) { ioError = IoErrorUnknown; - LOG_WARN(logger(), L"Invalid ACL - path=" << Path2WStr(path).c_str()); + LOG_WARN(logger(), L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - return false; // Invalid ACL, Failed to set permissions. + return false; } ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in SetNamedSecurityInfo - path=" << Path2WStr(path).c_str() << L" error=" + LOG_WARN(logger(), L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); From 442f5be3b7f24f08a91cf482acfe1b1885d83aaf Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 08:51:37 +0200 Subject: [PATCH 16/67] Update log message in case of getItemType Fail. --- src/libcommonserver/io/iohelper_mac.cpp | 2 +- src/libcommonserver/io/iohelper_win.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index d1f7f9c6c..55d330c27 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -223,7 +223,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex const bool success = getItemType(path, itemType); if (!success) { LOGW_WARN(logger(), - L"Failed to check if the item is a symlink: " << Utility::formatIoError(path, itemType.ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); return false; } exists = itemType.ioError != IoErrorNoSuchFileOrDirectory; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 21ab6144f..516d85e83 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -416,7 +416,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex const bool success = getItemType(path, itemType); ioError = itemType.ioError; if (!success) { - LOGW_WARN(logger(), L"Failed to check if the item is a symlink - " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); return false; } From 2b8086f3105f5b91a2d6e6076678d188adf011da Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 08:51:37 +0200 Subject: [PATCH 17/67] Update log message in case of getItemType Fail. --- src/libcommonserver/io/iohelper_mac.cpp | 3 +-- src/libcommonserver/io/iohelper_win.cpp | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index d1f7f9c6c..4e69e0a2b 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -222,8 +222,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex ItemType itemType; const bool success = getItemType(path, itemType); if (!success) { - LOGW_WARN(logger(), - L"Failed to check if the item is a symlink: " << Utility::formatIoError(path, itemType.ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); return false; } exists = itemType.ioError != IoErrorNoSuchFileOrDirectory; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 21ab6144f..516d85e83 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -416,7 +416,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex const bool success = getItemType(path, itemType); ioError = itemType.ioError; if (!success) { - LOGW_WARN(logger(), L"Failed to check if the item is a symlink - " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); return false; } From 48b783901b26662edb18a1f29c8f48d5d55e1efd Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 08:58:18 +0200 Subject: [PATCH 18/67] Unwanted Cmake files push [REVERT] --- CMakeLists.txt | 2 -- test/libcommonserver/CMakeLists.txt | 8 ++++---- test/libparms/CMakeLists.txt | 8 ++++---- test/libsyncengine/CMakeLists.txt | 8 ++++---- test/server/CMakeLists.txt | 8 ++++---- 5 files changed, 16 insertions(+), 18 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 823aeb14b..1ef67e320 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,8 +7,6 @@ IF(APPLE) SET(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build architectures for Mac OS X" FORCE) ENDIF(APPLE) -SET(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;") - set(BIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") set(BUILD_SHARED_LIBS FALSE) diff --git a/test/libcommonserver/CMakeLists.txt b/test/libcommonserver/CMakeLists.txt index d2a283fe8..84929f82d 100644 --- a/test/libcommonserver/CMakeLists.txt +++ b/test/libcommonserver/CMakeLists.txt @@ -33,8 +33,8 @@ if(WIN32) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -64,9 +64,9 @@ endif() if (WIN32) target_link_libraries(${testcommon_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testcommon_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libparms/CMakeLists.txt b/test/libparms/CMakeLists.txt index 1c26a0b79..1bf7577d5 100644 --- a/test/libparms/CMakeLists.txt +++ b/test/libparms/CMakeLists.txt @@ -18,8 +18,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -48,9 +48,9 @@ endif() if (WIN32) target_link_libraries(${testparms_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testparms_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libsyncengine/CMakeLists.txt b/test/libsyncengine/CMakeLists.txt index 8d7c69624..bae0f08df 100644 --- a/test/libsyncengine/CMakeLists.txt +++ b/test/libsyncengine/CMakeLists.txt @@ -48,8 +48,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -78,9 +78,9 @@ endif() if (WIN32) target_link_libraries(${testsyncengine_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testsyncengine_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/server/CMakeLists.txt b/test/server/CMakeLists.txt index d9b1779e8..cfa9396e6 100644 --- a/test/server/CMakeLists.txt +++ b/test/server/CMakeLists.txt @@ -15,8 +15,8 @@ if(APPLE) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -47,9 +47,9 @@ endif() if (WIN32) target_link_libraries(${testserver_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testserver_NAME} "/usr/local/lib/libcppunit.dylib") From 98f6661193cad331f65b8f8e708a4464af46cd83 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 09:14:09 +0200 Subject: [PATCH 19/67] Bug fix mac. --- src/libcommonserver/io/iohelper_mac.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index 4e69e0a2b..33c1ee002 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -222,7 +222,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex ItemType itemType; const bool success = getItemType(path, itemType); if (!success) { - LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, itemType.ioError).c_str()); return false; } exists = itemType.ioError != IoErrorNoSuchFileOrDirectory; From 65a761ef13dfafa58d7b20656c882eb163ee2c8b Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 09:20:29 +0200 Subject: [PATCH 20/67] Replace the redundant type with "auto". [SENTRY] --- src/libcommonserver/io/iohelper_win.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 516d85e83..f2b720049 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -506,7 +506,7 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ LPCWSTR pathw_c = Path2WStr(path).c_str(); size_t pathw_len = Path2WStr(path).length(); - std::unique_ptr pathw_ptr = std::make_unique(pathw_len + 1); + auto pathw_ptr = std::make_unique(pathw_len + 1); Path2WStr(path).copy(pathw_ptr.get(), pathw_len); LPWSTR pathw = pathw_ptr.get(); pathw[pathw_len] = L'\0'; From bc898cf60adc071bfd769b3f70ff8c1f9ab68fed Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 09:28:24 +0200 Subject: [PATCH 21/67] Remove useless old style array. [SENTRY] --- src/libcommonserver/io/iohelper_win.cpp | 38 ++++++++++++------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index f2b720049..4a40c138f 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -129,7 +129,8 @@ bool IoHelper::getNodeId(const SyncPath &path, NodeId &nodeId) noexcept { (PZW_QUERY_DIRECTORY_FILE)GetProcAddress(GetModuleHandle(L"ntdll.dll"), "ZwQueryDirectoryFile"); if (zwQueryDirectoryFile == 0) { - LOG_WARN(Log::instance()->getLogger(), L"Error in GetProcAddress: " << Utility::formatSyncPath(path.parent_path()).c_str()); + LOG_WARN(Log::instance()->getLogger(), + L"Error in GetProcAddress: " << Utility::formatSyncPath(path.parent_path()).c_str()); return false; } @@ -491,17 +492,17 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ PACL pACL_old = nullptr; // Current ACL PACL pACL_new = nullptr; // New ACL PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; - EXPLICIT_ACCESS ExplicitAccess[1]; - ZeroMemory(&ExplicitAccess[0], sizeof(ExplicitAccess)); - - ExplicitAccess[0].grfAccessPermissions = permission; - ExplicitAccess[0].grfAccessMode = accessMode; - ExplicitAccess[0].grfInheritance = NO_INHERITANCE; - ExplicitAccess[0].Trustee.pMultipleTrustee = nullptr; - ExplicitAccess[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; - ExplicitAccess[0].Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; - ExplicitAccess[0].Trustee.TrusteeType = Utility::_trustee.TrusteeType; - ExplicitAccess[0].Trustee.ptstrName = Utility::_trustee.ptstrName; + EXPLICIT_ACCESS ExplicitAccess; + ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); + + ExplicitAccess.grfAccessPermissions = permission; + ExplicitAccess.grfAccessMode = accessMode; + ExplicitAccess.grfInheritance = NO_INHERITANCE; + ExplicitAccess.Trustee.pMultipleTrustee = nullptr; + ExplicitAccess.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + ExplicitAccess.Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; + ExplicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; + ExplicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; LPCWSTR pathw_c = Path2WStr(path).c_str(); size_t pathw_len = Path2WStr(path).length(); @@ -518,18 +519,18 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOG_WARN(logger(), L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. return _isExpectedError(ioError); } - ValueReturned = SetEntriesInAcl(1, ExplicitAccess, pACL_old, &pACL_new); + ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOG_WARN(logger(), L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -543,15 +544,14 @@ bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_ LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - return false; + return false; } - ValueReturned = - SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); + ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOG_WARN(logger(), L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. From 080c6dd8eb73ecb99098a8d129b9495a00f46afd Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 09:33:08 +0200 Subject: [PATCH 22/67] Add comment. --- src/libcommonserver/io/iohelper.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libcommonserver/io/iohelper.cpp b/src/libcommonserver/io/iohelper.cpp index 88da61efe..5ac9785e9 100644 --- a/src/libcommonserver/io/iohelper.cpp +++ b/src/libcommonserver/io/iohelper.cpp @@ -509,6 +509,7 @@ bool IoHelper::createSymlink(const SyncPath &targetPath, const SyncPath &path, I } #ifndef _WIN32 +//See iohelper_win.cpp for the Windows implementation bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { return _setRightsUnix(path, read, write, exec, ioError); } From e5c9ae48b2c83678f82c26032721c0a0478395aa Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 10:04:56 +0200 Subject: [PATCH 23/67] Enhance readability by renamming function and reducing iohelper.h --- src/libcommonserver/io/iohelper.cpp | 4 +- src/libcommonserver/io/iohelper.h | 7 +- src/libcommonserver/io/iohelper_win.cpp | 187 ++++++++++++------------ 3 files changed, 100 insertions(+), 98 deletions(-) diff --git a/src/libcommonserver/io/iohelper.cpp b/src/libcommonserver/io/iohelper.cpp index 5ac9785e9..124bd6c08 100644 --- a/src/libcommonserver/io/iohelper.cpp +++ b/src/libcommonserver/io/iohelper.cpp @@ -511,11 +511,11 @@ bool IoHelper::createSymlink(const SyncPath &targetPath, const SyncPath &path, I #ifndef _WIN32 //See iohelper_win.cpp for the Windows implementation bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - return _setRightsUnix(path, read, write, exec, ioError); + return _setRightsStandart(path, read, write, exec, ioError); } #endif -bool IoHelper::_setRightsUnix(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { +bool IoHelper::_setRightsStandart(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { ioError = IoErrorSuccess; std::filesystem::perms perms = std::filesystem::perms::none; if (read) { diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index fd95a646a..e5ec29208 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -306,12 +306,7 @@ struct IoHelper { static bool _setTargetType(ItemType &itemType) noexcept; static bool _checkIfIsHiddenFile(const SyncPath &path, bool &isHidden, IoError &ioError) noexcept; - static bool _setRightsUnix(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; - -#ifdef _WIN32 - static bool _setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, - IoError &ioError) noexcept; -#endif + static bool _setRightsStandart(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; }; } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 4a40c138f..6ec87da13 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -330,6 +330,87 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return IoHelper::getXAttrValue(itemPath.native(), FILE_ATTRIBUTE_OFFLINE, isDehydrated, ioError); } + +static bool setRightsApiWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, + log4cplus::Logger logger) noexcept { // Always return false if ioError is not IoErrorSuccess, + // caller should check _isExpectedError(ioError) + PACL pACL_old = nullptr; // Current ACL + PACL pACL_new = nullptr; // New ACL + PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; + EXPLICIT_ACCESS ExplicitAccess; + ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); + + ExplicitAccess.grfAccessPermissions = permission; + ExplicitAccess.grfAccessMode = accessMode; + ExplicitAccess.grfInheritance = NO_INHERITANCE; + ExplicitAccess.Trustee.pMultipleTrustee = nullptr; + ExplicitAccess.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + ExplicitAccess.Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; + ExplicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; + ExplicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; + + LPCWSTR pathw_c = Path2WStr(path).c_str(); + size_t pathw_len = Path2WStr(path).length(); + + auto pathw_ptr = std::make_unique(pathw_len + 1); + Path2WStr(path).copy(pathw_ptr.get(), pathw_len); + LPWSTR pathw = pathw_ptr.get(); + pathw[pathw_len] = L'\0'; + + + DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, + nullptr, &pSecurityDescriptor); + + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return false; + } + + ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return false; + } + + if (!IsValidAcl(pACL_new)) { + ioError = IoErrorUnknown; + LOG_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); + + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return false; + } + + ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); + if (ValueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(ValueReturned); + LOG_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" + << IoHelper::ioError2StdString(ioError).c_str()); + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + return false; + } + + + LocalFree(pSecurityDescriptor); + LocalFree(pACL_new); + // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + + return true; +} + bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { read = false; write = false; @@ -386,13 +467,16 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) if (!readCtrl) { - _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError); // Force read control - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - LocalFree(psecDesc); - GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - _setRightsWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, - ioError); // Revoke read control after reading the permissions - readCtrl = (rights & READ_CONTROL) == READ_CONTROL; + if (setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, + logger())) { // Try to force read control + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, + &psecDesc); + LocalFree(psecDesc); + GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, + logger()); // Revoke read control after reading the permissions + readCtrl = (rights & READ_CONTROL) == READ_CONTROL; + } if (!readCtrl) { ioError = IoErrorAccessDenied; LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); @@ -452,6 +536,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } + bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { if (Utility::_trustee.ptstrName) { ioError = IoErrorSuccess; @@ -477,93 +562,15 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } bool res = false; - res = _setRightsWindows(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError); + res = setRightsApiWindows(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()); if (!res) { - return false; + return _isExpectedError(ioError); } - res = _setRightsWindows(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError); - return res; - } else { - return _setRightsUnix(path, read, write, exec, ioError); - } -} - -bool IoHelper::_setRightsWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError) noexcept { - PACL pACL_old = nullptr; // Current ACL - PACL pACL_new = nullptr; // New ACL - PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; - EXPLICIT_ACCESS ExplicitAccess; - ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); - - ExplicitAccess.grfAccessPermissions = permission; - ExplicitAccess.grfAccessMode = accessMode; - ExplicitAccess.grfInheritance = NO_INHERITANCE; - ExplicitAccess.Trustee.pMultipleTrustee = nullptr; - ExplicitAccess.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; - ExplicitAccess.Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; - ExplicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; - ExplicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; - - LPCWSTR pathw_c = Path2WStr(path).c_str(); - size_t pathw_len = Path2WStr(path).length(); - - auto pathw_ptr = std::make_unique(pathw_len + 1); - Path2WStr(path).copy(pathw_ptr.get(), pathw_len); - LPWSTR pathw = pathw_ptr.get(); - pathw[pathw_len] = L'\0'; - - - DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, - nullptr, &pSecurityDescriptor); - - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - return _isExpectedError(ioError); - } - - ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - return _isExpectedError(ioError); - } - - if (!IsValidAcl(pACL_new)) { - ioError = IoErrorUnknown; - LOG_WARN(logger(), L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); - - LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - return false; - } - - ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger(), L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); - LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + res = setRightsApiWindows(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); return _isExpectedError(ioError); + } else { + return _setRightsStandart(path, read, write, exec, ioError); } - - - LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. - - return true; } bool IoHelper::checkIfIsJunction(const SyncPath &path, bool &isJunction, IoError &ioError) noexcept { From 0b9035375852429796be1c61910a5ae1ef6e9040 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 13:30:36 +0200 Subject: [PATCH 24/67] Correct some problems revealed by the test implementation. --- src/libcommonserver/io/iohelper_win.cpp | 34 +++++++++++++++++++------ 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 6ec87da13..1dbcf9bcf 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -330,7 +330,6 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return IoHelper::getXAttrValue(itemPath.native(), FILE_ATTRIBUTE_OFFLINE, isDehydrated, ioError); } - static bool setRightsApiWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, log4cplus::Logger logger) noexcept { // Always return false if ioError is not IoErrorSuccess, // caller should check _isExpectedError(ioError) @@ -458,9 +457,22 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex write = false; exec = false; LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + return _isExpectedError(ioError); + } + + if (result == ERROR_INVALID_SID) { // Access denied, try to force read control + setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, + &psecDesc); + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); + ioError = dWordError2ioError(result); + + if (result != ERROR_SUCCESS) { + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + return _isExpectedError(ioError); + } } - LOGW_WARN(logger(), L"Error in GetEffectiveRightsFromAcl - path=" << szFilePath << L" result=" << result); - return _isExpectedError(ioError); } bool readCtrl = @@ -487,7 +499,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex exists = true; read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; - exec = (rights & FILE_GENERIC_EXECUTE) == FILE_GENERIC_EXECUTE; + exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; return true; @@ -544,19 +556,19 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, DWORD deniedPermission = 0; if (!read) { - deniedPermission |= FILE_GENERIC_READ & ~READ_CONTROL & ~SYNCHRONIZE; + deniedPermission |= (FILE_GENERIC_READ & ~READ_CONTROL & ~SYNCHRONIZE); } else { grantedPermission |= FILE_GENERIC_READ; } if (!write) { - deniedPermission |= FILE_GENERIC_WRITE & ~READ_CONTROL & ~SYNCHRONIZE; + deniedPermission |= (FILE_GENERIC_WRITE & ~READ_CONTROL & ~SYNCHRONIZE); } else { grantedPermission |= FILE_GENERIC_WRITE; } if (!exec) { - deniedPermission |= FILE_GENERIC_EXECUTE & ~READ_CONTROL & ~SYNCHRONIZE & ~FILE_READ_ATTRIBUTES; + deniedPermission |= (FILE_GENERIC_EXECUTE & ~READ_CONTROL & ~SYNCHRONIZE & ~FILE_READ_ATTRIBUTES); } else { grantedPermission |= FILE_GENERIC_EXECUTE; } @@ -567,7 +579,13 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, return _isExpectedError(ioError); } res = setRightsApiWindows(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); - return _isExpectedError(ioError); + + if (!res) { + return _isExpectedError(ioError); + } + + return true; + } else { return _setRightsStandart(path, read, write, exec, ioError); } From d459a4ee1172be98b61ba42a1b8d85914f19a2ee Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 13:52:05 +0200 Subject: [PATCH 25/67] Test implementation --- test/libcommonserver/CMakeLists.txt | 2 +- .../io/testchecksetgetrights.cpp | 286 ++++++++++++++++++ test/libcommonserver/io/testio.h | 2 + 3 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 test/libcommonserver/io/testchecksetgetrights.cpp diff --git a/test/libcommonserver/CMakeLists.txt b/test/libcommonserver/CMakeLists.txt index 84929f82d..85d165039 100644 --- a/test/libcommonserver/CMakeLists.txt +++ b/test/libcommonserver/CMakeLists.txt @@ -17,7 +17,7 @@ set(testcommon_SRCS db/testdb.h db/testdb.cpp # io io/testio.h io/testio.cpp io/testgetitemtype.cpp io/testgetfilesize.cpp io/testcheckifpathexists.cpp io/testgetnodeid.cpp io/testgetfilestat.cpp io/testisfileaccessible.cpp io/testfilechanged.cpp - io/testcheckifisdirectory.cpp io/testcreatesymlink.cpp io/testcheckifdehydrated.cpp + io/testcheckifisdirectory.cpp io/testcreatesymlink.cpp io/testcheckifdehydrated.cpp io/testchecksetgetrights.cpp ) if (USE_OUR_OWN_SQLITE3) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp new file mode 100644 index 000000000..1e01af576 --- /dev/null +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -0,0 +1,286 @@ +/* + * Infomaniak kDrive - Desktop + * Copyright (C) 2023-2024 Infomaniak Network SA + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include "testio.h" +#include "libcommonserver/utility/utility.h" + +#include + +using namespace CppUnit; + +namespace KDC { + +static struct rightsSet { + bool read; + bool write; + bool execute; +}; + +static void rightsSetFromInt(int rights, rightsSet& rightsSet) { + rightsSet.read = rights & 4; + rightsSet.write = rights & 2; + rightsSet.execute = rights & 1; +} + +static int rightsSetToInt(rightsSet rightsSet) { + int rights = 0; + if (rightsSet.read) { + rights += 4; + } + if (rightsSet.write) { + rights += 2; + } + if (rightsSet.execute) { + rights += 1; + } + + return rights; +} + +void TestIo::testCheckSetAndGetRights() { + #ifdef _WIN32 + CPPUNIT_ASSERT(Utility::init()); // Initialize the utility library, needed to access/change the permissions on Windows + #endif + + const TemporaryDirectory temporaryDirectory; + + // Test if the rights are correctly set and get on a directory + { + const SyncPath path = temporaryDirectory.path / "changePerm"; + + IoError ioError = IoErrorUnknown; + CPPUNIT_ASSERT(IoHelper::createDirectory(path, ioError)); + CPPUNIT_ASSERT(ioError == IoErrorSuccess); + + bool isReadable = false; + bool isWritable = false; + bool isExecutable = false; + bool exists = false; + + rightsSet rightsSet = {false, false, false}; + + // For a directory + + /* Test all the possible rights and the all the possible order of rights modification. ie: + * | READ | WRITE | EXECUTE | | + * | 0 | 0 | 0 | v + * | 0 | 0 | 1 | | + * | 0 | 0 | 0 | v + * | 0 | 1 | 0 | | + * | 0 | 0 | 0 | v + * | 0 | 1 | 1 | | + * | 0 | 0 | 0 | v + * | 1 | 0 | 0 | | + * | 0 | 0 | 0 | v + * | 1 | 0 | 1 | | + * | 0 | 0 | 0 | v + * | 1 | 1 | 0 | | + * | 0 | 0 | 0 | v + * | 1 | 1 | 1 | | + * | 0 | 0 | 0 | v + * ... + */ + + for (int baseRigths = 0; baseRigths < 7; + baseRigths++) { // Test all the possible rights and the all the possible order of rights modification + for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { + rightsSetFromInt(baseRigths, rightsSet); + + bool result = IoHelper::setRights(path, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); + result &= ioError == IoErrorSuccess; + if (!result) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to set base rights */); + } + + result = IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists); + if (!result) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to get base rights */); + } + + if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && + isExecutable == rightsSet.execute)) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); + } + + rightsSetFromInt(targetRigths, rightsSet); + + result = IoHelper::setRights(path, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); + result &= ioError == IoErrorSuccess; + if (!result) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to set target rights */); + } + + result = IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists); + if (!result) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to get target rights */); + } + + if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && + isExecutable == rightsSet.execute)) { + IoHelper::setRights(path, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Set target rights mismatch with get target rights */); + } + } + } + // Restore the rights + IoHelper::setRights(path, true, true, true, ioError); + } + + // Test if the rights are correctly set and get on a file + { + const SyncPath filepath = temporaryDirectory.path / "changePerm.txt"; + + IoError ioError = IoErrorUnknown; + + std::ofstream file(filepath.string()); + CPPUNIT_ASSERT(file.is_open()); + file << "testCheckSetAndGetRights"; + file.close(); + + bool isReadable = false; + bool isWritable = false; + bool isExecutable = false; + bool exists = false; + + rightsSet rightsSet = {false, false, false}; + + // For a directory + for (int baseRigths = 0; baseRigths < 7; + baseRigths++) { // Test all the possible rights and the all the possible order of rights modification + for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { + rightsSetFromInt(baseRigths, rightsSet); + + bool result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); + result &= ioError == IoErrorSuccess; + if (!result) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to set base rights */); + } + + result = IoHelper::getRights(filepath, isReadable, isWritable, isExecutable, exists); + if (!result) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to get base rights */); + } + + if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && + isExecutable == rightsSet.execute)) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); + } + + rightsSetFromInt(targetRigths, rightsSet); + + result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); + result &= ioError == IoErrorSuccess; + if (!result) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to set target rights */); + } + + result = IoHelper::getRights(filepath, isReadable, isWritable, isExecutable, exists); + if (!result) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Failed to get target rights */); + } + + if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && + isExecutable == rightsSet.execute)) { + IoHelper::setRights(filepath, true, true, true, ioError); + CPPUNIT_ASSERT(false /* Set target rights mismatch with get target rights */); + } + } + } + + // Restore the rights + IoHelper::setRights(filepath, true, true, true, ioError); + } + + // Check permission is not recursive on folder + { + const SyncPath path = temporaryDirectory.path / "testCheckSetAndGetRights"; + const SyncPath subFolderPath = path / "subFolder"; + const SyncPath subFilePath = path / "subFile.txt"; + + + IoError ioError = IoErrorUnknown; + CPPUNIT_ASSERT(IoHelper::createDirectory(path, ioError)); + CPPUNIT_ASSERT(ioError == IoErrorSuccess); + + CPPUNIT_ASSERT(IoHelper::createDirectory(subFolderPath, ioError)); + CPPUNIT_ASSERT(ioError == IoErrorSuccess); + + std::ofstream file(subFilePath.string()); + CPPUNIT_ASSERT(file.is_open()); + file << "testCheckSetAndGetRights"; + file.close(); + + + bool isReadable = false; + bool isWritable = false; + bool isExecutable = false; + bool exists = false; + + CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFilePath, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + + bool result = IoHelper::setRights(path, true, false, true, ioError); + result &= ioError == IoErrorSuccess; + if (!result) { + IoHelper::setRights(path, true, true, true, ioError); // Restore the rights for delete + CPPUNIT_ASSERT(false /* Failed to set base rights */); + } + + + CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && !isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFilePath, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + + // Restore the rights + IoHelper::setRights(path, true, true, true, ioError); // Restore the rights for delete + } + + // Test on a non existing file + { + const SyncPath path = temporaryDirectory.path / "testCheckSetAndGetRights/nonExistingFile.txt"; + bool isReadable = false; + bool isWritable = false; + bool isExecutable = false; + bool exists = false; + IoError ioError = IoErrorUnknown; + + CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); + CPPUNIT_ASSERT(!exists); + + CPPUNIT_ASSERT(IoHelper::setRights(path, true, true, true, ioError)); + CPPUNIT_ASSERT(ioError == IoErrorNoSuchFileOrDirectory); + } +} +} // namespace KDC diff --git a/test/libcommonserver/io/testio.h b/test/libcommonserver/io/testio.h index abec3cdb0..93e3846bd 100644 --- a/test/libcommonserver/io/testio.h +++ b/test/libcommonserver/io/testio.h @@ -73,6 +73,7 @@ class TestIo : public CppUnit::TestFixture { CPPUNIT_TEST(testCreateJunction); #endif CPPUNIT_TEST(testCheckIfFileIsDehydrated); + CPPUNIT_TEST(testCheckSetAndGetRights); CPPUNIT_TEST_SUITE_END(); public: @@ -105,6 +106,7 @@ class TestIo : public CppUnit::TestFixture { void testCreateJunction(void); #endif void testCheckIfFileIsDehydrated(void); + void testCheckSetAndGetRights(void); private: void testGetItemTypeSimpleCases(void); From 796d1a3aaec4a150d6ce1a3b477f8e761dec97c0 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 15:26:32 +0200 Subject: [PATCH 26/67] Add verbose to test to help debug. --- test/libcommonserver/io/testchecksetgetrights.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index 1e01af576..1526ecbb6 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -117,6 +117,8 @@ void TestIo::testCheckSetAndGetRights() { if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && isExecutable == rightsSet.execute)) { IoHelper::setRights(path, true, true, true, ioError); + std::cout << "setted rigths (RWX): " << rightsSet.read << rightsSet.write << rightsSet.execute + << " | readed rigths(RWX): " << isReadable << isWritable << isExecutable << std::endl; CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); } @@ -138,6 +140,8 @@ void TestIo::testCheckSetAndGetRights() { if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && isExecutable == rightsSet.execute)) { IoHelper::setRights(path, true, true, true, ioError); + std::cout << "setted rigths (RWX): " << rightsSet.read << rightsSet.write << rightsSet.execute + << " | readed rigths(RWX): " << isReadable << isWritable << isExecutable << std::endl; CPPUNIT_ASSERT(false /* Set target rights mismatch with get target rights */); } } From 7207973fb431baaa806b081bd98e9693f5c4d578 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 15:28:30 +0200 Subject: [PATCH 27/67] Force rigths to predefined state for linux and mac when testing. --- test/libcommonserver/io/testchecksetgetrights.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index 1526ecbb6..c9260df90 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -245,6 +245,7 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; + bool result = IoHelper::setRights(path, true, true , true, ioError); CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); @@ -252,7 +253,7 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(IoHelper::getRights(subFilePath, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); - bool result = IoHelper::setRights(path, true, false, true, ioError); + result = IoHelper::setRights(path, true, false, true, ioError); result &= ioError == IoErrorSuccess; if (!result) { IoHelper::setRights(path, true, true, true, ioError); // Restore the rights for delete From 306bb4e9ce111c8d89fa686ac0d7f2214b9074ab Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 15:43:03 +0200 Subject: [PATCH 28/67] Add verbose to help debug. --- src/libcommonserver/io/iohelper_win.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 1dbcf9bcf..af5a0fb4a 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -42,6 +42,7 @@ #include #define SECURITY_WIN32 #include +#include namespace KDC { @@ -587,6 +588,7 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, return true; } else { + std::cout << "setRights: No trustee set" << std::endl; return _setRightsStandart(path, read, write, exec, ioError); } } From 8687a1f01bdd7c790b3c6e9e557d7364e5b00ca0 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 15:43:26 +0200 Subject: [PATCH 29/67] Fix mac and inux tests. --- test/libcommonserver/io/testchecksetgetrights.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index c9260df90..6b89504ac 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -246,10 +246,15 @@ void TestIo::testCheckSetAndGetRights() { bool exists = false; bool result = IoHelper::setRights(path, true, true , true, ioError); + bool result = IoHelper::setRights(subFolderPath, true, true, true, ioError); + bool result = IoHelper::setRights(subFilePath, true, true, true, ioError); + CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFilePath, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); @@ -260,7 +265,6 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(false /* Failed to set base rights */); } - CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && !isWritable && isExecutable); CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); From 986c6fb1a0a86e8f8fd697122f8d540c5c6ed947 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 15:43:26 +0200 Subject: [PATCH 30/67] Fix mac and linux tests. --- test/libcommonserver/io/testchecksetgetrights.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index c9260df90..ae7537930 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -245,11 +245,16 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; - bool result = IoHelper::setRights(path, true, true , true, ioError); + bool result = IoHelper::setRights(path, true, true, true, ioError); + result = IoHelper::setRights(subFolderPath, true, true, true, ioError); + result = IoHelper::setRights(subFilePath, true, true, true, ioError); + CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); + CPPUNIT_ASSERT(IoHelper::getRights(subFilePath, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && isWritable && isExecutable); @@ -260,7 +265,6 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(false /* Failed to set base rights */); } - CPPUNIT_ASSERT(IoHelper::getRights(path, isReadable, isWritable, isExecutable, exists)); CPPUNIT_ASSERT(exists && isReadable && !isWritable && isExecutable); CPPUNIT_ASSERT(IoHelper::getRights(subFolderPath, isReadable, isWritable, isExecutable, exists)); From 350c4099ce16d8be0aa1d9d99d4660c1a0a76319 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 16:00:42 +0200 Subject: [PATCH 31/67] Add verbose to debug. --- src/libcommonserver/utility/utility_win.cpp | 9 ++++ .../io/testchecksetgetrights.cpp | 45 ++++++++++--------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index 6915a7284..210a47b23 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -47,6 +47,7 @@ namespace KDC { static bool initTrusteeWithUserSID() { if (Utility::_psid != NULL) { + std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; LocalFree(Utility::_psid); Utility::_psid = NULL; } @@ -65,10 +66,12 @@ static bool initTrusteeWithUserSID() { dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in GetUserNameExW - err=" << dwError); free(szUserName); + std::cout << "TestIo::testCheckSetAndGetRights: Error in GetUserNameExW - err=" << dwError << std::endl; return false; } } else { LOGW_WARN(Log::instance()->getLogger(), "Error in GetUserNameExW - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in GetUserNameExW - err=" << dwError << std::endl; return false; } @@ -85,6 +88,7 @@ static bool initTrusteeWithUserSID() { LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW - err=" << dwError); free(szUserName); Utility::_psid = NULL; + std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW - err=" << dwError << std::endl; return false; } } @@ -94,6 +98,7 @@ static bool initTrusteeWithUserSID() { if (Utility::_psid == NULL) { LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); free(szUserName); + std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } @@ -103,6 +108,7 @@ static bool initTrusteeWithUserSID() { free(szUserName); LocalFree(Utility::_psid); Utility::_psid = NULL; + std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } @@ -113,6 +119,7 @@ static bool initTrusteeWithUserSID() { LocalFree(Utility::_psid); Utility::_psid = NULL; LocalFree(pdomain); + std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW - err=" << dwError << std::endl; return false; } @@ -123,11 +130,13 @@ static bool initTrusteeWithUserSID() { BuildTrusteeWithSidW(&Utility::_trustee, Utility::_psid); // NB: Don't free _psid as it is referenced in _trustee + std::cout << "TestIo::testCheckSetAndGetRights: trustee set" << std::endl; return true; } static bool init_private() { if (Utility::_psid != NULL) { + std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; return false; } diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index ae7537930..f3866a3d5 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -53,9 +53,12 @@ static int rightsSetToInt(rightsSet rightsSet) { } void TestIo::testCheckSetAndGetRights() { - #ifdef _WIN32 - CPPUNIT_ASSERT(Utility::init()); // Initialize the utility library, needed to access/change the permissions on Windows - #endif + std::cout << "TestIo::testCheckSetAndGetRights: start" << std::endl; + +#ifdef _WIN32 + std::cout << "TestIo::testCheckSetAndGetRights: init trustee" << std::endl; + CPPUNIT_ASSERT(Utility::init()); // Initialize the utility library, needed to access/change the permissions on Windows +#endif const TemporaryDirectory temporaryDirectory; @@ -77,24 +80,24 @@ void TestIo::testCheckSetAndGetRights() { // For a directory /* Test all the possible rights and the all the possible order of rights modification. ie: - * | READ | WRITE | EXECUTE | | - * | 0 | 0 | 0 | v - * | 0 | 0 | 1 | | - * | 0 | 0 | 0 | v - * | 0 | 1 | 0 | | - * | 0 | 0 | 0 | v - * | 0 | 1 | 1 | | - * | 0 | 0 | 0 | v - * | 1 | 0 | 0 | | - * | 0 | 0 | 0 | v - * | 1 | 0 | 1 | | - * | 0 | 0 | 0 | v - * | 1 | 1 | 0 | | - * | 0 | 0 | 0 | v - * | 1 | 1 | 1 | | - * | 0 | 0 | 0 | v - * ... - */ + * | READ | WRITE | EXECUTE | | + * | 0 | 0 | 0 | v + * | 0 | 0 | 1 | | + * | 0 | 0 | 0 | v + * | 0 | 1 | 0 | | + * | 0 | 0 | 0 | v + * | 0 | 1 | 1 | | + * | 0 | 0 | 0 | v + * | 1 | 0 | 0 | | + * | 0 | 0 | 0 | v + * | 1 | 0 | 1 | | + * | 0 | 0 | 0 | v + * | 1 | 1 | 0 | | + * | 0 | 0 | 0 | v + * | 1 | 1 | 1 | | + * | 0 | 0 | 0 | v + * ... + */ for (int baseRigths = 0; baseRigths < 7; baseRigths++) { // Test all the possible rights and the all the possible order of rights modification From 60be97dc602ef4d6d09a91283bcb07a256f0feda Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 16:18:55 +0200 Subject: [PATCH 32/67] Add verbose for debug. --- src/libcommonserver/utility/utility_win.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index 210a47b23..9316783c8 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -85,10 +85,12 @@ static bool initTrusteeWithUserSID() { if (dwError == ERROR_INSUFFICIENT_BUFFER) { // Normal case as Utility::_psid is NULL } else { - LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW - err=" << dwError); - free(szUserName); + LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW 1 - err=" << dwError); Utility::_psid = NULL; - std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW - err=" << dwError << std::endl; + std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW 1 - err=" << dwError << " - account name=" + << Utility::ws2s(szUserName) << std::endl; + free(szUserName); + return false; } } @@ -114,12 +116,14 @@ static bool initTrusteeWithUserSID() { if (!LookupAccountNameW(NULL, szUserName, Utility::_psid, &sidsize, pdomain, &dlen, &stype)) { WORD dwError = GetLastError(); - LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW - err=" << dwError); - free(szUserName); + LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW 2 - err=" << dwError); LocalFree(Utility::_psid); Utility::_psid = NULL; LocalFree(pdomain); - std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW - err=" << dwError << std::endl; + std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW 2 - err=" << dwError + << " - account name=" << Utility::ws2s(szUserName) << std::endl; + free(szUserName); + return false; } From a0bb606e22dbf6b2c9d3621594eca154c9f6ae6b Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 16:24:38 +0200 Subject: [PATCH 33/67] Replace the 'W' function by auto select between ANSI or unicode function for win api. --- src/libcommonserver/utility/utility_win.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index 9316783c8..4026edb58 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -57,12 +57,12 @@ static bool initTrusteeWithUserSID() { // Get user name DWORD userNameSize = 0; WCHAR *szUserName = NULL; - GetUserNameExW(NameSamCompatible, szUserName, &userNameSize); + GetUserNameEx(NameSamCompatible, szUserName, &userNameSize); WORD dwError = GetLastError(); if (dwError == ERROR_MORE_DATA) { // Normal case as szUserName is NULL szUserName = (WCHAR *)malloc(userNameSize * sizeof(WCHAR)); - if (!GetUserNameExW(NameSamCompatible, szUserName, &userNameSize)) { + if (!GetUserNameEx(NameSamCompatible, szUserName, &userNameSize)) { dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in GetUserNameExW - err=" << dwError); free(szUserName); @@ -80,7 +80,7 @@ static bool initTrusteeWithUserSID() { DWORD sidsize = 0; DWORD dlen = 0; SID_NAME_USE stype; - if (!LookupAccountNameW(NULL, szUserName, Utility::_psid, &sidsize, NULL, &dlen, &stype)) { + if (!LookupAccountName(NULL, szUserName, Utility::_psid, &sidsize, NULL, &dlen, &stype)) { dwError = GetLastError(); if (dwError == ERROR_INSUFFICIENT_BUFFER) { // Normal case as Utility::_psid is NULL @@ -114,7 +114,7 @@ static bool initTrusteeWithUserSID() { return false; } - if (!LookupAccountNameW(NULL, szUserName, Utility::_psid, &sidsize, pdomain, &dlen, &stype)) { + if (!LookupAccountName(NULL, szUserName, Utility::_psid, &sidsize, pdomain, &dlen, &stype)) { WORD dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW 2 - err=" << dwError); LocalFree(Utility::_psid); @@ -131,7 +131,7 @@ static bool initTrusteeWithUserSID() { LocalFree(pdomain); // Build trustee structure - BuildTrusteeWithSidW(&Utility::_trustee, Utility::_psid); + BuildTrusteeWithSid(&Utility::_trustee, Utility::_psid); // NB: Don't free _psid as it is referenced in _trustee std::cout << "TestIo::testCheckSetAndGetRights: trustee set" << std::endl; From 14576643ac84ea2cf40bd4a299aafde1efb37949 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 17:19:10 +0200 Subject: [PATCH 34/67] Replace the way we fetch the user SID, the old version wasn't working for domain or work group (ie: the CI). --- src/libcommonserver/utility/utility_win.cpp | 121 +++++++++----------- 1 file changed, 55 insertions(+), 66 deletions(-) diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index 4026edb58..62dc92524 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -46,100 +46,89 @@ namespace KDC { static bool initTrusteeWithUserSID() { - if (Utility::_psid != NULL) { + if (Utility::_psid != nullptr) { // should never happen + delete[] Utility::_psid; + Utility::_psid = nullptr; + + LOGW_WARN(Log::instance()->getLogger(), "Error in initTrusteeWithUserSID - _pssid is not null"); std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; - LocalFree(Utility::_psid); - Utility::_psid = NULL; } - Utility::_trustee = {0}; - // Get user name - DWORD userNameSize = 0; - WCHAR *szUserName = NULL; - GetUserNameEx(NameSamCompatible, szUserName, &userNameSize); - WORD dwError = GetLastError(); - if (dwError == ERROR_MORE_DATA) { - // Normal case as szUserName is NULL - szUserName = (WCHAR *)malloc(userNameSize * sizeof(WCHAR)); - if (!GetUserNameEx(NameSamCompatible, szUserName, &userNameSize)) { - dwError = GetLastError(); - LOGW_WARN(Log::instance()->getLogger(), "Error in GetUserNameExW - err=" << dwError); - free(szUserName); - std::cout << "TestIo::testCheckSetAndGetRights: Error in GetUserNameExW - err=" << dwError << std::endl; - return false; - } - } else { - LOGW_WARN(Log::instance()->getLogger(), "Error in GetUserNameExW - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in GetUserNameExW - err=" << dwError << std::endl; - return false; - } + // Get SID associated with the current process + auto hToken_std = std::make_unique(); + PHANDLE hToken = hToken_std.get(); + if (!OpenThreadToken(GetCurrentThread(), TOKEN_QUERY, TRUE, hToken)) { + DWORD dwError = GetLastError(); + if (dwError == ERROR_NO_TOKEN) { + if (!ImpersonateSelf(SecurityImpersonation)) { + dwError = GetLastError(); + LOGW_WARN(Log::instance()->getLogger(), "Error in ImpersonateSelf - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in ImpersonateSelf - err=" << dwError << std::endl; + return false; + } - // Get size of SID and domain - Utility::_psid = NULL; - DWORD sidsize = 0; - DWORD dlen = 0; - SID_NAME_USE stype; - if (!LookupAccountName(NULL, szUserName, Utility::_psid, &sidsize, NULL, &dlen, &stype)) { - dwError = GetLastError(); - if (dwError == ERROR_INSUFFICIENT_BUFFER) { - // Normal case as Utility::_psid is NULL + if (!OpenThreadToken(GetCurrentThread(), TOKEN_QUERY, TRUE, hToken)) { + dwError = GetLastError(); + LOGW_WARN(Log::instance()->getLogger(), "Error in OpenThreadToken - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in OpenThreadToken 1 - err=" << dwError << std::endl; + return false; + } } else { - LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW 1 - err=" << dwError); - Utility::_psid = NULL; - std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW 1 - err=" << dwError << " - account name=" - << Utility::ws2s(szUserName) << std::endl; - free(szUserName); - + LOGW_WARN(Log::instance()->getLogger(), "Error in OpenThreadToken - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in OpenThreadToken 2 - err=" << dwError << std::endl; return false; } } - // Get SID and domain - Utility::_psid = (PSID)LocalAlloc(0, sidsize); - if (Utility::_psid == NULL) { + DWORD dwLength = 0; + GetTokenInformation(*hToken, TokenUser, nullptr, 0, &dwLength); + if (dwLength == 0) { + DWORD dwError = GetLastError(); + LOGW_WARN(Log::instance()->getLogger(), "Error in GetTokenInformation 1 - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in GetTokenInformation 1 - err=" << dwError << std::endl; + return false; + } + auto pTokenUser_std = std::make_unique(dwLength); + PTOKEN_USER pTokenUser = pTokenUser_std.get(); + if (pTokenUser == nullptr) { LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); - free(szUserName); std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } - LPTSTR pdomain = (LPTSTR)LocalAlloc(0, dlen * sizeof(WCHAR)); - if (pdomain == NULL) { + if (!GetTokenInformation(*hToken, TokenUser, pTokenUser, dwLength, &dwLength)) { + DWORD dwError = GetLastError(); + LOGW_WARN(Log::instance()->getLogger(), "Error in GetTokenInformation 2 - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in GetTokenInformation 2 - err=" << dwError << std::endl; + return false; + } + + Utility::_psid = new BYTE[GetLengthSid(pTokenUser->User.Sid)]; + if (Utility::_psid == nullptr) { LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); - free(szUserName); - LocalFree(Utility::_psid); - Utility::_psid = NULL; std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } - if (!LookupAccountName(NULL, szUserName, Utility::_psid, &sidsize, pdomain, &dlen, &stype)) { - WORD dwError = GetLastError(); - LOGW_WARN(Log::instance()->getLogger(), "Error in LookupAccountNameW 2 - err=" << dwError); - LocalFree(Utility::_psid); - Utility::_psid = NULL; - LocalFree(pdomain); - std::cout << "TestIo::testCheckSetAndGetRights: Error in LookupAccountNameW 2 - err=" << dwError - << " - account name=" << Utility::ws2s(szUserName) << std::endl; - free(szUserName); - + if (!CopySid(GetLengthSid(pTokenUser->User.Sid), Utility::_psid, pTokenUser->User.Sid)) { + DWORD dwError = GetLastError(); + LOGW_WARN(Log::instance()->getLogger(), "Error in CopySid - err=" << dwError); + std::cout << "TestIo::testCheckSetAndGetRights: Error in CopySid - err=" << dwError << std::endl; + delete[] Utility::_psid; + Utility::_psid = nullptr; return false; } - free(szUserName); - LocalFree(pdomain); - - // Build trustee structure + // initialize the trustee structure BuildTrusteeWithSid(&Utility::_trustee, Utility::_psid); - // NB: Don't free _psid as it is referenced in _trustee std::cout << "TestIo::testCheckSetAndGetRights: trustee set" << std::endl; return true; } static bool init_private() { - if (Utility::_psid != NULL) { + if (Utility::_psid != nullptr) { std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; return false; } @@ -150,8 +139,8 @@ static bool init_private() { } static void free_private() { - if (Utility::_psid != NULL) { - LocalFree(Utility::_psid); + if (Utility::_psid != nullptr) { + delete[] Utility::_psid; Utility::_psid = NULL; } } From 283fd9dcf594bf26eaa67ef3cd93bf2ae1dc784c Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 17:34:49 +0200 Subject: [PATCH 35/67] Remove all the verbose. --- src/libcommonserver/io/iohelper_win.cpp | 1 - src/libcommonserver/utility/utility_win.cpp | 11 ----------- 2 files changed, 12 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index af5a0fb4a..ef65feab7 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -588,7 +588,6 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, return true; } else { - std::cout << "setRights: No trustee set" << std::endl; return _setRightsStandart(path, read, write, exec, ioError); } } diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index 62dc92524..a3be81cc7 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -51,7 +51,6 @@ static bool initTrusteeWithUserSID() { Utility::_psid = nullptr; LOGW_WARN(Log::instance()->getLogger(), "Error in initTrusteeWithUserSID - _pssid is not null"); - std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; } Utility::_trustee = {0}; @@ -64,19 +63,16 @@ static bool initTrusteeWithUserSID() { if (!ImpersonateSelf(SecurityImpersonation)) { dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in ImpersonateSelf - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in ImpersonateSelf - err=" << dwError << std::endl; return false; } if (!OpenThreadToken(GetCurrentThread(), TOKEN_QUERY, TRUE, hToken)) { dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in OpenThreadToken - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in OpenThreadToken 1 - err=" << dwError << std::endl; return false; } } else { LOGW_WARN(Log::instance()->getLogger(), "Error in OpenThreadToken - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in OpenThreadToken 2 - err=" << dwError << std::endl; return false; } } @@ -86,35 +82,30 @@ static bool initTrusteeWithUserSID() { if (dwLength == 0) { DWORD dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in GetTokenInformation 1 - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in GetTokenInformation 1 - err=" << dwError << std::endl; return false; } auto pTokenUser_std = std::make_unique(dwLength); PTOKEN_USER pTokenUser = pTokenUser_std.get(); if (pTokenUser == nullptr) { LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); - std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } if (!GetTokenInformation(*hToken, TokenUser, pTokenUser, dwLength, &dwLength)) { DWORD dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in GetTokenInformation 2 - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in GetTokenInformation 2 - err=" << dwError << std::endl; return false; } Utility::_psid = new BYTE[GetLengthSid(pTokenUser->User.Sid)]; if (Utility::_psid == nullptr) { LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); - std::cout << "TestIo::testCheckSetAndGetRights: Memory allocation error" << std::endl; return false; } if (!CopySid(GetLengthSid(pTokenUser->User.Sid), Utility::_psid, pTokenUser->User.Sid)) { DWORD dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in CopySid - err=" << dwError); - std::cout << "TestIo::testCheckSetAndGetRights: Error in CopySid - err=" << dwError << std::endl; delete[] Utility::_psid; Utility::_psid = nullptr; return false; @@ -123,13 +114,11 @@ static bool initTrusteeWithUserSID() { // initialize the trustee structure BuildTrusteeWithSid(&Utility::_trustee, Utility::_psid); - std::cout << "TestIo::testCheckSetAndGetRights: trustee set" << std::endl; return true; } static bool init_private() { if (Utility::_psid != nullptr) { - std::cout << "TestIo::testCheckSetAndGetRights: _pssid is not null" << std::endl; return false; } From d28ad57c079e2b83eda9050bed8a8f0ab90054ae Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 24 Apr 2024 17:43:51 +0200 Subject: [PATCH 36/67] Refactor this code to not nest more than 3 if|for|do|while|switch statements. [SENTRY] --- src/libcommonserver/io/iohelper_win.cpp | 41 ++++++++++++------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index ef65feab7..7c7e66769 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -451,29 +451,26 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); ioError = dWordError2ioError(result); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (ioError == IoErrorAccessDenied) { - read = false; - write = false; - exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return _isExpectedError(ioError); - } + exists = ioError != IoErrorNoSuchFileOrDirectory; + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + return _isExpectedError(ioError); + } - if (result == ERROR_INVALID_SID) { // Access denied, try to force read control - setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, - &psecDesc); - result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); - ioError = dWordError2ioError(result); - - if (result != ERROR_SUCCESS) { - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return _isExpectedError(ioError); - } - } + if (result == ERROR_INVALID_SID) { // Access denied, try to force read control + setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); + ioError = dWordError2ioError(result); + } + + if (ioError != IoErrorSuccess) { + LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + return _isExpectedError(ioError); } bool readCtrl = From e520eda7cdd1db4d0b93d823e6b43d683375dd5e Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 08:27:56 +0200 Subject: [PATCH 37/67] Remove debug verbose. --- test/libcommonserver/io/testchecksetgetrights.cpp | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index f3866a3d5..21b813ac0 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -53,13 +53,9 @@ static int rightsSetToInt(rightsSet rightsSet) { } void TestIo::testCheckSetAndGetRights() { - std::cout << "TestIo::testCheckSetAndGetRights: start" << std::endl; - #ifdef _WIN32 - std::cout << "TestIo::testCheckSetAndGetRights: init trustee" << std::endl; CPPUNIT_ASSERT(Utility::init()); // Initialize the utility library, needed to access/change the permissions on Windows #endif - const TemporaryDirectory temporaryDirectory; // Test if the rights are correctly set and get on a directory @@ -79,7 +75,7 @@ void TestIo::testCheckSetAndGetRights() { // For a directory - /* Test all the possible rights and the all the possible order of rights modification. ie: + /* Test all the possible rights and all the possible order of rights modification. ie: * | READ | WRITE | EXECUTE | | * | 0 | 0 | 0 | v * | 0 | 0 | 1 | | @@ -99,8 +95,7 @@ void TestIo::testCheckSetAndGetRights() { * ... */ - for (int baseRigths = 0; baseRigths < 7; - baseRigths++) { // Test all the possible rights and the all the possible order of rights modification + for (int baseRigths = 0; baseRigths < 7; baseRigths++) { for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { rightsSetFromInt(baseRigths, rightsSet); @@ -120,8 +115,6 @@ void TestIo::testCheckSetAndGetRights() { if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && isExecutable == rightsSet.execute)) { IoHelper::setRights(path, true, true, true, ioError); - std::cout << "setted rigths (RWX): " << rightsSet.read << rightsSet.write << rightsSet.execute - << " | readed rigths(RWX): " << isReadable << isWritable << isExecutable << std::endl; CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); } @@ -143,8 +136,6 @@ void TestIo::testCheckSetAndGetRights() { if (!(exists && isReadable == rightsSet.read && isWritable == rightsSet.write && isExecutable == rightsSet.execute)) { IoHelper::setRights(path, true, true, true, ioError); - std::cout << "setted rigths (RWX): " << rightsSet.read << rightsSet.write << rightsSet.execute - << " | readed rigths(RWX): " << isReadable << isWritable << isExecutable << std::endl; CPPUNIT_ASSERT(false /* Set target rights mismatch with get target rights */); } } From 93349d6d4d0e01ae13b6b6c4bc45d2ea2d474838 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 08:43:41 +0200 Subject: [PATCH 38/67] Remove old static member. --- src/libcommonserver/io/iohelper.h | 4 ---- src/libcommonserver/io/iohelper_win.cpp | 2 -- 2 files changed, 6 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index e5ec29208..de84dd462 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -34,10 +34,6 @@ struct IoHelper { inline static void setLogger(log4cplus::Logger logger) { _logger = logger; } -#ifdef _WIN32 - static int _getAndSetRightsMethod; -#endif - static IoError stdError2ioError(int error) noexcept; static IoError stdError2ioError(const std::error_code &ec) noexcept; static IoError posixError2ioError(int error) noexcept; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 7c7e66769..3ae227218 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -90,8 +90,6 @@ time_t FileTimeToUnixTime(LARGE_INTEGER filetime, DWORD *remainder) { } } // namespace -int IoHelper::_getAndSetRightsMethod = 0; - bool IoHelper::fileExists(const std::error_code &ec) noexcept { return (ec.value() != ERROR_FILE_NOT_FOUND) && (ec.value() != ERROR_PATH_NOT_FOUND) && (ec.value() != ERROR_INVALID_DRIVE); } From 1f4e936be96c65cf3be8732c136eee59f82e6dcb Mon Sep 17 00:00:00 2001 From: Herve Eruam <82284536+herve-er@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:18:49 +0200 Subject: [PATCH 39/67] Update windows.yml --- .github/workflows/windows.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index d61e13400..cb0c073eb 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -41,5 +41,11 @@ jobs: - name: Run common tests run: ./build-Release/install/bin/kDrive_test_common.exe + - name: Upload logs artifact + uses: actions/upload-artifact@v2 + with: + name: kDrive-logs + path: ${{ env.LOCALAPPDATA }}/Temp/kDrive-logdir + - name: Clean-up generated code run : powershell ./infomaniak-build-tools/windows/build-drive.ps1 -clean all From 06fb0c497247cf5cd9e7ca23fdfb4f6b9b8c75a1 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 09:27:13 +0200 Subject: [PATCH 40/67] Revert last commit. --- src/libcommonserver/io/iohelper.h | 4 ++++ src/libcommonserver/io/iohelper_win.cpp | 2 ++ 2 files changed, 6 insertions(+) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index de84dd462..e5ec29208 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -34,6 +34,10 @@ struct IoHelper { inline static void setLogger(log4cplus::Logger logger) { _logger = logger; } +#ifdef _WIN32 + static int _getAndSetRightsMethod; +#endif + static IoError stdError2ioError(int error) noexcept; static IoError stdError2ioError(const std::error_code &ec) noexcept; static IoError posixError2ioError(int error) noexcept; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 3ae227218..7c7e66769 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -90,6 +90,8 @@ time_t FileTimeToUnixTime(LARGE_INTEGER filetime, DWORD *remainder) { } } // namespace +int IoHelper::_getAndSetRightsMethod = 0; + bool IoHelper::fileExists(const std::error_code &ec) noexcept { return (ec.value() != ERROR_FILE_NOT_FOUND) && (ec.value() != ERROR_PATH_NOT_FOUND) && (ec.value() != ERROR_INVALID_DRIVE); } From 8ee8029db55f76149430f8e73200b090ab7cde78 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 09:30:00 +0200 Subject: [PATCH 41/67] Remove artifact upload + remove static int _getAndSetRightsMethod; --- .github/workflows/windows.yml | 6 ------ src/libcommonserver/io/iohelper.h | 4 ---- src/libcommonserver/io/iohelper_win.cpp | 2 -- 3 files changed, 12 deletions(-) diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index cb0c073eb..5809fa3f9 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -40,12 +40,6 @@ jobs: - name: Run common tests run: ./build-Release/install/bin/kDrive_test_common.exe - - - name: Upload logs artifact - uses: actions/upload-artifact@v2 - with: - name: kDrive-logs - path: ${{ env.LOCALAPPDATA }}/Temp/kDrive-logdir - name: Clean-up generated code run : powershell ./infomaniak-build-tools/windows/build-drive.ps1 -clean all diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index e5ec29208..de84dd462 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -34,10 +34,6 @@ struct IoHelper { inline static void setLogger(log4cplus::Logger logger) { _logger = logger; } -#ifdef _WIN32 - static int _getAndSetRightsMethod; -#endif - static IoError stdError2ioError(int error) noexcept; static IoError stdError2ioError(const std::error_code &ec) noexcept; static IoError posixError2ioError(int error) noexcept; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 7c7e66769..3ae227218 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -90,8 +90,6 @@ time_t FileTimeToUnixTime(LARGE_INTEGER filetime, DWORD *remainder) { } } // namespace -int IoHelper::_getAndSetRightsMethod = 0; - bool IoHelper::fileExists(const std::error_code &ec) noexcept { return (ec.value() != ERROR_FILE_NOT_FOUND) && (ec.value() != ERROR_PATH_NOT_FOUND) && (ec.value() != ERROR_INVALID_DRIVE); } From fa6928860904a71a27086e8e04a66a4cd1e2a588 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 09:57:49 +0200 Subject: [PATCH 42/67] Replace _setRightsStandard by _setRightsStd --- src/libcommonserver/io/iohelper.cpp | 4 ++-- src/libcommonserver/io/iohelper.h | 2 +- src/libcommonserver/io/iohelper_win.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libcommonserver/io/iohelper.cpp b/src/libcommonserver/io/iohelper.cpp index 124bd6c08..e929dd367 100644 --- a/src/libcommonserver/io/iohelper.cpp +++ b/src/libcommonserver/io/iohelper.cpp @@ -511,11 +511,11 @@ bool IoHelper::createSymlink(const SyncPath &targetPath, const SyncPath &path, I #ifndef _WIN32 //See iohelper_win.cpp for the Windows implementation bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - return _setRightsStandart(path, read, write, exec, ioError); + return _setRightsStd(path, read, write, exec, ioError); } #endif -bool IoHelper::_setRightsStandart(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { +bool IoHelper::_setRightsStd(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { ioError = IoErrorSuccess; std::filesystem::perms perms = std::filesystem::perms::none; if (read) { diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index de84dd462..5153471e7 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -302,7 +302,7 @@ struct IoHelper { static bool _setTargetType(ItemType &itemType) noexcept; static bool _checkIfIsHiddenFile(const SyncPath &path, bool &isHidden, IoError &ioError) noexcept; - static bool _setRightsStandart(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; + static bool _setRightsStd(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept; }; } // namespace KDC diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 3ae227218..873f3bd6b 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -583,7 +583,7 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, return true; } else { - return _setRightsStandart(path, read, write, exec, ioError); + return _setRightsStd(path, read, write, exec, ioError); } } From 9ea157ff2c91673d52d61859dad3a92616b49d5f Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 10:00:26 +0200 Subject: [PATCH 43/67] Replace setRightsApiWindows by setRightsWindowsApi. --- src/libcommonserver/io/iohelper_win.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 873f3bd6b..3d34af456 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -329,7 +329,7 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return IoHelper::getXAttrValue(itemPath.native(), FILE_ATTRIBUTE_OFFLINE, isDehydrated, ioError); } -static bool setRightsApiWindows(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, +static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, log4cplus::Logger logger) noexcept { // Always return false if ioError is not IoErrorSuccess, // caller should check _isExpectedError(ioError) PACL pACL_old = nullptr; // Current ACL @@ -459,10 +459,10 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } if (result == ERROR_INVALID_SID) { // Access denied, try to force read control - setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); ioError = dWordError2ioError(result); } @@ -475,13 +475,13 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) if (!readCtrl) { - if (setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, + if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger())) { // Try to force read control GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); LocalFree(psecDesc); GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsApiWindows(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); // Revoke read control after reading the permissions readCtrl = (rights & READ_CONTROL) == READ_CONTROL; } @@ -570,11 +570,11 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } bool res = false; - res = setRightsApiWindows(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()); + res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()); if (!res) { return _isExpectedError(ioError); } - res = setRightsApiWindows(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); + res = setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); if (!res) { return _isExpectedError(ioError); From e7585ae4f7d3907178fe10880674799df7fdda14 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 10:13:45 +0200 Subject: [PATCH 44/67] Remove empty lines and improve comments. --- src/libcommonserver/io/iohelper_win.cpp | 16 +--------------- .../libcommonserver/io/testchecksetgetrights.cpp | 4 ++-- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 3d34af456..6433750c2 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -354,8 +354,6 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M Path2WStr(path).copy(pathw_ptr.get(), pathw_len); LPWSTR pathw = pathw_ptr.get(); pathw[pathw_len] = L'\0'; - - DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, nullptr, &pSecurityDescriptor); @@ -443,7 +441,6 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return _isExpectedError(ioError); } - // Get rights for trustee ACCESS_MASK rights = 0; result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); @@ -497,13 +494,10 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; return true; - - } - // Fallback method else { // Fallback method - // !!! When Deny Full control to file/directory => returns exists == false !!! + // !!! When Full control to file/directory is denied to the user, the function will return as if the file/directory does not exist. ItemType itemType; const bool success = getItemType(path, itemType); @@ -539,8 +533,6 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); return true; } - - return true; } @@ -579,9 +571,7 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, if (!res) { return _isExpectedError(ioError); } - return true; - } else { return _setRightsStd(path, read, write, exec, ioError); } @@ -615,11 +605,8 @@ bool IoHelper::checkIfIsJunction(const SyncPath &path, bool &isJunction, IoError return ioError == IoErrorNoSuchFileOrDirectory; } } - CloseHandle(hFile); - isJunction = ReparseBuffer.ReparseTag == IO_REPARSE_TAG_MOUNT_POINT; - return true; } @@ -687,7 +674,6 @@ bool IoHelper::readJunction(const SyncPath &path, std::string &data, SyncPath &t pPath[nameLength] = L'\0'; if (wcsncmp(pPath, L"\\??\\", 4) == 0) pPath += 4; // Skip 'non-parsed' prefix - targetPath = SyncPath(pPath); return true; diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index 21b813ac0..b07aa9868 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -144,7 +144,7 @@ void TestIo::testCheckSetAndGetRights() { IoHelper::setRights(path, true, true, true, ioError); } - // Test if the rights are correctly set and get on a file + // Test if the rights are correctly set and if they can be successfully retrieved from a file { const SyncPath filepath = temporaryDirectory.path / "changePerm.txt"; @@ -214,7 +214,7 @@ void TestIo::testCheckSetAndGetRights() { IoHelper::setRights(filepath, true, true, true, ioError); } - // Check permission is not recursive on folder + // Check permissions are not set recursively on a folder { const SyncPath path = temporaryDirectory.path / "testCheckSetAndGetRights"; const SyncPath subFolderPath = path / "subFolder"; From f12af122248f6dcb2d72c6ec08715e7c181b8fcd Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 10:20:08 +0200 Subject: [PATCH 45/67] use CPPUNIT_ASSERT_EQUAL instead of CPPUNIT_ASSERT(... == ...) --- test/libcommonserver/io/testchecksetgetrights.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index b07aa9868..f379fb7fd 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -64,7 +64,7 @@ void TestIo::testCheckSetAndGetRights() { IoError ioError = IoErrorUnknown; CPPUNIT_ASSERT(IoHelper::createDirectory(path, ioError)); - CPPUNIT_ASSERT(ioError == IoErrorSuccess); + CPPUNIT_ASSERT_EQUAL(IoErrorSuccess, ioError); bool isReadable = false; bool isWritable = false; @@ -223,10 +223,10 @@ void TestIo::testCheckSetAndGetRights() { IoError ioError = IoErrorUnknown; CPPUNIT_ASSERT(IoHelper::createDirectory(path, ioError)); - CPPUNIT_ASSERT(ioError == IoErrorSuccess); + CPPUNIT_ASSERT_EQUAL(IoErrorSuccess, ioError); CPPUNIT_ASSERT(IoHelper::createDirectory(subFolderPath, ioError)); - CPPUNIT_ASSERT(ioError == IoErrorSuccess); + CPPUNIT_ASSERT_EQUAL(IoErrorSuccess, ioError); std::ofstream file(subFilePath.string()); CPPUNIT_ASSERT(file.is_open()); From c435db833e99dab58407ed5f7386c12499fd6d43 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 10:34:10 +0200 Subject: [PATCH 46/67] Change rightsSet to Rightset and add a constructor with an int as input. --- .../io/testchecksetgetrights.cpp | 42 +++++-------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index f379fb7fd..4c7b80951 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -25,33 +25,14 @@ using namespace CppUnit; namespace KDC { -static struct rightsSet { +static struct RightsSet { + RightsSet(int rights) : read(rights & 4), write(rights & 2), execute(rights & 1){}; + RightsSet(bool read, bool write, bool execute) : read(read), write(write), execute(execute){}; bool read; bool write; bool execute; }; -static void rightsSetFromInt(int rights, rightsSet& rightsSet) { - rightsSet.read = rights & 4; - rightsSet.write = rights & 2; - rightsSet.execute = rights & 1; -} - -static int rightsSetToInt(rightsSet rightsSet) { - int rights = 0; - if (rightsSet.read) { - rights += 4; - } - if (rightsSet.write) { - rights += 2; - } - if (rightsSet.execute) { - rights += 1; - } - - return rights; -} - void TestIo::testCheckSetAndGetRights() { #ifdef _WIN32 CPPUNIT_ASSERT(Utility::init()); // Initialize the utility library, needed to access/change the permissions on Windows @@ -71,7 +52,7 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; - rightsSet rightsSet = {false, false, false}; + RightsSet rightsSet(false, false, false); // For a directory @@ -97,8 +78,7 @@ void TestIo::testCheckSetAndGetRights() { for (int baseRigths = 0; baseRigths < 7; baseRigths++) { for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { - rightsSetFromInt(baseRigths, rightsSet); - + rightsSet = RightsSet(baseRigths); bool result = IoHelper::setRights(path, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { @@ -118,8 +98,7 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); } - rightsSetFromInt(targetRigths, rightsSet); - + rightsSet = RightsSet(targetRigths); result = IoHelper::setRights(path, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { @@ -160,14 +139,14 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; - rightsSet rightsSet = {false, false, false}; + RightsSet rightsSet = {false, false, false}; // For a directory for (int baseRigths = 0; baseRigths < 7; baseRigths++) { // Test all the possible rights and the all the possible order of rights modification for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { - rightsSetFromInt(baseRigths, rightsSet); + rightsSet = RightsSet(baseRigths); bool result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { @@ -187,8 +166,7 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(false /* Set base rights mismatch with get base rights */); } - rightsSetFromInt(targetRigths, rightsSet); - + rightsSet = RightsSet(targetRigths); result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { @@ -228,7 +206,7 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(IoHelper::createDirectory(subFolderPath, ioError)); CPPUNIT_ASSERT_EQUAL(IoErrorSuccess, ioError); - std::ofstream file(subFilePath.string()); + std::ofstream file(subFilePath); CPPUNIT_ASSERT(file.is_open()); file << "testCheckSetAndGetRights"; file.close(); From dd6b5a0af9be3ce6d4dcfee81698ea2a2da085be Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 10:37:38 +0200 Subject: [PATCH 47/67] Remove useless .string --- test/libcommonserver/io/testchecksetgetrights.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index 4c7b80951..b3788518b 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -129,7 +129,7 @@ void TestIo::testCheckSetAndGetRights() { IoError ioError = IoErrorUnknown; - std::ofstream file(filepath.string()); + std::ofstream file(filepath); CPPUNIT_ASSERT(file.is_open()); file << "testCheckSetAndGetRights"; file.close(); From d35a39f8e6dc881ecf08866316e317cdfe6806d2 Mon Sep 17 00:00:00 2001 From: Herve Eruam <82284536+herve-er@users.noreply.github.com> Date: Thu, 25 Apr 2024 10:45:50 +0200 Subject: [PATCH 48/67] Apply suggestions from code review (LOG) Co-authored-by: Luc Guyot <162997198+luc-guyot-infomaniak@users.noreply.github.com> --- src/libcommonserver/io/iohelper_win.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 6433750c2..fc6dea535 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -359,7 +359,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" + LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -370,7 +370,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" + LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str()); << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -380,7 +380,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (!IsValidAcl(pACL_new)) { ioError = IoErrorUnknown; - LOG_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); + LOGW_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -391,7 +391,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" + LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); << IoHelper::ioError2StdString(ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -437,7 +437,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); } - LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW - path=" << szFilePath << L" result=" << result); + LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW:" << Utility::formatSyncPath(path).c_str() << L" result=" << result); return _isExpectedError(ioError); } @@ -484,7 +484,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } if (!readCtrl) { ioError = IoErrorAccessDenied; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + LOGW_INFO(logger(), L"Access denied : " << Utility::formatSyncPath(path).c_str()); return _isExpectedError(ioError); } } From 83c98ab2cad98271d0e761f136ae9bf377589cec Mon Sep 17 00:00:00 2001 From: Herve Eruam <82284536+herve-er@users.noreply.github.com> Date: Thu, 25 Apr 2024 10:45:50 +0200 Subject: [PATCH 49/67] Apply suggestions from code review (LOG) Co-authored-by: Luc Guyot <162997198+luc-guyot-infomaniak@users.noreply.github.com> --- src/libcommonserver/io/iohelper_win.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 6433750c2..f1ee9dba9 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -359,8 +359,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -370,8 +369,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -380,7 +378,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (!IsValidAcl(pACL_new)) { ioError = IoErrorUnknown; - LOG_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); + LOGW_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -391,8 +389,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOG_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatSyncPath(path).c_str() << L" error=" - << IoHelper::ioError2StdString(ioError).c_str()); + LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -437,7 +434,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); } - LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW - path=" << szFilePath << L" result=" << result); + LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW:" << Utility::formatSyncPath(path).c_str() << L" result=" << result); return _isExpectedError(ioError); } @@ -484,7 +481,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } if (!readCtrl) { ioError = IoErrorAccessDenied; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); + LOGW_INFO(logger(), L"Access denied : " << Utility::formatSyncPath(path).c_str()); return _isExpectedError(ioError); } } From 6472cc919065de890487e746a983781ea479f403 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 11:44:31 +0200 Subject: [PATCH 50/67] Minor change for a safer implementation. --- test/libcommonserver/io/testchecksetgetrights.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index b3788518b..da7ca78b0 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -139,14 +139,12 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; - RightsSet rightsSet = {false, false, false}; - // For a directory for (int baseRigths = 0; baseRigths < 7; baseRigths++) { // Test all the possible rights and the all the possible order of rights modification for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { - rightsSet = RightsSet(baseRigths); + auto rightsSet = RightsSet(baseRigths); bool result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { From 99cfbb61674dc6c137fcc411837895b67842c5ad Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 11:46:01 +0200 Subject: [PATCH 51/67] idem. --- test/libcommonserver/io/testchecksetgetrights.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index da7ca78b0..a2814fce7 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -52,10 +52,6 @@ void TestIo::testCheckSetAndGetRights() { bool isExecutable = false; bool exists = false; - RightsSet rightsSet(false, false, false); - - // For a directory - /* Test all the possible rights and all the possible order of rights modification. ie: * | READ | WRITE | EXECUTE | | * | 0 | 0 | 0 | v @@ -78,7 +74,7 @@ void TestIo::testCheckSetAndGetRights() { for (int baseRigths = 0; baseRigths < 7; baseRigths++) { for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { - rightsSet = RightsSet(baseRigths); + auto rightsSet = RightsSet(baseRigths); bool result = IoHelper::setRights(path, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; if (!result) { From a5c903deb7f5fd45adf9baed8c722b029eb1d50f Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 11:51:50 +0200 Subject: [PATCH 52/67] Update some log messages. --- src/libcommonserver/io/iohelper_mac.cpp | 4 ++-- src/libcommonserver/io/iohelper_win.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index 33c1ee002..1b68b86bf 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -222,7 +222,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex ItemType itemType; const bool success = getItemType(path, itemType); if (!success) { - LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, itemType.ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type: " << Utility::formatIoError(path, itemType.ioError).c_str()); return false; } exists = itemType.ioError != IoErrorNoSuchFileOrDirectory; @@ -241,7 +241,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } - LOGW_WARN(logger(), L"Failed to get permissions - " << Utility::formatStdError(path, ec).c_str()); + LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); return false; } diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index f1ee9dba9..d84a99daa 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -500,7 +500,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex const bool success = getItemType(path, itemType); ioError = itemType.ioError; if (!success) { - LOGW_WARN(logger(), L"Failed to get item type - " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger(), L"Failed to get item type: " << Utility::formatIoError(path, ioError).c_str()); return false; } @@ -520,7 +520,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return true; } - LOGW_WARN(logger(), L"Failed to get permissions - " << Utility::formatStdError(path, ec).c_str()); + LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); return false; } From 2d3286eda4c47c605bfe584dead36d5aef25f348 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 12:14:52 +0200 Subject: [PATCH 53/67] Code optimization. --- src/libcommonserver/io/iohelper_win.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index d84a99daa..5e914c00f 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -332,8 +332,8 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, log4cplus::Logger logger) noexcept { // Always return false if ioError is not IoErrorSuccess, // caller should check _isExpectedError(ioError) - PACL pACL_old = nullptr; // Current ACL - PACL pACL_new = nullptr; // New ACL + PACL pACL_old = nullptr; // Current ACL + PACL pACL_new = nullptr; // New ACL PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; EXPLICIT_ACCESS ExplicitAccess; ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); @@ -347,13 +347,16 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ExplicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; ExplicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; - LPCWSTR pathw_c = Path2WStr(path).c_str(); - size_t pathw_len = Path2WStr(path).length(); + std::wstring path_wstr = Path2WStr(path); + size_t pathw_len = path_wstr.length(); auto pathw_ptr = std::make_unique(pathw_len + 1); - Path2WStr(path).copy(pathw_ptr.get(), pathw_len); + path_wstr.copy(pathw_ptr.get(), pathw_len); + + LPCWSTR pathw_c = path_wstr.c_str(); LPWSTR pathw = pathw_ptr.get(); pathw[pathw_len] = L'\0'; + DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, nullptr, &pSecurityDescriptor); @@ -434,7 +437,8 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); } - LOGW_WARN(logger(), L"Error in GetNamedSecurityInfoW:" << Utility::formatSyncPath(path).c_str() << L" result=" << result); + LOGW_WARN(logger(), + L"Error in GetNamedSecurityInfoW:" << Utility::formatSyncPath(path).c_str() << L" result=" << result); return _isExpectedError(ioError); } @@ -491,8 +495,7 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; return true; - } - else { + } else { // Fallback method // !!! When Full control to file/directory is denied to the user, the function will return as if the file/directory does not exist. From 3debdbcf7a15212925500533da54c30577353844 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 14:39:56 +0200 Subject: [PATCH 54/67] Re implement fallback method + Add sentry event in cas of fail with windows API. --- src/libcommonserver/io/iohelper.h | 4 + src/libcommonserver/io/iohelper_win.cpp | 278 +++++++++++++----------- 2 files changed, 160 insertions(+), 122 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 5153471e7..6a93457e8 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -34,6 +34,10 @@ struct IoHelper { inline static void setLogger(log4cplus::Logger logger) { _logger = logger; } +#ifdef _WIN32 + static int _getAndSetRightsMethod; +#endif + static IoError stdError2ioError(int error) noexcept; static IoError stdError2ioError(const std::error_code &ec) noexcept; static IoError posixError2ioError(int error) noexcept; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 5e914c00f..35e7519e1 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -90,6 +90,8 @@ time_t FileTimeToUnixTime(LARGE_INTEGER filetime, DWORD *remainder) { } } // namespace +int IoHelper::_getAndSetRightsMethod = 0; + bool IoHelper::fileExists(const std::error_code &ec) noexcept { return (ec.value() != ERROR_FILE_NOT_FOUND) && (ec.value() != ERROR_PATH_NOT_FOUND) && (ec.value() != ERROR_INVALID_DRIVE); } @@ -330,10 +332,10 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra } static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, - log4cplus::Logger logger) noexcept { // Always return false if ioError is not IoErrorSuccess, - // caller should check _isExpectedError(ioError) - PACL pACL_old = nullptr; // Current ACL - PACL pACL_new = nullptr; // New ACL + log4cplus::Logger logger) noexcept { // Return false if we should try the fallback method + + PACL pACL_old = nullptr; // Current ACL + PACL pACL_new = nullptr; // New ACL PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; EXPLICIT_ACCESS ExplicitAccess; ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); @@ -362,7 +364,8 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() + << L" | DWORD error: " << ValueReturned); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -372,7 +375,8 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " + << ValueReturned); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -381,7 +385,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M if (!IsValidAcl(pACL_new)) { ioError = IoErrorUnknown; - LOGW_WARN(logger, L"Invalid ACL: " << Utility::formatSyncPath(path).c_str()); + LOGW_WARN(logger, L"Invalid new ACL: " << Utility::formatSyncPath(path).c_str()); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); @@ -392,7 +396,8 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); - LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str()); + LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() + << L" | DWORD error: " << ValueReturned); LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -407,138 +412,162 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M return true; } -bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { - read = false; - write = false; - exec = false; - exists = false; - +static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists, + log4cplus::Logger logger) noexcept { // Return false if we should try the fallback method IoError ioError = IoErrorSuccess; + WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; + lstrcpyW(szFilePath, path.native().c_str()); + + // Get security info + PACL pfileACL = NULL; + PSECURITY_DESCRIPTOR psecDesc = NULL; + DWORD result = + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + LocalFree(psecDesc); + ioError = dWordError2ioError(result); + + if (ioError != IoErrorSuccess) { + exists = ioError != IoErrorNoSuchFileOrDirectory; - // Preferred method - if (Utility::_trustee.ptstrName) { - WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; - lstrcpyW(szFilePath, path.native().c_str()); - - // Get security info - PACL pfileACL = NULL; - PSECURITY_DESCRIPTOR psecDesc = NULL; - DWORD result = - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - LocalFree(psecDesc); - ioError = dWordError2ioError(result); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - - if (ioError == IoErrorAccessDenied) { - read = false; - write = false; - exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - } + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + exists = false; + return true; // Expected error if access is denied + } - LOGW_WARN(logger(), - L"Error in GetNamedSecurityInfoW:" << Utility::formatSyncPath(path).c_str() << L" result=" << result); - return _isExpectedError(ioError); + if (exists) { + LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() + << L" | DWORD error: " << result); + return false; // Unexpected error if the file exists but we can't get the security info (and it's not an access + // denied) } + return true; // Expected error if the file doesn't exist + } + + // Get rights for trustee + ACCESS_MASK rights = 0; + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + ioError = dWordError2ioError(result); + exists = ioError != IoErrorNoSuchFileOrDirectory; + + if (ioError == IoErrorAccessDenied) { + read = false; + write = false; + exec = false; + exists = false; + LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: " << Utility::formatIoError(path, ioError).c_str()); + return true; // Expected error if access is denied + } - // Get rights for trustee - ACCESS_MASK rights = 0; + if (result == ERROR_INVALID_SID) { // Access denied, try to force read control and get the rights + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger); + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger); ioError = dWordError2ioError(result); + } - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (ioError == IoErrorAccessDenied) { + if (ioError != IoErrorSuccess) { + LOGW_INFO(logger, + L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " << result); + return false; // Unexpected error if we can't get the rights + } + + bool readCtrl = (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) + if (!readCtrl) { + if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, + logger)) { // Try to force read control + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + LocalFree(psecDesc); + GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, + logger); // Revoke read control after reading the permissions + readCtrl = (rights & READ_CONTROL) == READ_CONTROL; + } + if (!readCtrl) { + ioError = IoErrorAccessDenied; + LOGW_INFO(logger, L"Access denied: " << Utility::formatIoError(path, ioError).c_str()); read = false; write = false; exec = false; - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return _isExpectedError(ioError); - } - - if (result == ERROR_INVALID_SID) { // Access denied, try to force read control - setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger()); - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger()); - ioError = dWordError2ioError(result); + exists = false; + return true; // Expected error if access is denied } + } - if (ioError != IoErrorSuccess) { - LOGW_INFO(logger(), L"Access denied - path=" << szFilePath); - return _isExpectedError(ioError); - } + exists = true; + read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; + write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; + exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; + return true; +} - bool readCtrl = - (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) +bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { + // !!! When Full control to file/directory is denied to the user, the function will return as if the file/directory does not exist. - if (!readCtrl) { - if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, - logger())) { // Try to force read control - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, - &psecDesc); - LocalFree(psecDesc); - GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, - logger()); // Revoke read control after reading the permissions - readCtrl = (rights & READ_CONTROL) == READ_CONTROL; - } - if (!readCtrl) { - ioError = IoErrorAccessDenied; - LOGW_INFO(logger(), L"Access denied : " << Utility::formatSyncPath(path).c_str()); - return _isExpectedError(ioError); - } + read = false; + write = false; + exec = false; + exists = false; + // Preferred method + if (_getAndSetRightsMethod == 0 && Utility::_trustee.ptstrName) { + if (getRightsWindowsApi(path, read, write, exec, exists, logger())) { + return true; } + LOGW_WARN(logger(), L"Failed to get rights using Windows API, falling back to std::filesystem."); + sentry_value_t event = sentry_value_new_event(); + sentry_value_t exc = + sentry_value_new_exception("Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); + sentry_value_set_stacktrace(exc, NULL, 0); + sentry_event_add_exception(event, exc); + sentry_capture_event(event); + Utility::_trustee.ptstrName = nullptr; + _getAndSetRightsMethod = 1; + } + + // Fallback method. + IoError ioError; + ItemType itemType; + const bool success = getItemType(path, itemType); + ioError = itemType.ioError; + + if (!success) { + LOGW_WARN(logger(), L"Failed to get item type: " << Utility::formatIoError(path, ioError).c_str()); + return false; + } - exists = true; - read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; - write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; - exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; + exists = ioError != IoErrorNoSuchFileOrDirectory; + if (!exists) { return true; - } else { - // Fallback method - // !!! When Full control to file/directory is denied to the user, the function will return as if the file/directory does not exist. - - ItemType itemType; - const bool success = getItemType(path, itemType); - ioError = itemType.ioError; - if (!success) { - LOGW_WARN(logger(), L"Failed to get item type: " << Utility::formatIoError(path, ioError).c_str()); - return false; - } + } + const bool isSymlink = itemType.linkType == LinkTypeSymlink; + std::error_code ec; + std::filesystem::perms perms = + isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); + ioError = stdError2ioError(ec); + if (ioError != IoErrorSuccess) { exists = ioError != IoErrorNoSuchFileOrDirectory; if (!exists) { return true; } - const bool isSymlink = itemType.linkType == LinkTypeSymlink; - - std::error_code ec; - std::filesystem::perms perms = - isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); - ioError = stdError2ioError(ec); - if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; - } - LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); - return false; - } - - exists = true; - read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); - write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); - exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); - return true; + LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); + return false; } + + exists = true; + read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); + write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); + exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); return true; } - bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, IoError &ioError) noexcept { - if (Utility::_trustee.ptstrName) { + // Preferred method + if (_getAndSetRightsMethod == 0 && Utility::_trustee.ptstrName) { ioError = IoErrorSuccess; DWORD grantedPermission = WRITE_DAC; DWORD deniedPermission = 0; @@ -560,21 +589,26 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } else { grantedPermission |= FILE_GENERIC_EXECUTE; } - bool res = false; res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()); - if (!res) { - return _isExpectedError(ioError); + if (res) { + res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); } - res = setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); - - if (!res) { - return _isExpectedError(ioError); + if (res) { + return true; } - return true; - } else { - return _setRightsStd(path, read, write, exec, ioError); - } + LOGW_WARN(logger(), L"Failed to set rights using Windows API, falling back to std::filesystem."); + sentry_value_t event = sentry_value_new_event(); + sentry_value_t exc = + sentry_value_new_exception("Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); + sentry_value_set_stacktrace(exc, NULL, 0); + sentry_event_add_exception(event, exc); + sentry_capture_event(event); + Utility::_trustee.ptstrName = nullptr; + _getAndSetRightsMethod = 1; + + } + return _setRightsStd(path, read, write, exec, ioError); } bool IoHelper::checkIfIsJunction(const SyncPath &path, bool &isJunction, IoError &ioError) noexcept { From 0df63ad98d852703e2c6e662910767bf9d239542 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 14:57:10 +0200 Subject: [PATCH 55/67] Replace PSID _psid with an unique_ptr --- src/libcommonserver/utility/utility.cpp | 2 +- src/libcommonserver/utility/utility.h | 2 +- src/libcommonserver/utility/utility_win.cpp | 20 ++++++-------------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/libcommonserver/utility/utility.cpp b/src/libcommonserver/utility/utility.cpp index 29c84210f..5b0e84605 100644 --- a/src/libcommonserver/utility/utility.cpp +++ b/src/libcommonserver/utility/utility.cpp @@ -100,7 +100,7 @@ struct VariantPrinter { log4cplus::Logger Utility::_logger; #ifdef _WIN32 -PSID Utility::_psid = NULL; +std::unique_ptr Utility::_psid = nullptr; TRUSTEE Utility::_trustee = {0}; #endif diff --git a/src/libcommonserver/utility/utility.h b/src/libcommonserver/utility/utility.h index ffd84a023..3b6fac900 100644 --- a/src/libcommonserver/utility/utility.h +++ b/src/libcommonserver/utility/utility.h @@ -47,7 +47,7 @@ namespace KDC { struct COMMONSERVER_EXPORT Utility { #ifdef _WIN32 - static PSID _psid; + static std::unique_ptr _psid; static TRUSTEE _trustee; #endif diff --git a/src/libcommonserver/utility/utility_win.cpp b/src/libcommonserver/utility/utility_win.cpp index a3be81cc7..264067b5d 100644 --- a/src/libcommonserver/utility/utility_win.cpp +++ b/src/libcommonserver/utility/utility_win.cpp @@ -47,9 +47,7 @@ namespace KDC { static bool initTrusteeWithUserSID() { if (Utility::_psid != nullptr) { // should never happen - delete[] Utility::_psid; - Utility::_psid = nullptr; - + Utility::_psid.reset(); LOGW_WARN(Log::instance()->getLogger(), "Error in initTrusteeWithUserSID - _pssid is not null"); } Utility::_trustee = {0}; @@ -97,22 +95,17 @@ static bool initTrusteeWithUserSID() { return false; } - Utility::_psid = new BYTE[GetLengthSid(pTokenUser->User.Sid)]; - if (Utility::_psid == nullptr) { - LOGW_WARN(Log::instance()->getLogger(), "Memory allocation error"); - return false; - } + Utility::_psid = std::make_unique(GetLengthSid(pTokenUser->User.Sid)); - if (!CopySid(GetLengthSid(pTokenUser->User.Sid), Utility::_psid, pTokenUser->User.Sid)) { + if (!CopySid(GetLengthSid(pTokenUser->User.Sid), Utility::_psid.get(), pTokenUser->User.Sid)) { DWORD dwError = GetLastError(); LOGW_WARN(Log::instance()->getLogger(), "Error in CopySid - err=" << dwError); - delete[] Utility::_psid; - Utility::_psid = nullptr; + Utility::_psid.reset(); return false; } // initialize the trustee structure - BuildTrusteeWithSid(&Utility::_trustee, Utility::_psid); + BuildTrusteeWithSid(&Utility::_trustee, Utility::_psid.get()); return true; } @@ -129,8 +122,7 @@ static bool init_private() { static void free_private() { if (Utility::_psid != nullptr) { - delete[] Utility::_psid; - Utility::_psid = NULL; + Utility::_psid.reset(); } } From de432dbeb91fb2a0ff6d22d0d63515937f247006 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 15:22:19 +0200 Subject: [PATCH 56/67] Fix error with non existing path. --- src/libcommonserver/io/iohelper_win.cpp | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 35e7519e1..e9021e447 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -361,7 +361,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, nullptr, &pSecurityDescriptor); - + ioError = dWordError2ioError(ValueReturned); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() @@ -369,10 +369,14 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { + return true; + } return false; } ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); + ioError = dWordError2ioError(ValueReturned); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " @@ -380,6 +384,9 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { + return true; + } return false; } @@ -394,6 +401,7 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M } ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); + ioError = dWordError2ioError(ValueReturned); if (ValueReturned != ERROR_SUCCESS) { ioError = dWordError2ioError(ValueReturned); LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() @@ -401,6 +409,9 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACL_new); // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { + return true; + } return false; } @@ -518,8 +529,8 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } LOGW_WARN(logger(), L"Failed to get rights using Windows API, falling back to std::filesystem."); sentry_value_t event = sentry_value_new_event(); - sentry_value_t exc = - sentry_value_new_exception("Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); + sentry_value_t exc = sentry_value_new_exception( + "Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); sentry_value_set_stacktrace(exc, NULL, 0); sentry_event_add_exception(event, exc); sentry_capture_event(event); @@ -599,14 +610,13 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } LOGW_WARN(logger(), L"Failed to set rights using Windows API, falling back to std::filesystem."); sentry_value_t event = sentry_value_new_event(); - sentry_value_t exc = - sentry_value_new_exception("Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); + sentry_value_t exc = sentry_value_new_exception( + "Exception", "Failed to set/get rights using Windows API, falling back to std::filesystem."); sentry_value_set_stacktrace(exc, NULL, 0); sentry_event_add_exception(event, exc); sentry_capture_event(event); Utility::_trustee.ptstrName = nullptr; _getAndSetRightsMethod = 1; - } return _setRightsStd(path, read, write, exec, ioError); } From 1503b4da73de1934bc8783a04c0c0d6f426afa5e Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 15:31:02 +0200 Subject: [PATCH 57/67] Add a check on each test for windos to ensure all the call the windows api succeeded. --- CMakeLists.txt | 2 ++ test/libcommonserver/CMakeLists.txt | 8 ++++---- test/libcommonserver/io/testchecksetgetrights.cpp | 13 ++++++++++++- test/libparms/CMakeLists.txt | 8 ++++---- test/libsyncengine/CMakeLists.txt | 8 ++++---- test/server/CMakeLists.txt | 8 ++++---- 6 files changed, 30 insertions(+), 17 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1ef67e320..823aeb14b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,6 +7,8 @@ IF(APPLE) SET(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build architectures for Mac OS X" FORCE) ENDIF(APPLE) +SET(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;") + set(BIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") set(BUILD_SHARED_LIBS FALSE) diff --git a/test/libcommonserver/CMakeLists.txt b/test/libcommonserver/CMakeLists.txt index 85d165039..6efb2f9ec 100644 --- a/test/libcommonserver/CMakeLists.txt +++ b/test/libcommonserver/CMakeLists.txt @@ -33,8 +33,8 @@ if(WIN32) endif() if (WIN32) - include_directories("F:/Projects/log4cplus/include") - include_directories("C:/Program Files (x86)/cppunit/include") + include_directories("C:/Projects/log4cplus/include") + include_directories("C:/Projects/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -64,9 +64,9 @@ endif() if (WIN32) target_link_libraries(${testcommon_NAME} debug - "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" + "C:/Projects/cppunit/lib/cppunitd.lib" optimized - "C:/Program Files (x86)/cppunit/lib/cppunit.lib") + "C:/Projects/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testcommon_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index a2814fce7..6933c54aa 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -117,6 +117,9 @@ void TestIo::testCheckSetAndGetRights() { } // Restore the rights IoHelper::setRights(path, true, true, true, ioError); +#ifdef _WIN32 + CPPUNIT_ASSERT_EQUAL(0, IoHelper::_getAndSetRightsMethod); // Check that no error occurred with the wndows API +#endif } // Test if the rights are correctly set and if they can be successfully retrieved from a file @@ -139,7 +142,6 @@ void TestIo::testCheckSetAndGetRights() { for (int baseRigths = 0; baseRigths < 7; baseRigths++) { // Test all the possible rights and the all the possible order of rights modification for (int targetRigths = baseRigths + 1; targetRigths < 8; targetRigths++) { - auto rightsSet = RightsSet(baseRigths); bool result = IoHelper::setRights(filepath, rightsSet.read, rightsSet.write, rightsSet.execute, ioError); result &= ioError == IoErrorSuccess; @@ -184,6 +186,9 @@ void TestIo::testCheckSetAndGetRights() { // Restore the rights IoHelper::setRights(filepath, true, true, true, ioError); +#ifdef _WIN32 + CPPUNIT_ASSERT_EQUAL(0, IoHelper::_getAndSetRightsMethod); // Check that no error occurred with the wndows API +#endif } // Check permissions are not set recursively on a folder @@ -240,6 +245,9 @@ void TestIo::testCheckSetAndGetRights() { // Restore the rights IoHelper::setRights(path, true, true, true, ioError); // Restore the rights for delete +#ifdef _WIN32 + CPPUNIT_ASSERT_EQUAL(0, IoHelper::_getAndSetRightsMethod); // Check that no error occurred with the wndows API +#endif } // Test on a non existing file @@ -256,6 +264,9 @@ void TestIo::testCheckSetAndGetRights() { CPPUNIT_ASSERT(IoHelper::setRights(path, true, true, true, ioError)); CPPUNIT_ASSERT(ioError == IoErrorNoSuchFileOrDirectory); +#ifdef _WIN32 + CPPUNIT_ASSERT_EQUAL(0, IoHelper::_getAndSetRightsMethod); // Check that no error occurred with the wndows API +#endif } } } // namespace KDC diff --git a/test/libparms/CMakeLists.txt b/test/libparms/CMakeLists.txt index 1bf7577d5..1c26a0b79 100644 --- a/test/libparms/CMakeLists.txt +++ b/test/libparms/CMakeLists.txt @@ -18,8 +18,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("F:/Projects/log4cplus/include") - include_directories("C:/Program Files (x86)/cppunit/include") + include_directories("C:/Projects/log4cplus/include") + include_directories("C:/Projects/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -48,9 +48,9 @@ endif() if (WIN32) target_link_libraries(${testparms_NAME} debug - "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" + "C:/Projects/cppunit/lib/cppunitd.lib" optimized - "C:/Program Files (x86)/cppunit/lib/cppunit.lib") + "C:/Projects/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testparms_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libsyncengine/CMakeLists.txt b/test/libsyncengine/CMakeLists.txt index bae0f08df..8d7c69624 100644 --- a/test/libsyncengine/CMakeLists.txt +++ b/test/libsyncengine/CMakeLists.txt @@ -48,8 +48,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("F:/Projects/log4cplus/include") - include_directories("C:/Program Files (x86)/cppunit/include") + include_directories("C:/Projects/log4cplus/include") + include_directories("C:/Projects/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -78,9 +78,9 @@ endif() if (WIN32) target_link_libraries(${testsyncengine_NAME} debug - "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" + "C:/Projects/cppunit/lib/cppunitd.lib" optimized - "C:/Program Files (x86)/cppunit/lib/cppunit.lib") + "C:/Projects/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testsyncengine_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/server/CMakeLists.txt b/test/server/CMakeLists.txt index cfa9396e6..d9b1779e8 100644 --- a/test/server/CMakeLists.txt +++ b/test/server/CMakeLists.txt @@ -15,8 +15,8 @@ if(APPLE) endif() if (WIN32) - include_directories("F:/Projects/log4cplus/include") - include_directories("C:/Program Files (x86)/cppunit/include") + include_directories("C:/Projects/log4cplus/include") + include_directories("C:/Projects/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -47,9 +47,9 @@ endif() if (WIN32) target_link_libraries(${testserver_NAME} debug - "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" + "C:/Projects/cppunit/lib/cppunitd.lib" optimized - "C:/Program Files (x86)/cppunit/lib/cppunit.lib") + "C:/Projects/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testserver_NAME} "/usr/local/lib/libcppunit.dylib") From 4314bed4ee45b8601049ed6e55a0a568e18aa9af Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Thu, 25 Apr 2024 15:32:11 +0200 Subject: [PATCH 58/67] Revert unwanted push of CMAKE file. --- CMakeLists.txt | 2 -- test/libcommonserver/CMakeLists.txt | 8 ++++---- test/libparms/CMakeLists.txt | 8 ++++---- test/libsyncengine/CMakeLists.txt | 8 ++++---- test/server/CMakeLists.txt | 8 ++++---- 5 files changed, 16 insertions(+), 18 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 823aeb14b..1ef67e320 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,8 +7,6 @@ IF(APPLE) SET(CMAKE_OSX_ARCHITECTURES "x86_64;arm64" CACHE STRING "Build architectures for Mac OS X" FORCE) ENDIF(APPLE) -SET(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;") - set(BIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin") set(BUILD_SHARED_LIBS FALSE) diff --git a/test/libcommonserver/CMakeLists.txt b/test/libcommonserver/CMakeLists.txt index 6efb2f9ec..85d165039 100644 --- a/test/libcommonserver/CMakeLists.txt +++ b/test/libcommonserver/CMakeLists.txt @@ -33,8 +33,8 @@ if(WIN32) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -64,9 +64,9 @@ endif() if (WIN32) target_link_libraries(${testcommon_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testcommon_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libparms/CMakeLists.txt b/test/libparms/CMakeLists.txt index 1c26a0b79..1bf7577d5 100644 --- a/test/libparms/CMakeLists.txt +++ b/test/libparms/CMakeLists.txt @@ -18,8 +18,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -48,9 +48,9 @@ endif() if (WIN32) target_link_libraries(${testparms_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testparms_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/libsyncengine/CMakeLists.txt b/test/libsyncengine/CMakeLists.txt index 8d7c69624..bae0f08df 100644 --- a/test/libsyncengine/CMakeLists.txt +++ b/test/libsyncengine/CMakeLists.txt @@ -48,8 +48,8 @@ if (USE_OUR_OWN_SQLITE3) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -78,9 +78,9 @@ endif() if (WIN32) target_link_libraries(${testsyncengine_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testsyncengine_NAME} "/usr/local/lib/libcppunit.dylib") diff --git a/test/server/CMakeLists.txt b/test/server/CMakeLists.txt index d9b1779e8..cfa9396e6 100644 --- a/test/server/CMakeLists.txt +++ b/test/server/CMakeLists.txt @@ -15,8 +15,8 @@ if(APPLE) endif() if (WIN32) - include_directories("C:/Projects/log4cplus/include") - include_directories("C:/Projects/cppunit/include") + include_directories("F:/Projects/log4cplus/include") + include_directories("C:/Program Files (x86)/cppunit/include") else() include_directories("/usr/local/include") endif() @@ -47,9 +47,9 @@ endif() if (WIN32) target_link_libraries(${testserver_NAME} debug - "C:/Projects/cppunit/lib/cppunitd.lib" + "C:/Program Files (x86)/cppunit/lib/cppunitd.lib" optimized - "C:/Projects/cppunit/lib/cppunit.lib") + "C:/Program Files (x86)/cppunit/lib/cppunit.lib") elseif(APPLE) target_link_libraries(${testserver_NAME} "/usr/local/lib/libcppunit.dylib") From f1535f95ef6066fd2a386f7357bce7593c517588 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Fri, 26 Apr 2024 13:29:16 +0200 Subject: [PATCH 59/67] Correct variable naming. --- src/libcommonserver/io/iohelper_win.cpp | 90 ++++++++++++------------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index e9021e447..f665eb109 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -334,81 +334,81 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, log4cplus::Logger logger) noexcept { // Return false if we should try the fallback method - PACL pACL_old = nullptr; // Current ACL - PACL pACL_new = nullptr; // New ACL + PACL pACLold = nullptr; // Current ACL + PACL pACLnew = nullptr; // New ACL PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; - EXPLICIT_ACCESS ExplicitAccess; - ZeroMemory(&ExplicitAccess, sizeof(ExplicitAccess)); + EXPLICIT_ACCESS explicitAccess; + ZeroMemory(&explicitAccess, sizeof(explicitAccess)); - ExplicitAccess.grfAccessPermissions = permission; - ExplicitAccess.grfAccessMode = accessMode; - ExplicitAccess.grfInheritance = NO_INHERITANCE; - ExplicitAccess.Trustee.pMultipleTrustee = nullptr; - ExplicitAccess.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; - ExplicitAccess.Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; - ExplicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; - ExplicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; + explicitAccess.grfAccessPermissions = permission; + explicitAccess.grfAccessMode = accessMode; + explicitAccess.grfInheritance = NO_INHERITANCE; + explicitAccess.Trustee.pMultipleTrustee = nullptr; + explicitAccess.Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE; + explicitAccess.Trustee.TrusteeForm = Utility::_trustee.TrusteeForm; + explicitAccess.Trustee.TrusteeType = Utility::_trustee.TrusteeType; + explicitAccess.Trustee.ptstrName = Utility::_trustee.ptstrName; - std::wstring path_wstr = Path2WStr(path); - size_t pathw_len = path_wstr.length(); + std::wstring pathWstr = Path2WStr(path); + size_t pathLen = pathWstr.length(); - auto pathw_ptr = std::make_unique(pathw_len + 1); - path_wstr.copy(pathw_ptr.get(), pathw_len); + auto pathwPtr = std::make_unique(pathLen + 1); + pathWstr.copy(pathwPtr.get(), pathLen); - LPCWSTR pathw_c = path_wstr.c_str(); - LPWSTR pathw = pathw_ptr.get(); - pathw[pathw_len] = L'\0'; + LPCWSTR pathw_c = pathWstr.c_str(); + LPWSTR pathw = pathwPtr.get(); + pathw[pathLen] = L'\0'; - DWORD ValueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACL_old, + DWORD valueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACLold, nullptr, &pSecurityDescriptor); - ioError = dWordError2ioError(ValueReturned); - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); + ioError = dWordError2ioError(valueReturned); + if (valueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(valueReturned); LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() - << L" | DWORD error: " << ValueReturned); + << L" | DWORD error: " << valueReturned); LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + LocalFree(pACLnew); + // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { return true; } return false; } - ValueReturned = SetEntriesInAcl(1, &ExplicitAccess, pACL_old, &pACL_new); - ioError = dWordError2ioError(ValueReturned); - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); + valueReturned = SetEntriesInAcl(1, &explicitAccess, pACLold, &pACLnew); + ioError = dWordError2ioError(valueReturned); + if (valueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(valueReturned); LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " - << ValueReturned); + << valueReturned); LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + LocalFree(pACLnew); + // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { return true; } return false; } - if (!IsValidAcl(pACL_new)) { + if (!IsValidAcl(pACLnew)) { ioError = IoErrorUnknown; LOGW_WARN(logger, L"Invalid new ACL: " << Utility::formatSyncPath(path).c_str()); LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + LocalFree(pACLnew); + // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. return false; } - ValueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACL_new, nullptr); - ioError = dWordError2ioError(ValueReturned); - if (ValueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(ValueReturned); + valueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACLnew, nullptr); + ioError = dWordError2ioError(valueReturned); + if (valueReturned != ERROR_SUCCESS) { + ioError = dWordError2ioError(valueReturned); LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() - << L" | DWORD error: " << ValueReturned); + << L" | DWORD error: " << valueReturned); LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + LocalFree(pACLnew); + // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { return true; } @@ -417,8 +417,8 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); - LocalFree(pACL_new); - // pACL_old is a pointer to the ACL in the security descriptor, so it should not be freed. + LocalFree(pACLnew); + // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. return true; } From 7958bc934b4f37fe2c010ce98a8ef901d871b296 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 29 Apr 2024 11:48:33 +0200 Subject: [PATCH 60/67] Remove exists and set back the _isExpected error in get/set rights. --- src/libcommonserver/io/iohelper.h | 2 +- src/libcommonserver/io/iohelper_win.cpp | 56 ++++++++----------- .../propagation/executor/executorworker.cpp | 4 +- .../computefsoperationworker.cpp | 2 +- .../localfilesystemobserverworker.cpp | 10 +++- .../io/testchecksetgetrights.cpp | 51 ++++++++--------- 6 files changed, 58 insertions(+), 67 deletions(-) diff --git a/src/libcommonserver/io/iohelper.h b/src/libcommonserver/io/iohelper.h index 6a93457e8..c7d43b11e 100644 --- a/src/libcommonserver/io/iohelper.h +++ b/src/libcommonserver/io/iohelper.h @@ -267,7 +267,7 @@ struct IoHelper { \param exists is a boolean indicating whether the item exists. \return true if no unexpected error occurred, false otherwise. */ - static bool getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept; + static bool getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError &ioError) noexcept; //! Set the rights of the item indicated by `path`. /*! diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index f665eb109..8d44b480d 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -423,12 +423,13 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M return true; } -static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists, - log4cplus::Logger logger) noexcept { // Return false if we should try the fallback method - IoError ioError = IoErrorSuccess; +static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, bool &exec, IoError &ioError, + log4cplus::Logger logger) noexcept { // Always return false if ioError != IoErrorSuccess, caller + // should call _isExpectedError. + ioError = IoErrorSuccess; WCHAR szFilePath[MAX_PATH_LENGTH_WIN_LONG]; lstrcpyW(szFilePath, path.native().c_str()); - + bool exists = false; // Get security info PACL pfileACL = NULL; PSECURITY_DESCRIPTOR psecDesc = NULL; @@ -445,16 +446,13 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b write = false; exec = false; exists = false; - return true; // Expected error if access is denied } if (exists) { - LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() - << L" | DWORD error: " << result); - return false; // Unexpected error if the file exists but we can't get the security info (and it's not an access - // denied) + LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() << L", errDowrd= " + << result); } - return true; // Expected error if the file doesn't exist + return false; // Caller should call _isExpectedError } // Get rights for trustee @@ -468,8 +466,9 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b write = false; exec = false; exists = false; - LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: " << Utility::formatIoError(path, ioError).c_str()); - return true; // Expected error if access is denied + LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: " << Utility::formatIoError(path, ioError).c_str() + << L", errDowrd= " << result); + return false; // Caller should call _isExpectedError } if (result == ERROR_INVALID_SID) { // Access denied, try to force read control and get the rights @@ -481,9 +480,8 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (ioError != IoErrorSuccess) { - LOGW_INFO(logger, - L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " << result); - return false; // Unexpected error if we can't get the rights + LOGW_INFO(logger, L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L", errDowrd= " << result); + return false; // Caller should call _isExpectedError } bool readCtrl = (rights & READ_CONTROL) == READ_CONTROL; // Check if we have read control (needed to read the permissions) @@ -504,7 +502,7 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b write = false; exec = false; exists = false; - return true; // Expected error if access is denied + return false; // Expected error if access is denied } } @@ -515,16 +513,14 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b return true; } -bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { +bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError &ioError) noexcept { // !!! When Full control to file/directory is denied to the user, the function will return as if the file/directory does not exist. - read = false; write = false; exec = false; - exists = false; // Preferred method if (_getAndSetRightsMethod == 0 && Utility::_trustee.ptstrName) { - if (getRightsWindowsApi(path, read, write, exec, exists, logger())) { + if (getRightsWindowsApi(path, read, write, exec, ioError, logger()) || _isExpectedError(ioError)) { return true; } LOGW_WARN(logger(), L"Failed to get rights using Windows API, falling back to std::filesystem."); @@ -539,7 +535,6 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex } // Fallback method. - IoError ioError; ItemType itemType; const bool success = getItemType(path, itemType); ioError = itemType.ioError; @@ -549,9 +544,8 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex return false; } - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; + if (ioError != IoErrorSuccess) { + return _isExpectedError(ioError); } const bool isSymlink = itemType.linkType == LinkTypeSymlink; @@ -560,16 +554,10 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); ioError = stdError2ioError(ec); if (ioError != IoErrorSuccess) { - exists = ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; - } - LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); - return false; + return _isExpectedError(ioError); } - exists = true; read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); @@ -601,13 +589,15 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, grantedPermission |= FILE_GENERIC_EXECUTE; } bool res = false; - res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()); + res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); if (res) { - res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()); + res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()) || _isExpectedError(ioError); } + if (res) { return true; } + LOGW_WARN(logger(), L"Failed to set rights using Windows API, falling back to std::filesystem."); sentry_value_t event = sentry_value_new_event(); sentry_value_t exc = sentry_value_new_exception( diff --git a/src/libsyncengine/propagation/executor/executorworker.cpp b/src/libsyncengine/propagation/executor/executorworker.cpp index 27008ab4e..e07fd3d2d 100644 --- a/src/libsyncengine/propagation/executor/executorworker.cpp +++ b/src/libsyncengine/propagation/executor/executorworker.cpp @@ -1402,10 +1402,12 @@ bool ExecutorWorker::hasRight(SyncOpPtr syncOp, bool &exists) { bool readPermission = false; bool writePermission = false; bool execPermission = false; - if (!IoHelper::getRights(absoluteLocalFilePath, readPermission, writePermission, execPermission, exists)) { + IoError ioError = IoErrorSuccess; + if (!IoHelper::getRights(absoluteLocalFilePath, readPermission, writePermission, execPermission, ioError)) { LOGW_WARN(_logger, L"Error in Utility::getRights for path=" << Path2WStr(absoluteLocalFilePath).c_str()); return false; } + exists = ioError != IoErrorNoSuchFileOrDirectory; if (syncOp->targetSide() == ReplicaSideLocal) { switch (syncOp->type()) { diff --git a/src/libsyncengine/update_detection/file_system_observer/computefsoperationworker.cpp b/src/libsyncengine/update_detection/file_system_observer/computefsoperationworker.cpp index 43869bb13..6eefea9bc 100644 --- a/src/libsyncengine/update_detection/file_system_observer/computefsoperationworker.cpp +++ b/src/libsyncengine/update_detection/file_system_observer/computefsoperationworker.cpp @@ -799,7 +799,7 @@ ExitCode ComputeFSOperationWorker::checkIfOkToDelete(ReplicaSide side, const Syn bool readPermission = false; bool writePermission = false; bool execPermission = false; - if (!IoHelper::getRights(absolutePath, readPermission, writePermission, execPermission, exists)) { + if (!IoHelper::getRights(absolutePath, readPermission, writePermission, execPermission, ioError)) { LOGW_WARN(_logger, L"Error in Utility::getRights for path=" << Path2WStr(absolutePath).c_str()); setExitCause(ExitCauseFileAccessError); return ExitCodeSystemError; diff --git a/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.cpp b/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.cpp index be70f14d1..5f5447711 100644 --- a/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.cpp +++ b/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.cpp @@ -161,11 +161,12 @@ void LocalFileSystemObserverWorker::changesDetected(const std::list Date: Mon, 29 Apr 2024 12:00:07 +0200 Subject: [PATCH 61/67] Implement with isExpected Error on mac and Linux. --- src/libcommonserver/io/iohelper_linux.cpp | 13 +++++-------- src/libcommonserver/io/iohelper_mac.cpp | 19 ++++++++----------- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/libcommonserver/io/iohelper_linux.cpp b/src/libcommonserver/io/iohelper_linux.cpp index 9529cdcef..7ac71efb1 100644 --- a/src/libcommonserver/io/iohelper_linux.cpp +++ b/src/libcommonserver/io/iohelper_linux.cpp @@ -116,25 +116,22 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return true; } -bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { +bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError &ioError) noexcept { read = false; write = false; exec = false; - exists = false; std::error_code ec; std::filesystem::perms perms = std::filesystem::status(path, ec).permissions(); if (ec.value() != 0) { - exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + ioError = stdError2ioError(ec); if (!exists) { - // Path doesn't exist - return true; + ioError = IoErrorNoSuchFileOrDirectory; } - - return false; + return _isExpectedError(ioError); } - exists = true; read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); write = ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index 1b68b86bf..7febe5f4d 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -213,11 +213,10 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return true; } -bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, bool &exists) noexcept { +bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError ioError) noexcept { read = false; write = false; exec = false; - exists = false; ItemType itemType; const bool success = getItemType(path, itemType); @@ -225,9 +224,9 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex LOGW_WARN(logger(), L"Failed to get item type: " << Utility::formatIoError(path, itemType.ioError).c_str()); return false; } - exists = itemType.ioError != IoErrorNoSuchFileOrDirectory; - if (!exists) { - return true; + ioError = itemType.ioError; + if (ioError != IoErrorSuccess) { + return _isExpectedError(ioError); } const bool isSymlink = itemType.linkType == LinkTypeSymlink; @@ -235,17 +234,15 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex std::filesystem::perms perms = isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); if (ec.value() != 0) { - exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + ioError = stdError2ioError(ec); if (!exists) { - // Path doesn't exist - return true; + ioError = IoErrorNoSuchFileOrDirectory; } - LOGW_WARN(logger(), L"Failed to get permissions: " << Utility::formatStdError(path, ec).c_str()); - return false; + return _isExpectedError(ioError); } - exists = true; read = ((perms & std::filesystem::perms::owner_read) != std::filesystem::perms::none); write = isLocked(path) ? false : ((perms & std::filesystem::perms::owner_write) != std::filesystem::perms::none); exec = ((perms & std::filesystem::perms::owner_exec) != std::filesystem::perms::none); From 6052e47a8470e3197e06d8924aa8a8d25f893750 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Mon, 29 Apr 2024 12:04:12 +0200 Subject: [PATCH 62/67] bug fix on mac. --- src/libcommonserver/io/iohelper_mac.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index 7febe5f4d..c2efa3fc8 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -213,7 +213,7 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra return true; } -bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError ioError) noexcept { +bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &exec, IoError &ioError) noexcept { read = false; write = false; exec = false; From 8076d7f496c98e3308e820fb6b207f68e3544ada Mon Sep 17 00:00:00 2001 From: Herve Eruam <82284536+herve-er@users.noreply.github.com> Date: Tue, 30 Apr 2024 11:22:39 +0200 Subject: [PATCH 63/67] Apply suggestions from code review Co-authored-by: Luc Guyot <162997198+luc-guyot-infomaniak@users.noreply.github.com> --- src/libcommonserver/io/iohelper_linux.cpp | 4 ++-- src/libcommonserver/io/iohelper_mac.cpp | 4 ++-- src/libcommonserver/io/iohelper_win.cpp | 6 +++--- src/libsyncengine/propagation/executor/executorworker.cpp | 2 +- test/libcommonserver/io/testchecksetgetrights.cpp | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libcommonserver/io/iohelper_linux.cpp b/src/libcommonserver/io/iohelper_linux.cpp index 7ac71efb1..4e1639e02 100644 --- a/src/libcommonserver/io/iohelper_linux.cpp +++ b/src/libcommonserver/io/iohelper_linux.cpp @@ -123,8 +123,8 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex std::error_code ec; std::filesystem::perms perms = std::filesystem::status(path, ec).permissions(); - if (ec.value() != 0) { - bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + if (ec) { + const bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); ioError = stdError2ioError(ec); if (!exists) { ioError = IoErrorNoSuchFileOrDirectory; diff --git a/src/libcommonserver/io/iohelper_mac.cpp b/src/libcommonserver/io/iohelper_mac.cpp index c2efa3fc8..27b01b762 100644 --- a/src/libcommonserver/io/iohelper_mac.cpp +++ b/src/libcommonserver/io/iohelper_mac.cpp @@ -233,8 +233,8 @@ bool IoHelper::getRights(const SyncPath &path, bool &read, bool &write, bool &ex std::error_code ec; std::filesystem::perms perms = isSymlink ? std::filesystem::symlink_status(path, ec).permissions() : std::filesystem::status(path, ec).permissions(); - if (ec.value() != 0) { - bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); + if (ec) { + const bool exists = (ec.value() != static_cast(std::errc::no_such_file_or_directory)); ioError = stdError2ioError(ec); if (!exists) { ioError = IoErrorNoSuchFileOrDirectory; diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 8d44b480d..986578fdc 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -449,7 +449,7 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (exists) { - LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() << L", errDowrd= " + LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() << L", DWORD error: " << result); } return false; // Caller should call _isExpectedError @@ -467,7 +467,7 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b exec = false; exists = false; LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: " << Utility::formatIoError(path, ioError).c_str() - << L", errDowrd= " << result); + << L", DWORD error: " << result); return false; // Caller should call _isExpectedError } @@ -480,7 +480,7 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (ioError != IoErrorSuccess) { - LOGW_INFO(logger, L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L", errDowrd= " << result); + LOGW_INFO(logger, L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L", DWORD error: " << result); return false; // Caller should call _isExpectedError } diff --git a/src/libsyncengine/propagation/executor/executorworker.cpp b/src/libsyncengine/propagation/executor/executorworker.cpp index e07fd3d2d..a9408830d 100644 --- a/src/libsyncengine/propagation/executor/executorworker.cpp +++ b/src/libsyncengine/propagation/executor/executorworker.cpp @@ -1404,7 +1404,7 @@ bool ExecutorWorker::hasRight(SyncOpPtr syncOp, bool &exists) { bool execPermission = false; IoError ioError = IoErrorSuccess; if (!IoHelper::getRights(absoluteLocalFilePath, readPermission, writePermission, execPermission, ioError)) { - LOGW_WARN(_logger, L"Error in Utility::getRights for path=" << Path2WStr(absoluteLocalFilePath).c_str()); + LOGW_WARN(_logger, L"Error in Utility::getRights: " << Utility::formatSyncPath(absoluteLocalFilePath).c_str()); return false; } exists = ioError != IoErrorNoSuchFileOrDirectory; diff --git a/test/libcommonserver/io/testchecksetgetrights.cpp b/test/libcommonserver/io/testchecksetgetrights.cpp index b52236690..7d5a35c90 100644 --- a/test/libcommonserver/io/testchecksetgetrights.cpp +++ b/test/libcommonserver/io/testchecksetgetrights.cpp @@ -39,7 +39,7 @@ void TestIo::testCheckSetAndGetRights() { #endif const TemporaryDirectory temporaryDirectory; - // Test if the rights are correctly set and get on a directory + // Test if the rights are correctly set on a directory and if it can be retrieved successfully. { const SyncPath path = temporaryDirectory.path / "changePerm"; From 5b1ece6f3db1e3c4fb9b55758d5044286324abe4 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 30 Apr 2024 11:47:24 +0200 Subject: [PATCH 64/67] Correct setRightsWindowsApi behaviour in case of error and free(pDesc in case of access denied. --- src/libcommonserver/io/iohelper_win.cpp | 41 ++++++++++--------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 986578fdc..116008dff 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -332,10 +332,10 @@ bool IoHelper::checkIfFileIsDehydrated(const SyncPath &itemPath, bool &isDehydra } static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_MODE accessMode, IoError &ioError, - log4cplus::Logger logger) noexcept { // Return false if we should try the fallback method - - PACL pACLold = nullptr; // Current ACL - PACL pACLnew = nullptr; // New ACL + log4cplus::Logger logger) noexcept { // Always return false if ioError != IoErrorSuccess, caller + // should call _isExpectedError + PACL pACLold = nullptr; // Current ACL + PACL pACLnew = nullptr; // New ACL PSECURITY_DESCRIPTOR pSecurityDescriptor = nullptr; EXPLICIT_ACCESS explicitAccess; ZeroMemory(&explicitAccess, sizeof(explicitAccess)); @@ -369,9 +369,6 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. - if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { - return true; - } return false; } @@ -384,9 +381,6 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. - if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { - return true; - } return false; } @@ -409,13 +403,9 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. - if (ioError == IoErrorNoSuchFileOrDirectory || ioError == IoErrorAccessDenied) { - return true; - } return false; } - LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -449,8 +439,8 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (exists) { - LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() << L", DWORD error: " - << result); + LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() + << L", DWORD error: " << result); } return false; // Caller should call _isExpectedError } @@ -472,11 +462,13 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (result == ERROR_INVALID_SID) { // Access denied, try to force read control and get the rights - setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger); - GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); - setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger); - ioError = dWordError2ioError(result); + if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger)) { + GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); + LocalFree(psecDesc); + result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger); + ioError = dWordError2ioError(result); + } } if (ioError != IoErrorSuccess) { @@ -506,7 +498,6 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } } - exists = true; read = (rights & FILE_GENERIC_READ) == FILE_GENERIC_READ; write = (rights & FILE_GENERIC_WRITE) == FILE_GENERIC_WRITE; exec = (rights & FILE_EXECUTE) == FILE_EXECUTE; @@ -589,9 +580,11 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, grantedPermission |= FILE_GENERIC_EXECUTE; } bool res = false; - res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); + res = + setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); if (res) { - res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()) || _isExpectedError(ioError); + res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()) || + _isExpectedError(ioError); } if (res) { From a4fd7e36b99627986f1103db69d5d23a69f4a40c Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 30 Apr 2024 11:47:35 +0200 Subject: [PATCH 65/67] Complete previous commit. --- src/libcommonserver/io/iohelper_win.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 116008dff..9835e49b4 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -464,8 +464,9 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b if (result == ERROR_INVALID_SID) { // Access denied, try to force read control and get the rights if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger)) { GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - LocalFree(psecDesc); result = GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + LocalFree(psecDesc); + setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger); ioError = dWordError2ioError(result); } @@ -481,8 +482,8 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b if (setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::GRANT_ACCESS, ioError, logger)) { // Try to force read control GetNamedSecurityInfo(szFilePath, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, NULL, NULL, &pfileACL, NULL, &psecDesc); - LocalFree(psecDesc); GetEffectiveRightsFromAcl(pfileACL, &Utility::_trustee, &rights); + LocalFree(psecDesc); setRightsWindowsApi(path, READ_CONTROL, ACCESS_MODE::REVOKE_ACCESS, ioError, logger); // Revoke read control after reading the permissions readCtrl = (rights & READ_CONTROL) == READ_CONTROL; From 60c971646a9768e7c50e8b729c89455051814d2f Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Tue, 30 Apr 2024 11:58:02 +0200 Subject: [PATCH 66/67] Minor code refactor. --- src/libcommonserver/io/iohelper_win.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index 9835e49b4..e84b8b363 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -580,9 +580,7 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } else { grantedPermission |= FILE_GENERIC_EXECUTE; } - bool res = false; - res = - setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); + bool res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); if (res) { res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()) || _isExpectedError(ioError); From 27625cf3218657e6318dece0b3282dc77dd60a33 Mon Sep 17 00:00:00 2001 From: Herve_eruam Date: Wed, 1 May 2024 09:11:45 +0200 Subject: [PATCH 67/67] Update error message to show only DWORD when dealing with win API. --- src/libcommonserver/io/iohelper_win.cpp | 51 +++++++++++++------------ 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/libcommonserver/io/iohelper_win.cpp b/src/libcommonserver/io/iohelper_win.cpp index e84b8b363..dcf2fe5ee 100644 --- a/src/libcommonserver/io/iohelper_win.cpp +++ b/src/libcommonserver/io/iohelper_win.cpp @@ -359,25 +359,25 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M LPWSTR pathw = pathwPtr.get(); pathw[pathLen] = L'\0'; - DWORD valueReturned = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACLold, - nullptr, &pSecurityDescriptor); - ioError = dWordError2ioError(valueReturned); - if (valueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(valueReturned); - LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() - << L" | DWORD error: " << valueReturned); + DWORD result = GetNamedSecurityInfo(pathw_c, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, &pACLold, nullptr, + &pSecurityDescriptor); + ioError = dWordError2ioError(result); + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOGW_WARN(logger, L"Error in GetNamedSecurityInfo: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" << result + << L"'"); LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. return false; } - valueReturned = SetEntriesInAcl(1, &explicitAccess, pACLold, &pACLnew); - ioError = dWordError2ioError(valueReturned); - if (valueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(valueReturned); - LOGW_WARN(logger, L"Error in SetEntriesInAcl: " << Utility::formatIoError(path, ioError).c_str() << L" | DWORD error: " - << valueReturned); + result = SetEntriesInAcl(1, &explicitAccess, pACLold, &pACLnew); + ioError = dWordError2ioError(result); + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOGW_WARN(logger, + L"Error in SetEntriesInAcl: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" << result << L"'"); LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -394,12 +394,12 @@ static bool setRightsWindowsApi(const SyncPath &path, DWORD permission, ACCESS_M return false; } - valueReturned = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACLnew, nullptr); - ioError = dWordError2ioError(valueReturned); - if (valueReturned != ERROR_SUCCESS) { - ioError = dWordError2ioError(valueReturned); - LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: " << Utility::formatIoError(path, ioError).c_str() - << L" | DWORD error: " << valueReturned); + result = SetNamedSecurityInfo(pathw, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, nullptr, nullptr, pACLnew, nullptr); + ioError = dWordError2ioError(result); + if (result != ERROR_SUCCESS) { + ioError = dWordError2ioError(result); + LOGW_WARN(logger, L"Error in SetNamedSecurityInfo: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" << result + << L"'"); LocalFree(pSecurityDescriptor); LocalFree(pACLnew); // pACLold is a pointer to the ACL in the security descriptor, so it should not be freed. @@ -439,8 +439,8 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (exists) { - LOGW_INFO(logger, L"GetNamedSecurityInfo failed: " << Utility::formatIoError(path, ioError).c_str() - << L", DWORD error: " << result); + LOGW_INFO(logger, L"GetNamedSecurityInfo failed: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" + << result << L"'"); } return false; // Caller should call _isExpectedError } @@ -456,8 +456,8 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b write = false; exec = false; exists = false; - LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: " << Utility::formatIoError(path, ioError).c_str() - << L", DWORD error: " << result); + LOGW_INFO(logger, L"GetEffectiveRightsFromAcl failed: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" + << result << L"'"); return false; // Caller should call _isExpectedError } @@ -473,7 +473,7 @@ static bool getRightsWindowsApi(const SyncPath &path, bool &read, bool &write, b } if (ioError != IoErrorSuccess) { - LOGW_INFO(logger, L"Unexpected error: " << Utility::formatIoError(path, ioError).c_str() << L", DWORD error: " << result); + LOGW_INFO(logger, L"Unexpected error: path='" << Utility::formatSyncPath(path) << L"',DWORD err='" << result << L"'"); return false; // Caller should call _isExpectedError } @@ -580,7 +580,8 @@ bool IoHelper::setRights(const SyncPath &path, bool read, bool write, bool exec, } else { grantedPermission |= FILE_GENERIC_EXECUTE; } - bool res = setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); + bool res = + setRightsWindowsApi(path, grantedPermission, ACCESS_MODE::SET_ACCESS, ioError, logger()) || _isExpectedError(ioError); if (res) { res &= setRightsWindowsApi(path, deniedPermission, ACCESS_MODE::DENY_ACCESS, ioError, logger()) || _isExpectedError(ioError);