Add AnnounceUpgrade test to the module boinccas module tests. #6635

AenBleidd · 2025-11-01T22:14:10Z

No description provided.

Copilot

Pull Request Overview

This PR adds unit tests for the BOINC Custom Action (boinccas) installer component, specifically testing the CAAnnounceUpgrade function. The tests use a helper library to load the DLL dynamically and mock MSI database operations.

Adds unit test infrastructure for testing boinccas installer custom actions
Integrates Windows Installer Library (WIL) for RAII resource management
Configures conditional compilation to enable boinccas tests in CI

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
win_build/unittests.vcxproj	Adds boinccas test files, MSI-related library dependencies, and conditional preprocessor definition
win_build/installer.vcxproj	Refactors dependency organization by consolidating common dependencies
win_build/boinc.sln	Adds installer project as dependency for unittests and reorders dependencies
tests/unit-tests/boinccas/test_boinccas_CAAnnounceUpgrade.cpp	Implements unit test for AnnounceUpgrade custom action
tests/unit-tests/boinccas/boinccas_helper.h	Declares helper functions and classes for MSI and registry operations
tests/unit-tests/boinccas/boinccas_helper.cpp	Implements MSI database and registry manipulation helpers
3rdParty/vcpkg_ports/configs/msbuild/vcpkg.json	Adds WIL dependency for Windows resource management
.github/workflows/windows.yml	Enables BOINCCAS_TEST preprocessor flag in CI builds

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T22:27:43Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+void MsiHelper::createTable(const std::string_view& sql_create) {
+    MSIHANDLE hView;
+    auto result = MsiDatabaseOpenView(hMsi, sql_create.data(), &hView);


The MSIHANDLE hView is not being closed on all error paths. If MsiDatabaseOpenView succeeds but MsiViewExecute fails (lines 52-56), or if MsiViewExecute succeeds but MsiViewClose fails (lines 57-61), the handle will leak. Wrap hView with a RAII wrapper or use a unique_ptr with a custom deleter, or ensure MsiCloseHandle(hView) is called before throwing exceptions.

Copilot · 2025-11-01T22:27:43Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    MSIHANDLE hView;
+    auto result = MsiDatabaseOpenView(hMsi, sql_insert.data(), &hView);


The MSIHANDLE hView is not being closed on error paths in the loop (lines 82-108). If any operation within the loop throws an exception, hView will leak. Wrap hView with a RAII wrapper or ensure it's closed in all error paths.

Copilot · 2025-11-01T22:27:44Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+            std::to_string(result));
+    }
+    for (const auto& record : properties) {
+        const auto hRecord = MsiCreateRecord(static_cast<UINT>(2));


[nitpick] The explicit cast static_cast<UINT>(2) is unnecessary since 2 can be directly passed as a UINT literal (2u) or the literal 2 will be implicitly converted. Consider using MsiCreateRecord(2) for cleaner code.

Suggested change

const auto hRecord = MsiCreateRecord(static_cast<UINT>(2));

const auto hRecord = MsiCreateRecord(2);

Copilot

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T23:19:01Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);
+    RegCloseKey(hKey);


The RegDeleteKey function is being called while a handle to the key is still open. The key handle should be closed before attempting to delete the key. Additionally, RegDeleteKey only deletes empty keys; use RegDeleteTree to delete a key with subkeys, or ensure the key is empty before deletion.

Suggested change

RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);

RegCloseKey(hKey);

RegCloseKey(hKey);

RegDeleteTree(HKEY_LOCAL_MACHINE, registryKey);

Copilot · 2025-11-01T23:19:01Z

clientsetup/win/boinccas.cpp

        NULL
    );
-	if (lReturnValue != ERROR_SUCCESS) return ERROR_INSTALL_FAILURE;
+	if (lReturnValue != ERROR_SUCCESS) return ERROR_INSTALL_FAILURE + 3;


Using magic numbers (3, 4) added to ERROR_INSTALL_FAILURE reduces code readability. Consider defining named constants like ERROR_REGISTRY_CREATE_FAILED = ERROR_INSTALL_FAILURE + 3 to make the error codes self-documenting.

Copilot · 2025-11-01T23:19:02Z

clientsetup/win/boinccas.cpp


 	RegCloseKey(hkSetupHive);
-	if (lReturnValue != ERROR_SUCCESS) return ERROR_INSTALL_FAILURE;
+	if (lReturnValue != ERROR_SUCCESS) return ERROR_INSTALL_FAILURE + 4;


Using magic numbers (3, 4) added to ERROR_INSTALL_FAILURE reduces code readability. Consider defining named constants like ERROR_REGISTRY_SET_FAILED = ERROR_INSTALL_FAILURE + 4 to make the error codes self-documenting.

Copilot · 2025-11-01T23:19:02Z

clientsetup/win/boinccas.cpp

            strMessage.c_str()
        );
-        return ERROR_INSTALL_FAILURE;
+        return ERROR_INSTALL_FAILURE + 1;


Using magic numbers (1, 2) added to ERROR_INSTALL_FAILURE reduces code readability. Consider defining named constants like ERROR_PROPERTY_GET_FAILED = ERROR_INSTALL_FAILURE + 1 to make the error codes self-documenting.

Copilot · 2025-11-01T23:19:02Z

clientsetup/win/boinccas.cpp

        );
        if ( lpszBuffer ) free( lpszBuffer );
-        return ERROR_INSTALL_FAILURE;
+        return ERROR_INSTALL_FAILURE + 2;


Using magic numbers (1, 2) added to ERROR_INSTALL_FAILURE reduces code readability. Consider defining named constants like ERROR_PROPERTY_BUFFER_FAILED = ERROR_INSTALL_FAILURE + 2 to make the error codes self-documenting.

Copilot

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-01T23:56:41Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+constexpr auto registryKey =
+"SOFTWARE\\Space Sciences Laboratory, U.C. Berkeley\\BOINC Setup";


The registry key string literal is split across two lines, which may cause compilation issues. Consider placing the entire string on line 124 or using proper line continuation syntax.

Copilot

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

tests/unit-tests/boinccas/boinccas_helper.cpp:1

The createPropertiesTable() method is called in init() at line 40, but createPropertiesTable is declared as a public method in the header. Since init() is now being used and creates the Properties table during initialization, the test at line 55 of test_boinccas_CAAnnounceUpgrade.cpp should not call createPropertiesTable() again as it will fail trying to create an already-existing table.

// This file is part of BOINC.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T05:02:43Z

tests/unit-tests/boinccas/test_boinccas_CAAnnounceUpgrade.cpp

+            if (hMsi != 0) {
+                MsiCloseHandle(hMsi);
+            }


This MSI handle cleanup at line 55-57 duplicates the cleanup logic already present in the destructor (lines 35-37). The handle will be closed again in the destructor after this test completes, resulting in a double-close. Remove this redundant cleanup block or set hMsi to 0 after closing it.

Copilot · 2025-11-02T05:02:43Z

tests/unit-tests/boinccas/boinccas_helper.h

+        const std::vector<std::pair<std::string, std::string>>& properties);
+    std::string getMsiHandle() const {


Missing include directive for <vector>. The method signature uses std::vector but the required header is not included.

Copilot · 2025-11-02T05:02:44Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    }
+    result = MsiSummaryInfoSetProperty(hSummaryInfo, 9, VT_LPSTR, 0, nullptr,
+        "{2C4296B7-9E88-4CD8-9FC6-26CE7B053ED1}");
+    if (result != ERROR_SUCCESS) {


The return value of RegDeleteKey is not checked. If the key deletion fails, the function silently continues. Consider checking the result for proper error handling or logging.

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T05:39:18Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    if (result != ERROR_SUCCESS) {
+        throw std::runtime_error("Error closing view: " +
+            std::to_string(result));
+    }


Resource leak: hView handle is not closed with MsiCloseHandle() after MsiViewClose(). According to MSI documentation, both MsiViewClose() and MsiCloseHandle() must be called on view handles. Add MsiCloseHandle(hView) after line 78.

Suggested change

}

}

MsiCloseHandle(hView);

Copilot · 2025-11-02T05:39:19Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    MSIHANDLE hView;
+    auto result = MsiDatabaseOpenView(hMsi, sql_insert.data(), &hView);
+    if (result != ERROR_SUCCESS) {
+        throw std::runtime_error("Error creating view: " +
+            std::to_string(result));
+    }


Resource leak: hView handle is not closed with MsiCloseHandle() if an exception is thrown in the loop (lines 104-130) or after MsiViewClose() succeeds. Add MsiCloseHandle(hView) after line 136, similar to how hRecord is handled with MsiCloseHandle at line 125.

Copilot · 2025-11-02T05:39:19Z

tests/unit-tests/boinccas/test_boinccas_CAAnnounceUpgrade.cpp

+        EXPECT_NE(expectedVersion, getRegistryValue("UpgradingTo"));
+
+        if (hMsi != 0) {
+            MsiCloseHandle(hMsi);


The hMsi handle should be set to 0 after closing to prevent double-close in the destructor. Add 'hMsi = 0;' after line 56.

Suggested change

MsiCloseHandle(hMsi);

MsiCloseHandle(hMsi);

hMsi = 0;

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T07:05:59Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+            std::to_string(result));
+    }
+    result = MsiViewExecute(hView, 0);
+    if (result != ERROR_SUCCESS) {


Resource leak: hView is not closed if MsiViewExecute fails. The handle created by MsiDatabaseOpenView at line 64 should be closed with MsiCloseHandle(hView) before throwing the exception at line 71.

Suggested change

if (result != ERROR_SUCCESS) {

if (result != ERROR_SUCCESS) {

MsiCloseHandle(hView);

Copilot · 2025-11-02T07:05:59Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    MSIHANDLE hView;
+    auto result = MsiDatabaseOpenView(hMsi, sql_insert.data(), &hView);
+    if (result != ERROR_SUCCESS) {
+        throw std::runtime_error("Error creating view: " +
+            std::to_string(result));
+    }
+    for (const auto& record : properties) {
+        const auto hRecord = MsiCreateRecord(2);
+        if (hRecord == 0) {
+            throw std::runtime_error("Failed to create record");
+        }
+        result = MsiRecordSetString(hRecord, 1, record.first.c_str());
+        if (result != ERROR_SUCCESS) {
+            throw std::runtime_error("Failed to set record, errorcode: " +
+                std::to_string(result));
+        }
+        result = MsiRecordSetString(hRecord, 2, record.second.c_str());
+        if (result != ERROR_SUCCESS) {
+            throw std::runtime_error("Failed to set record, errorcode: " +
+                std::to_string(result));
+        }


Resource leak: hView is not closed if an exception is thrown within the loop (lines 107, 112, 117). Consider using RAII wrappers or adding proper cleanup in exception paths before throwing.

Copilot · 2025-11-02T07:05:59Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+        if (result != ERROR_SUCCESS) {
+            throw std::runtime_error("Failed to set record, errorcode: " +
+                std::to_string(result));
+        }
+        result = MsiRecordSetString(hRecord, 2, record.second.c_str());
+        if (result != ERROR_SUCCESS) {


Resource leak: hRecord is not closed if MsiRecordSetString fails at line 110. The handle created at line 105 should be closed with MsiCloseHandle(hRecord) before throwing the exception at line 112.

Suggested change

if (result != ERROR_SUCCESS) {

throw std::runtime_error("Failed to set record, errorcode: " +

std::to_string(result));

}

result = MsiRecordSetString(hRecord, 2, record.second.c_str());

if (result != ERROR_SUCCESS) {

if (result != ERROR_SUCCESS) {

MsiCloseHandle(hRecord);

throw std::runtime_error("Failed to set record, errorcode: " +

std::to_string(result));

}

result = MsiRecordSetString(hRecord, 2, record.second.c_str());

if (result != ERROR_SUCCESS) {

MsiCloseHandle(hRecord);

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T07:33:53Z

tests/unit-tests/boinccas/test_boinccas_CAAnnounceUpgrade.cpp

+            }
+        }
+        AnnounceUpgradeFn hFunc = nullptr;
+        MSIHANDLE hMsi = 0;


The msiHelper member variable is instantiated before hDll and hFunc are initialized in the constructor. Since MsiHelper creates an MSI database during construction and the test fixture's constructor then calls load_function_from_boinccas which may require the DLL to be present, this ordering could lead to issues. Consider moving msiHelper instantiation to after the DLL loading, or use a smart pointer to delay its construction.

Copilot · 2025-11-02T07:33:54Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    HKEY hKey = nullptr;
+    const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, registryKey, 0,
+        KEY_WRITE, &hKey);
+    if (openResult != ERROR_SUCCESS) {
+        return;
+    }
+    RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);
+    RegCloseKey(hKey);


The order of operations is incorrect. RegDeleteKey is called while hKey is still open, and then RegCloseKey is called on a potentially invalid handle. The key should be closed before attempting to delete it. Additionally, RegDeleteKey should use hKey as the first parameter, not HKEY_LOCAL_MACHINE, since the key is already opened.

Suggested change

HKEY hKey = nullptr;

const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, registryKey, 0,

KEY_WRITE, &hKey);

if (openResult != ERROR_SUCCESS) {

return;

}

RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);

RegCloseKey(hKey);

// To delete a registry key, open its parent and call RegDeleteKey with the subkey name.

constexpr auto parentKeyPath = "SOFTWARE\\Space Sciences Laboratory, U.C. Berkeley";

constexpr auto subKeyName = "BOINC Setup";

HKEY hParentKey = nullptr;

const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, parentKeyPath, 0,

KEY_WRITE, &hParentKey);

if (openResult != ERROR_SUCCESS) {

return;

}

RegCloseKey(hParentKey); // Close before deleting

// Reopen parent for deletion (RegDeleteKey requires open parent)

if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, parentKeyPath, 0, KEY_WRITE, &hParentKey) == ERROR_SUCCESS) {

RegDeleteKey(hParentKey, subKeyName);

RegCloseKey(hParentKey);

}

Copilot

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/unit-tests/boinccas/test_boinccas_CAAnnounceUpgrade.cpp:1

The preprocessor directive should be #ifdef BOINCCAS_TEST instead of #ifndef BOINCCAS_TEST. With #ifndef, tests are excluded when BOINCCAS_TEST is defined, which is opposite to the intended behavior. The build sets -p:BOINCCAS_TEST=true to enable these tests.

// This file is part of BOINC.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull Request Overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T17:19:12Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+        std::filesystem::remove(std::filesystem::current_path() / msiName);
+    }
+    catch (const std::exception& ex) {
+        throw std::runtime_error( "Failed to remove existing MSI file: " +


Extra space after opening parenthesis. Should be std::runtime_error( without the space.

Suggested change

throw std::runtime_error( "Failed to remove existing MSI file: " +

throw std::runtime_error("Failed to remove existing MSI file: " +

Copilot · 2025-11-02T17:19:12Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    try {
+        std::filesystem::remove(std::filesystem::current_path() / msiName);
+    }
+    catch (const std::exception& ex) {
+        throw std::runtime_error( "Failed to remove existing MSI file: " +
+            std::string(ex.what()));
+    }


Rethrowing exceptions in cleanup code can mask the original issue. The cleanup() method is called from the destructor (line 30), where exceptions should not be thrown. Consider using std::filesystem::remove with the std::error_code overload instead of the throwing version, or catch and log the error without rethrowing.

Suggested change

try {

std::filesystem::remove(std::filesystem::current_path() / msiName);

}

catch (const std::exception& ex) {

throw std::runtime_error( "Failed to remove existing MSI file: " +

std::string(ex.what()));

}

std::error_code ec;

std::filesystem::remove(std::filesystem::current_path() / msiName, ec);

if (ec) {

// Optionally log the error, but do not throw

OutputDebugStringA(("Failed to remove existing MSI file: " + ec.message()).c_str());

}

Copilot · 2025-11-02T17:19:12Z

clientsetup/win/CAAnnounceUpgrade.h

    CAAnnounceUpgrade(MSIHANDLE hMSIHandle);
-    ~CAAnnounceUpgrade();
-    virtual UINT OnExecution();
+    UINT OnExecution() override final;


[nitpick] Using both override and final is redundant for documentation purposes. While technically correct, final implies override, so using both is unnecessary. Consider using only final or keeping just override if the method might be overridden in derived classes (though that seems unlikely here).

Suggested change

UINT OnExecution() override final;

UINT OnExecution() final;

Copilot

Pull Request Overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-03T02:52:52Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+        std::filesystem::remove(std::filesystem::current_path() / msiName);
+    }
+    catch (const std::exception& ex) {
+        throw std::runtime_error( "Failed to remove existing MSI file: " +


Extra space after opening parenthesis in throw statement. Should be 'throw std::runtime_error("Failed' without the space.

Copilot · 2025-11-03T02:52:52Z

tests/unit-tests/boinccas/test_boinccas_CACleanupOldBinaries.cpp

+    TEST_F(test_boinccas_CACleanupOldBinaries,
+        CleanupOldBinaries_NonExistent_INSTALLDIR_Directory) {
+        const auto dir =
+            std::filesystem::current_path() /= "non_existent_directory";


Using compound assignment operator '/=' modifies the current_path() temporary, but the result is not what's intended. This should use the '/' operator instead: 'std::filesystem::current_path() / "non_existent_directory"' without the '=' to create a new path without modifying anything.

Suggested change

std::filesystem::current_path() /= "non_existent_directory";

std::filesystem::current_path() / "non_existent_directory";

Copilot · 2025-11-03T02:52:52Z

tests/unit-tests/boinccas/test_boinccas_CACleanupOldBinaries.cpp

+
+    TEST_F(test_boinccas_CACleanupOldBinaries,
+        CleanupOldBinaries_Empty_INSTALLDIR_Directory) {
+        testDir = std::filesystem::current_path() /= "empty";


Using compound assignment operator '/=' modifies the current_path() temporary. This should use the '/' operator instead: 'std::filesystem::current_path() / "empty"' without the '='.

Suggested change

testDir = std::filesystem::current_path() /= "empty";

testDir = std::filesystem::current_path() / "empty";

Copilot · 2025-11-03T02:52:52Z

tests/unit-tests/boinccas/test_boinccas_CACleanupOldBinaries.cpp

+
+    TEST_F(test_boinccas_CACleanupOldBinaries,
+        CleanupOldBinaries_NonEmpty_INSTALLDIR_Directory_RemoveAllListedFiles) {
+        testDir = std::filesystem::current_path() /= "non_empty";


Using compound assignment operator '/=' modifies the current_path() temporary. This should use the '/' operator instead: 'std::filesystem::current_path() / "non_empty"' without the '='.

Copilot · 2025-11-03T02:52:52Z

tests/unit-tests/boinccas/test_boinccas_CACleanupOldBinaries.cpp

+
+    TEST_F(test_boinccas_CACleanupOldBinaries,
+        CleanupOldBinaries_NonEmpty_INSTALLDIR_Directory_KeepUnListedFiles) {
+        testDir = std::filesystem::current_path() /= "non_empty";


Using compound assignment operator '/=' modifies the current_path() temporary. This should use the '/' operator instead: 'std::filesystem::current_path() / "non_empty"' without the '='.

Copilot

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T04:40:43Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    HKEY hKey = nullptr;
+    const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, registryKey, 0,
+        KEY_WRITE, &hKey);
+    if (openResult != ERROR_SUCCESS) {
+        return;
+    }
+    RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);
+    RegCloseKey(hKey);


The key handle is closed after attempting to delete the key, but RegDeleteKey should be called with the parent key handle (HKEY_LOCAL_MACHINE), not the opened subkey handle. Also, the opened key handle should be closed before attempting deletion. The current code may leak the handle if RegDeleteKey succeeds.

Suggested change

HKEY hKey = nullptr;

const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, registryKey, 0,

KEY_WRITE, &hKey);

if (openResult != ERROR_SUCCESS) {

return;

}

RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);

RegCloseKey(hKey);

RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);

Copilot · 2025-11-04T04:40:43Z

tests/unit-tests/boinccas/test_boinccas_CACreateAcctMgrLoginFile.cpp

+    //TEST_F(test_boinccas_CACreateAcctMgrLoginFile,
+    //    Empty_DATADIR_Directory_And_ACCTMGR_LOGIN_Property_Set) {
+    //    PMSIHANDLE hMsi;
+    //    const auto dir = std::filesystem::current_path() /= "test_data";
+    //    msiHelper.insertProperties({
+    //    {"DATADIR", dir.string().c_str()},
+    //    {"ACCTMGR_LOGIN", "testuser"}
+    //        });
+    //    const auto result = MsiOpenPackage(msiHelper.getMsiHandle().c_str(),
+    //        &hMsi);
+    //    ASSERT_EQ(0u, result);
+
+    //    EXPECT_EQ(0u, hFunc(hMsi));
+    //}


This large commented-out test case should either be uncommented and fixed (if intended for future use), or removed entirely to reduce code clutter.

Suggested change

//TEST_F(test_boinccas_CACreateAcctMgrLoginFile,

// Empty_DATADIR_Directory_And_ACCTMGR_LOGIN_Property_Set) {

// PMSIHANDLE hMsi;

// const auto dir = std::filesystem::current_path() /= "test_data";

// msiHelper.insertProperties({

// {"DATADIR", dir.string().c_str()},

// {"ACCTMGR_LOGIN", "testuser"}

// });

// const auto result = MsiOpenPackage(msiHelper.getMsiHandle().c_str(),

// &hMsi);

// ASSERT_EQ(0u, result);

// EXPECT_EQ(0u, hFunc(hMsi));

//}

Copilot · 2025-11-04T04:40:44Z

tests/unit-tests/boinccas/test_boinccas_CACleanupOldBinaries.cpp

+            &hMsi);
+        ASSERT_EQ(0u, result);
+        EXPECT_EQ(0u, hFunc(hMsi));
+        // verify that the directory is now empty


Comment on line 150 is incorrect. The test expects the directory to NOT be empty (EXPECT_FALSE), so the comment should say 'verify that the directory is not empty' or 'verify that unlisted files remain'.

Suggested change

// verify that the directory is now empty

// verify that the directory is not empty (unlisted files remain)

Copilot · 2025-11-04T04:40:44Z

win_build/unittests.vcxproj

      <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions Condition="'$(BOINCCAS_TEST)' == 'true'">BOINCCAS_TEST;%(PreprocessorDefinitions)</PreprocessorDefinitions>


Having two separate PreprocessorDefinitions elements can lead to the second one overriding the first. Consider consolidating them into a single element or ensuring the conditional definition properly appends to the base definitions.

Copilot

Pull Request Overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-05T00:21:47Z

clientsetup/win/CACreateAcctMgrLoginFile.cpp

+        _T("    <password_hash>%s</password_hash>\n")
+        _T("</acct_mgr_login>\n"),
+        strAcctMgrLogin.c_str(),
+        strAcctMgrPasswordHash.c_str()


Potential null pointer dereference when strAcctMgrPasswordHash is empty. The previous implementation explicitly checked if the string was empty and used an empty string literal. This change directly uses .c_str() which, while safe for empty strings in practice, removes the explicit semantic that an empty password hash should result in an empty XML element value. Consider restoring the original conditional logic: !strAcctMgrPasswordHash.empty() ? strAcctMgrPasswordHash.c_str() : _T(\"\")

Suggested change

strAcctMgrPasswordHash.c_str()

!strAcctMgrPasswordHash.empty() ? strAcctMgrPasswordHash.c_str() : _T("")

Copilot · 2025-11-05T00:21:47Z

clientsetup/win/CACreateAcctMgrLoginFile.cpp

+        return uiReturnValue;
    }

+    // The project_init.xml file is stored in the data directory.


Comment incorrectly references 'project_init.xml' when the code actually creates 'acct_mgr_login.xml'. Update comment to: 'The acct_mgr_login.xml file is stored in the data directory.'

Suggested change

// The project_init.xml file is stored in the data directory.

// The acct_mgr_login.xml file is stored in the data directory.

Copilot · 2025-11-05T00:21:48Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+        throw std::runtime_error( "Failed to remove existing MSI file: " +
+            std::string(ex.what()));


The exception is thrown in cleanup() which is called from the destructor. Throwing exceptions from destructors can lead to program termination if another exception is already in flight. Consider logging the error instead of throwing, or silently catch and ignore the exception since this is a cleanup operation.

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-13T04:19:23Z

tests/unit-tests/boinccas/test_boinccas_CACreateBOINCGroups.cpp

+#ifndef BOINCCAS_TEST
+    INSTANTIATE_TEST_SUITE_P(test_boinccas_CACreateBOINCGroupsProductType,
+        test_boinccas_CACreateBOINCGroups,
+        testing::Values("1", "2", "3"));
+
+    //TEST_P(test_boinccas_CACreateBOINCAccounts, CanCreateAccounts) {
+    //    ASSERT_FALSE(userExists(masterAccountName));
+    //    ASSERT_TRUE(userCreate(masterAccountName, masterAccountPassword));
+    //    ASSERT_TRUE(userExists(masterAccountName));
+    //    ASSERT_TRUE(userDelete(masterAccountName));
+    //    ASSERT_FALSE(userExists(masterAccountName));
+    //}
+
+    //TEST_P(test_boinccas_CACreateBOINCAccounts, CreateDefaultAccounts) {
+    //    PMSIHANDLE hMsi;
+    //    const auto result =
+    //        MsiOpenPackage(msiHelper.getMsiHandle().c_str(), &hMsi);
+    //    ASSERT_EQ(0u, result);
+
+    //    msiHelper.setProperty(hMsi, "MsiNTProductType", GetParam().data());
+    //    auto [errorcode, value] =
+    //        msiHelper.getProperty(hMsi, "MsiNTProductType");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(GetParam(), value);
+
+    //    msiHelper.setProperty(hMsi, "ComputerName", testPCName);
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "ComputerName");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(testPCName, value);
+
+    //    EXPECT_FALSE(userExists(getMasterAccountName()));
+    //    EXPECT_FALSE(userExists(getProjectAccountName()));
+    //    EXPECT_EQ(0u, hFunc(hMsi));
+    //    EXPECT_TRUE(userExists(getMasterAccountName()));
+    //    EXPECT_TRUE(userExists(getProjectAccountName()));
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(getMasterAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(getProjectAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(".\\" + getMasterAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(".\\" + getProjectAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_NE("", value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_NE("", value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "RETURN_REBOOTREQUESTED");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ("1", value);
+
+    //    EXPECT_TRUE(MsiGetMode(hMsi, MSIRUNMODE_REBOOTATEND));
+    //    // cancel reboot
+    //    MsiSetMode(hMsi, MSIRUNMODE_REBOOTATEND, FALSE);
+
+    //    TearDown();
+    //}
+
+    //TEST_P(test_boinccas_CACreateBOINCAccounts, ChangePasswords) {
+    //    PMSIHANDLE hMsi;
+    //    const auto result =
+    //        MsiOpenPackage(msiHelper.getMsiHandle().c_str(), &hMsi);
+    //    ASSERT_EQ(0u, result);
+
+    //    msiHelper.setProperty(hMsi, "MsiNTProductType", GetParam().data());
+    //    auto [errorcode, value] =
+    //        msiHelper.getProperty(hMsi, "MsiNTProductType");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(GetParam(), value);
+
+    //    msiHelper.setProperty(hMsi, "ComputerName", testPCName);
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "ComputerName");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(testPCName, value);
+
+    //    EXPECT_FALSE(userExists(getMasterAccountName()));
+    //    EXPECT_FALSE(userExists(getProjectAccountName()));
+    //    ASSERT_TRUE(userCreate(getMasterAccountName(),
+    //        masterAccountPassword));
+    //    ASSERT_TRUE(userCreate(getProjectAccountName(),
+    //        projectAccountPassword));
+    //    ASSERT_TRUE(userExists(getMasterAccountName()));
+    //    ASSERT_TRUE(userExists(getProjectAccountName()));
+    //    msiHelper.setProperty(
+    //        hMsi, "BOINC_MASTER_USERNAME", getMasterAccountName());
+    //    msiHelper.setProperty(
+    //        hMsi, "BOINC_PROJECT_USERNAME", getProjectAccountName());
+
+    //    EXPECT_EQ(0u, hFunc(hMsi));
+    //    EXPECT_TRUE(userExists(getMasterAccountName()));
+    //    EXPECT_TRUE(userExists(getProjectAccountName()));
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(getMasterAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(getProjectAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(".\\" + getMasterAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(".\\" + getProjectAccountName(), value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_NE(masterAccountPassword, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_NE(projectAccountPassword, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "RETURN_REBOOTREQUESTED");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ("", value);
+
+    //    EXPECT_FALSE(MsiGetMode(hMsi, MSIRUNMODE_REBOOTATEND));
+    //    // cancel reboot
+    //    MsiSetMode(hMsi, MSIRUNMODE_REBOOTATEND, FALSE);
+
+    //    TearDown();
+    //}
+
+    //TEST_P(test_boinccas_CACreateBOINCAccounts, DontChangeExistingAccounts) {
+    //    PMSIHANDLE hMsi;
+    //    const auto result =
+    //        MsiOpenPackage(msiHelper.getMsiHandle().c_str(), &hMsi);
+    //    ASSERT_EQ(0u, result);
+
+    //    msiHelper.setProperty(hMsi, "MsiNTProductType", GetParam().data());
+    //    auto [errorcode, value] =
+    //        msiHelper.getProperty(hMsi, "MsiNTProductType");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(GetParam(), value);
+
+    //    msiHelper.setProperty(hMsi, "ComputerName", testPCName);
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "ComputerName");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    ASSERT_EQ(testPCName, value);
+
+    //    constexpr auto testMasterAccountName = "test_master";
+    //    constexpr auto testProjectAccountName = "test_project";
+
+    //    EXPECT_FALSE(userExists(testMasterAccountName));
+    //    EXPECT_FALSE(userExists(testProjectAccountName));
+    //    ASSERT_TRUE(userCreate(testMasterAccountName,
+    //        masterAccountPassword));
+    //    ASSERT_TRUE(userCreate(testProjectAccountName,
+    //        projectAccountPassword));
+    //    ASSERT_TRUE(userExists(testMasterAccountName));
+    //    ASSERT_TRUE(userExists(testProjectAccountName));
+    //    msiHelper.setProperty(hMsi, "BOINC_MASTER_USERNAME",
+    //        testMasterAccountName);
+    //    msiHelper.setProperty(hMsi, "BOINC_MASTER_PASSWORD",
+    //        masterAccountPassword);
+    //    msiHelper.setProperty(hMsi, "BOINC_PROJECT_USERNAME",
+    //        testProjectAccountName);
+    //    msiHelper.setProperty(hMsi, "BOINC_PROJECT_PASSWORD",
+    //        projectAccountPassword);
+
+    //    EXPECT_EQ(0u, hFunc(hMsi));
+    //    EXPECT_TRUE(userExists(testMasterAccountName));
+    //    EXPECT_TRUE(userExists(testProjectAccountName));
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(testMasterAccountName, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_USERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(testProjectAccountName, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(testMasterAccountName, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_ISUSERNAME");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(testProjectAccountName, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_MASTER_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(masterAccountPassword, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "BOINC_PROJECT_PASSWORD");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ(projectAccountPassword, value);
+
+    //    std::tie(errorcode, value) =
+    //        msiHelper.getProperty(hMsi, "RETURN_REBOOTREQUESTED");
+    //    EXPECT_EQ(static_cast<unsigned int>(ERROR_SUCCESS), errorcode);
+    //    EXPECT_EQ("", value);
+
+    //    EXPECT_FALSE(MsiGetMode(hMsi, MSIRUNMODE_REBOOTATEND));
+    //    // cancel reboot
+    //    MsiSetMode(hMsi, MSIRUNMODE_REBOOTATEND, FALSE);
+
+    //    if (userExists(testMasterAccountName)) {
+    //        userDelete(testMasterAccountName);
+    //    }
+    //    if (userExists(testProjectAccountName)) {
+    //        userDelete(testProjectAccountName);
+    //    }
+
+    //    TearDown();
+    //}
+#endif


The BOINCCAS_TEST guard is inverted. The test suite and tests are defined when BOINCCAS_TEST is NOT defined (using #ifndef), which means they will only be compiled when the macro is not set. This is opposite to the pattern used in other test files like test_boinccas_CACreateBOINCAccounts.cpp which use #ifdef BOINCCAS_TEST. This will prevent these tests from running when intended.

Copilot · 2025-12-13T04:19:23Z

clientsetup/win/CACreateBOINCAccounts.cpp

+    tstring resultPwd(desiredLength, _T('\0'));
+    randomPwd.copy(resultPwd.data(), desiredLength);
+    return { resultPwd };


The password generation has two issues: 1) The resultPwd string is initialized with null terminators but the copy() method doesn't guarantee null-termination at the correct position, potentially leaving embedded nulls in the middle of the string. 2) If randomPwd contains fewer than desiredLength characters, copy() may read beyond the valid data. Use substr(0, desiredLength) instead of manual copy to safely extract the desired portion of the string.

Suggested change

tstring resultPwd(desiredLength, _T('\0'));

randomPwd.copy(resultPwd.data(), desiredLength);

return { resultPwd };

return randomPwd.substr(0, desiredLength);

Copilot · 2025-12-13T04:19:24Z

win_build/unittests.vcxproj

    <ClCompile>
      <AdditionalIncludeDirectories>../win_build;../lib;../zip;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
      <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions Condition="'$(BOINCCAS_TEST)' == 'true'">BOINCCAS_TEST;%(PreprocessorDefinitions)</PreprocessorDefinitions>


The PreprocessorDefinitions directive is duplicated on consecutive lines. Line 20 already defines PreprocessorDefinitions, and line 21 adds a conditional definition. This should be consolidated or the existing PreprocessorDefinitions on line 20 should include the conditional definition to avoid potential conflicts or confusion.

Copilot · 2025-12-13T04:19:24Z

clientsetup/win/CACreateAcctMgrLoginFile.cpp

+    std::wofstream fAcctMgrLoginFile(strAcctMgrLoginFile);
+    fAcctMgrLoginFile <<
+        _T("<acct_mgr_login>\n") <<
+        _T("    <login>") << login << _T("</login>\n") <<
+        _T("    <password_hash>") << pwdHash << _T("</password_hash>\n") <<
+        _T("</acct_mgr_login>\n");
+    fAcctMgrLoginFile.close();


The file stream is not checked for successful opening before writing. If the file cannot be opened (e.g., due to permissions), the write operations will silently fail. Add a check after line 60 to verify the stream is open and return an appropriate error code if it fails.

Copilot · 2025-12-13T04:19:24Z

tests/unit-tests/boinccas/test_boinccas_CACreateBOINCAccounts.cpp

+        EXPECT_NE(projectAccountPassword, projectPassword);
+        EXPECT_EQ(33, projectPassword.length());
+
+        EPECT_NE(masterPassword, projectPassword);


The macro name is misspelled. "EPECT_NE" should be "EXPECT_NE". This will cause a compilation error.

Suggested change

EPECT_NE(masterPassword, projectPassword);

EXPECT_NE(masterPassword, projectPassword);

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-16T06:12:34Z

clientsetup/win/CACreateBOINCAccounts.cpp

+                _T("Account used to execute BOINC as a system service");
            ui.usri1_priv = USER_PRIV_USER;
            ui.usri1_home_dir = NULL;
            ui.usri1_comment = NULL;


The line ui.usri1_comment = NULL; is duplicated. Line 173 already sets ui.usri1_comment, so line 177 is redundant.

Suggested change

ui.usri1_comment = NULL;

Copilot · 2025-12-16T06:12:34Z

clientsetup/win/CACreateBOINCAccounts.cpp

+                _T("Account used to execute BOINC applications");
            ui.usri1_priv = USER_PRIV_USER;
            ui.usri1_home_dir = NULL;
            ui.usri1_comment = NULL;


The line ui.usri1_comment = NULL; is duplicated. Line 293 already sets ui.usri1_comment, so line 297 is redundant.

Suggested change

ui.usri1_comment = NULL;

Copilot · 2025-12-16T06:12:34Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    if (!LookupAccountSid(nullptr, &buffer, name.data(), &nameSize,
+        domain.data(), &domainSize, &use)) {
+        return "Users";
+    }


The function returns the name buffer directly without properly trimming to the actual length. The LookupAccountSid function updates nameSize to reflect the actual string length, but the function returns the full buffer which was initially sized to 256 characters. This means the returned string will contain trailing null characters or garbage. After the LookupAccountSid call succeeds, you should resize the name string to nameSize to return only the actual group name.

Suggested change

}

}

name.resize(nameSize);

Copilot · 2025-12-16T06:12:35Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+        std::string value(size++, '\0');
+        result =
+            MsiGetProperty(hMsiHandle, propertyName.c_str(), value.data(),
+                &size);
+        if (result != ERROR_SUCCESS) {
+            return { result, {} };
+        }
+


The size increment should happen before the second call to MsiGetProperty, not after checking the error code. When MsiGetProperty returns ERROR_MORE_DATA, the size variable contains the required buffer size excluding the null terminator. You need to increment it before resizing and calling MsiGetProperty again to ensure there's room for the null terminator. The current code increments after creating the buffer, which is correct, but the increment is in the wrong position relative to the buffer creation.

Suggested change

std::string value(size++, '\0');

result =

MsiGetProperty(hMsiHandle, propertyName.c_str(), value.data(),

&size);

if (result != ERROR_SUCCESS) {

return { result, {} };

}

size++; // Increment before allocating buffer and second call

std::string value(size, '\0');

result =

MsiGetProperty(hMsiHandle, propertyName.c_str(), value.data(),

&size);

if (result != ERROR_SUCCESS) {

return { result, {} };

}

// Remove any trailing nulls (optional, but safe)

value.resize(strnlen(value.c_str(), size));

Copilot · 2025-12-16T06:12:35Z

clientsetup/win/CACreateBOINCAccounts.cpp

+    if (!Base64Encode(randomBuffer.data(), static_cast<int>(dwBufSize), encodedString.data(),
+        &dwSize, 0)) {
+        return { };
+    }


The Base64EncodeGetRequiredLength returns the required buffer size, but Base64Encode expects an int* for the size parameter and updates it to reflect the actual encoded length. However, the code passes &dwSize directly without checking if the encoding succeeded. If Base64Encode fails and returns false, dwSize may not be updated correctly, and the subsequent use of encodedString could read uninitialized or incorrect data. Consider checking the return value and handling the size appropriately.

Suggested change

}

}

encodedString.resize(dwSize);

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-16T07:42:16Z

tests/unit-tests/boinccas/test_boinccas_CACreateBOINCGroups.cpp

+        void TearDown() override {
+            //if (userExists(masterAccountName)) {
+            //    userDelete(masterAccountName);
+            //}
+            //if (userExists(projectAccountName)) {
+            //    userDelete(projectAccountName);
+            //}
+
+            //if (localGroupExists(adminsGroupName)) {
+            //    deleteLocalGroup(adminsGroupName);
+            //}
+            //if (localGroupExists(usersGroupName)) {
+            //    deleteLocalGroup(usersGroupName);
+            //}
+            //if (localGroupExists(projectsGroupName)) {
+            //    deleteLocalGroup(projectsGroupName);
+            //}
+        }


The TearDown method for CACreateBOINCGroups tests does not clean up the local groups that are created during tests. Tests like CreateGroups_ProtectionIsNotSet, CreateGroups_ProtectedDisabled, and CreateGroups_ProtectedEnabled_AddMembers all create local groups (boinc_admins, boinc_users, boinc_projects) but the cleanup code is commented out. This will cause test pollution where groups persist between test runs, potentially causing failures or incorrect test behavior.

The commented-out cleanup code in TearDown (lines 72-80) should be uncommented to properly clean up created resources.

Copilot · 2025-12-16T07:42:16Z

clientsetup/win/CACreateAcctMgrLoginFile.cpp

+    std::wofstream fAcctMgrLoginFile(strAcctMgrLoginFile);
+    fAcctMgrLoginFile <<
+        _T("<acct_mgr_login>\n") <<
+        _T("    <login>") << login << _T("</login>\n") <<
+        _T("    <password_hash>") << pwdHash << _T("</password_hash>\n") <<
+        _T("</acct_mgr_login>\n");


The file stream is not checked for errors after creation or after writing. If the file creation fails (e.g., due to permissions, disk space, or an invalid path), or if writing fails, the function will still return ERROR_SUCCESS without detecting the failure. The stream's state should be checked with fAcctMgrLoginFile.good() or by checking the stream's boolean conversion after critical operations to ensure the file operations succeeded.

Suggested change

std::wofstream fAcctMgrLoginFile(strAcctMgrLoginFile);

fAcctMgrLoginFile <<

_T("<acct_mgr_login>\n") <<

_T(" <login>") << login << _T("</login>\n") <<

_T(" <password_hash>") << pwdHash << _T("</password_hash>\n") <<

_T("</acct_mgr_login>\n");

std::wofstream fAcctMgrLoginFile(strAcctMgrLoginFile);

if (!fAcctMgrLoginFile.is_open()) {

return ERROR_INSTALL_FAILURE;

}

fAcctMgrLoginFile <<

_T("<acct_mgr_login>\n") <<

_T(" <login>") << login << _T("</login>\n") <<

_T(" <password_hash>") << pwdHash << _T("</password_hash>\n") <<

_T("</acct_mgr_login>\n");

if (!fAcctMgrLoginFile) {

fAcctMgrLoginFile.close();

return ERROR_INSTALL_FAILURE;

}

Copilot · 2025-12-16T07:42:17Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+            return false;
+        }
+
+        acctSid.reset(reinterpret_cast<PSID>(sidBuf.data()));


The SID management in isAccountMemberOfLocalGroup has a critical lifetime issue. When calling acctSid.reset(reinterpret_cast<PSID>(sidBuf.data())), you're storing a pointer to a local vector's data in a wil::unique_sid. When sidBuf goes out of scope at line 390, the underlying memory is freed, but acctSid still holds a dangling pointer that gets dereferenced at line 409. This will cause undefined behavior.

The SID data should be copied to heap-allocated memory managed by the unique_sid, not just reset to point at the stack buffer.

Suggested change

acctSid.reset(reinterpret_cast<PSID>(sidBuf.data()));

// Allocate heap memory for the SID and copy the data

PSID heapSid = ::LocalAlloc(LMEM_FIXED, sidSize);

if (!heapSid) {

return false;

}

if (!::CopySid(sidSize, heapSid, sidBuf.data())) {

::LocalFree(heapSid);

return false;

}

acctSid.reset(heapSid);

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-16T16:48:42Z

clientsetup/win/CACreateBOINCAccounts.cpp

+    tstring resultPwd(desiredLength, _T('\0'));
+    randomPwd.copy(resultPwd.data(), desiredLength);
+    return { resultPwd };


The copy method does not null-terminate the string, but the previous resize initialized resultPwd with null characters. However, copy will overwrite these characters, potentially leaving the string without proper null-termination if randomPwd is shorter than desiredLength. Consider using resize with the actual copied length or ensuring null-termination explicitly.

Suggested change

tstring resultPwd(desiredLength, _T('\0'));

randomPwd.copy(resultPwd.data(), desiredLength);

return { resultPwd };

if (randomPwd.size() > desiredLength) {

return randomPwd.substr(0, desiredLength);

} else {

return randomPwd;

}

Copilot · 2025-12-16T16:48:43Z

clientsetup/win/CACreateBOINCAccounts.cpp

+    tstring resultPwd(desiredLength, _T('\0'));
+    randomPwd.copy(resultPwd.data(), desiredLength);
+    return { resultPwd };


If randomPwd.size() is less than desiredLength, the copy operation will only copy the available characters, leaving the remaining characters as null bytes from the initialization. This will result in a password containing null characters in the middle, which may not be the intended behavior. Consider checking the size and handling the case where the encoded string is shorter than desired.

Suggested change

tstring resultPwd(desiredLength, _T('\0'));

randomPwd.copy(resultPwd.data(), desiredLength);

return { resultPwd };

// Ensure we do not pad with nulls if the encoded string is too short

if (randomPwd.size() >= desiredLength) {

return randomPwd.substr(0, desiredLength);

} else {

return randomPwd;

}

Copilot · 2025-12-16T16:48:43Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    value.resize(size);
+    queryResult = RegQueryValueEx(hKey, valueName.c_str(), nullptr, &type,
+        reinterpret_cast<LPBYTE>(value.data()), &size);


After calling RegQueryValueEx, the size parameter contains the size in bytes including the null terminator for string types. Resizing to size and then trimming at the first NUL may leave an extra character. Consider using size - 1 when resizing for string types or handle the null terminator more carefully.

Suggested change

value.resize(size);

queryResult = RegQueryValueEx(hKey, valueName.c_str(), nullptr, &type,

reinterpret_cast<LPBYTE>(value.data()), &size);

if ((type == REG_SZ || type == REG_EXPAND_SZ) && size > 0) {

value.resize(size - 1); // Exclude null terminator

queryResult = RegQueryValueEx(hKey, valueName.c_str(), nullptr, &type,

reinterpret_cast<LPBYTE>(value.data()), &size);

} else {

value.resize(size);

queryResult = RegQueryValueEx(hKey, valueName.c_str(), nullptr, &type,

reinterpret_cast<LPBYTE>(value.data()), &size);

}

Copilot · 2025-12-16T16:48:43Z

clientsetup/win/password.h


 // Prototypes
-BOOL GenerateRandomPassword( tstring& strPassword, DWORD dwDesiredLength );
+


The file now only contains an empty comment placeholder between the header guard and the endif. Consider removing this entire file since it no longer declares any functions or types and appears to be unused.

Copilot · 2025-12-16T16:48:44Z

clientsetup/win/stdafx.h

-
 // Modify the following defines if you have to target a platform prior to the ones specified below.
 // Refer to MSDN for the latest info on corresponding values for different platforms.



These Windows version macros target Windows Vista/Server 2008 (0x0600). Consider adding a comment explaining the minimum supported Windows version, as this is an important constraint for users and maintainers.

Suggested change

// Minimum supported Windows version: Windows Vista / Windows Server 2008 (0x0600)

// If you need to support an earlier version of Windows, update the following macros accordingly.

Copilot · 2025-12-16T16:48:44Z

clientsetup/win/stdafx.h


 #ifndef _WIN32_WINNT
-#define _WIN32_WINNT 0x0501
+#define _WIN32_WINNT 0x0600


These Windows version macros target Windows Vista/Server 2008 (0x0600). Consider adding a comment explaining the minimum supported Windows version, as this is an important constraint for users and maintainers.

Copilot · 2025-12-16T16:48:44Z

clientsetup/win/stdafx.h


 #ifndef _WIN32_WINDOWS
-#define _WIN32_WINDOWS 0x0501
+#define _WIN32_WINDOWS 0x0600


These Windows version macros target Windows Vista/Server 2008 (0x0600). Consider adding a comment explaining the minimum supported Windows version, as this is an important constraint for users and maintainers.

Copilot · 2025-12-16T16:48:44Z

clientsetup/win/stdafx.h


 #ifndef _WIN32_IE
-#define _WIN32_IE 0x0500
+#define _WIN32_IE 0x0700


These Windows version macros target Windows Vista/Server 2008 (0x0600). Consider adding a comment explaining the minimum supported Windows version, as this is an important constraint for users and maintainers.

Copilot · 2025-12-16T16:48:45Z

clientsetup/win/stdafx.h


 #ifndef _WIN32_MSI
-#define _WIN32_MSI 200
+#define _WIN32_MSI 400


These Windows version macros target Windows Vista/Server 2008 (0x0600). Consider adding a comment explaining the minimum supported Windows version, as this is an important constraint for users and maintainers.

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T19:08:02Z

tests/unit-tests/boinccas/boinccas_helper.cpp

+    }
+    std::string domainName(domainSize, '\0');
+    std::vector<BYTE> sidBuf(sidSize);
+    domainSize = 256;


The domainSize is reset to a hardcoded value of 256 after the first LookupAccountName call. This should use the actual required size from the first call. If the domain name is longer than 256 characters, this could lead to buffer overflow or incorrect behavior.

Suggested change

domainSize = 256;

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings November 1, 2025 22:14

Copilot AI reviewed Nov 1, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 1, 2025 23:09

Copilot AI reviewed Nov 1, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 1, 2025 23:48

Copilot AI reviewed Nov 1, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 04:53

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 05:36

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 06:53

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 07:27

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 09:20

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 2, 2025 17:06

Copilot AI reviewed Nov 2, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 3, 2025 02:45

Copilot AI reviewed Nov 3, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 4, 2025 04:28

Copilot AI reviewed Nov 4, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 5, 2025 00:13

Copilot AI reviewed Nov 5, 2025

View reviewed changes

AenBleidd added 6 commits December 6, 2025 23:42

Add AnnounceUpgrade test to the module boinccas module tests.

9286100

Signed-off-by: Vitalii Koshura <[email protected]>

Fix issues

2795f4f

Signed-off-by: Vitalii Koshura <[email protected]>

Add debug logic

c9292c1

Signed-off-by: Vitalii Koshura <[email protected]>

Fix unit tests

b0b37ff

Signed-off-by: Vitalii Koshura <[email protected]>

debug

bcba607

Signed-off-by: Vitalii Koshura <[email protected]>

fix

0e69878

Signed-off-by: Vitalii Koshura <[email protected]>

AenBleidd added 2 commits December 13, 2025 04:17

Refactoring

30b65d6

Signed-off-by: Vitalii Koshura <[email protected]>

Verify generated password length

0c3eb7b

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 13, 2025 04:09

Copilot started reviewing on behalf of AenBleidd December 13, 2025 04:14 View session

Fix typo

02716b0

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI reviewed Dec 13, 2025

View reviewed changes

Add CACreateBOINCGroups tests

478ef95

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 16, 2025 06:02

Copilot started reviewing on behalf of AenBleidd December 16, 2025 06:07 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

AenBleidd added 2 commits December 16, 2025 07:50

Fix

dd14142

Signed-off-by: Vitalii Koshura <[email protected]>

Fix

14a60d7

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 16, 2025 07:27

Copilot started reviewing on behalf of AenBleidd December 16, 2025 07:37 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

AenBleidd added 2 commits December 16, 2025 09:56

Don't reboot during the testing

1211d50

Signed-off-by: Vitalii Koshura <[email protected]>

Fix tests

d813dec

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 16, 2025 14:09

Copilot AI reviewed Dec 16, 2025

View reviewed changes

Copilot started reviewing on behalf of AenBleidd December 16, 2025 14:31 View session

AenBleidd added 2 commits December 16, 2025 17:20

Fix tests

665349b

Signed-off-by: Vitalii Koshura <[email protected]>

Fix tests

64553ef

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 16, 2025 16:45

Copilot AI reviewed Dec 16, 2025

View reviewed changes

AenBleidd added 2 commits December 17, 2025 19:32

Fix tests

a11576d

Signed-off-by: Vitalii Koshura <[email protected]>

Enable tests

f86ef01

Signed-off-by: Vitalii Koshura <[email protected]>

Copilot AI review requested due to automatic review settings December 17, 2025 19:00

Copilot started reviewing on behalf of AenBleidd December 17, 2025 19:02 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Refactor CreateBOINCGroups class

88eef93

Signed-off-by: Vitalii Koshura <[email protected]>

		MSIHANDLE hView;
		auto result = MsiDatabaseOpenView(hMsi, sql_insert.data(), &hView);

	const auto hRecord = MsiCreateRecord(static_cast<UINT>(2));
	const auto hRecord = MsiCreateRecord(2);

		RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);
		RegCloseKey(hKey);

		constexpr auto registryKey =
		"SOFTWARE\\Space Sciences Laboratory, U.C. Berkeley\\BOINC Setup";

		const std::vector<std::pair<std::string, std::string>>& properties);
		std::string getMsiHandle() const {

	if (result != ERROR_SUCCESS) {
	if (result != ERROR_SUCCESS) {
	MsiCloseHandle(hView);

-    HKEY hKey = nullptr;
-    const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, registryKey, 0,
-        KEY_WRITE, &hKey);
-    if (openResult != ERROR_SUCCESS) {
-        return;
-    }
-    RegDeleteKey(HKEY_LOCAL_MACHINE, registryKey);
-    RegCloseKey(hKey);
+    // To delete a registry key, open its parent and call RegDeleteKey with the subkey name.
+    constexpr auto parentKeyPath = "SOFTWARE\\Space Sciences Laboratory, U.C. Berkeley";
+    constexpr auto subKeyName = "BOINC Setup";
+    HKEY hParentKey = nullptr;
+    const auto openResult = RegOpenKeyEx(HKEY_LOCAL_MACHINE, parentKeyPath, 0,
+        KEY_WRITE, &hParentKey);
+    if (openResult != ERROR_SUCCESS) {
+        return;
+    }
+    RegCloseKey(hParentKey); // Close before deleting
+    // Reopen parent for deletion (RegDeleteKey requires open parent)
+    if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, parentKeyPath, 0, KEY_WRITE, &hParentKey) == ERROR_SUCCESS) {
+        RegDeleteKey(hParentKey, subKeyName);
+        RegCloseKey(hParentKey);
+    }

	throw std::runtime_error( "Failed to remove existing MSI file: " +
	throw std::runtime_error("Failed to remove existing MSI file: " +

-    try {
-        std::filesystem::remove(std::filesystem::current_path() / msiName);
-    }
-    catch (const std::exception& ex) {
-        throw std::runtime_error( "Failed to remove existing MSI file: " +
-            std::string(ex.what()));
-    }
+    std::error_code ec;
+    std::filesystem::remove(std::filesystem::current_path() / msiName, ec);
+    if (ec) {
+        // Optionally log the error, but do not throw
+        OutputDebugStringA(("Failed to remove existing MSI file: " + ec.message()).c_str());
+    }

	std::filesystem::current_path() /= "non_existent_directory";
	std::filesystem::current_path() / "non_existent_directory";

	testDir = std::filesystem::current_path() /= "empty";
	testDir = std::filesystem::current_path() / "empty";

	// verify that the directory is now empty
	// verify that the directory is not empty (unlisted files remain)

		<PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
		<PreprocessorDefinitions Condition="'$(BOINCCAS_TEST)' == 'true'">BOINCCAS_TEST;%(PreprocessorDefinitions)</PreprocessorDefinitions>

	strAcctMgrPasswordHash.c_str()
	!strAcctMgrPasswordHash.empty() ? strAcctMgrPasswordHash.c_str() : _T("")

	// The project_init.xml file is stored in the data directory.
	// The acct_mgr_login.xml file is stored in the data directory.

		throw std::runtime_error( "Failed to remove existing MSI file: " +
		std::string(ex.what()));

	EPECT_NE(masterPassword, projectPassword);
	EXPECT_NE(masterPassword, projectPassword);

-        acctSid.reset(reinterpret_cast<PSID>(sidBuf.data()));
+        // Allocate heap memory for the SID and copy the data
+        PSID heapSid = ::LocalAlloc(LMEM_FIXED, sidSize);
+        if (!heapSid) {
+            return false;
+        }
+        if (!::CopySid(sidSize, heapSid, sidBuf.data())) {
+            ::LocalFree(heapSid);
+            return false;
+        }
+        acctSid.reset(heapSid);

-    tstring resultPwd(desiredLength, _T('\0'));
-    randomPwd.copy(resultPwd.data(), desiredLength);
-    return { resultPwd };
+    if (randomPwd.size() > desiredLength) {
+        return randomPwd.substr(0, desiredLength);
+    } else {
+        return randomPwd;
+    }


		// Prototypes
		BOOL GenerateRandomPassword( tstring& strPassword, DWORD dwDesiredLength );

Add AnnounceUpgrade test to the module boinccas module tests. #6635

Are you sure you want to change the base?

Add AnnounceUpgrade test to the module boinccas module tests. #6635

Conversation

AenBleidd commented Nov 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 2, 2025


		// Modify the following defines if you have to target a platform prior to the ones specified below.
		// Refer to MSDN for the latest info on corresponding values for different platforms.



	// Minimum supported Windows version: Windows Vista / Windows Server 2008 (0x0600)
	// If you need to support an earlier version of Windows, update the following macros accordingly.