From 180d811099bcb42e4f1aaecc015bc8410ac2d41a Mon Sep 17 00:00:00 2001 From: Ben Hillis Date: Mon, 15 Dec 2025 13:03:02 -0800 Subject: [PATCH] Resolve issue with config file writing sections outside of their expected header. (#13898) * Resolve issue with config file writing sections outside of their expected header. * add more writewslconfig variations * formatting --------- Co-authored-by: Ben Hillis --- .github/copilot-instructions.md | 18 +++ src/shared/configfile/configfile.cpp | 59 ++++---- test/windows/UnitTests.cpp | 202 +++++++++++++++++++++++++++ 3 files changed, 251 insertions(+), 28 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 4ff6e45..11f406d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -66,12 +66,30 @@ Build parameters: ## Testing ### Unit Tests (Windows Only - TAEF Framework) + +**CRITICAL: ALWAYS build the ENTIRE project before running tests:** +```powershell +# Build everything first - this is required! +cmake --build . -- -m + +# Then run tests +bin\\\test.bat +``` + +**Why full build is required:** +- Tests depend on multiple components (libwsl.dll, wsltests.dll, wslservice.exe, etc.) +- Partial builds (e.g., only `configfile` or `wsltests`) will cause test failures +- Changed components must be built together to ensure compatibility +- **DO NOT skip the full build step even if only one file changed** + +Test execution: - Run all tests: `bin\\\test.bat` - **NEVER CANCEL: Full test suite takes 30-60 minutes. Set timeout to 90+ minutes.** - Run subset: `bin\\\test.bat /name:*UnitTest*` - Run specific test: `bin\\\test.bat /name:::` - WSL1 tests: Add `-Version 1` flag - Fast mode (after first run): Add `-f` flag (requires `wsl --set-default test_distro`) +- **Requires Administrator privileges** - test.bat will fail without admin rights Test debugging: - Wait for debugger: `/waitfordebugger` diff --git a/src/shared/configfile/configfile.cpp b/src/shared/configfile/configfile.cpp index 3078387..be3892b 100644 --- a/src/shared/configfile/configfile.cpp +++ b/src/shared/configfile/configfile.cpp @@ -240,7 +240,6 @@ int ParseConfigFile(std::vector& keys, FILE* file, int flags, const w { wint_t ch = 0; unsigned long line = 0; - size_t newKeyValueInsertPos = 0; bool trailingComment = false; bool inQuote = false; size_t trimmedLength = 0; @@ -328,8 +327,6 @@ NewLine: if (trailingComment) { - // Subtract 1 to account for ch being '\n' or WEOF. - newKeyValueInsertPos = configFileOutput.length() - 1; trailingComment = false; } } @@ -390,6 +387,37 @@ NewLine: break; case '[': + // We're about to parse a new section. If we have an unwritten key-value + // and the current section matches, write it now before moving to the new section. + if (updateConfigFile && !outputKeyValueUpdated && !removeKey && sectionLength > 0) + { + const auto& outputConfigKey = outputKey.value(); + if (outputConfigKey.Matches(key.c_str(), sectionLength)) + { + const auto& keyNames = outputConfigKey.GetNames(); + // Config key without name. + FAIL_FAST_IF(keyNames.empty()); + const auto keyNameUtf8 = keyNames.front(); + const auto keyName = wsl::shared::string::MultiByteToWide(keyNameUtf8); + const auto sectionKeySeparatorPos = keyName.find('.'); + // Config key without separated section/key name + FAIL_FAST_IF(sectionKeySeparatorPos == std::string_view::npos); + // Config key without section name + FAIL_FAST_IF(sectionKeySeparatorPos == 0); + // Config key without key name + FAIL_FAST_IF(sectionKeySeparatorPos == (keyName.length() - 1)); + + // Remove any trailing newlines before inserting the new key-value + while (!configFileOutput.empty() && configFileOutput.back() == L'\n') + { + configFileOutput.pop_back(); + } + + auto keyValue = std::format(L"\n{}={}\n\n", keyName.substr(sectionKeySeparatorPos + 1), outputKey.value().GetValue()); + configFileOutput += keyValue; + outputKeyValueUpdated = true; + } + } goto ParseSection; default: @@ -418,30 +446,6 @@ NewLine: } ParseSection: - if (updateConfigFile && !outputKeyValueUpdated && !removeKey && sectionLength > 0) - { - const auto& outputConfigKey = outputKey.value(); - if (outputConfigKey.Matches(key.c_str(), sectionLength)) - { - const auto& keyNames = outputConfigKey.GetNames(); - // Config key without name. - FAIL_FAST_IF(keyNames.empty()); - const auto keyNameUtf8 = keyNames.front(); - const auto keyName = wsl::shared::string::MultiByteToWide(keyNameUtf8); - const auto sectionKeySeparatorPos = keyName.find('.'); - // Config key without separated section/key name - FAIL_FAST_IF(sectionKeySeparatorPos == std::string_view::npos); - // Config key without section name - FAIL_FAST_IF(sectionKeySeparatorPos == 0); - // Config key without key name - FAIL_FAST_IF(sectionKeySeparatorPos == (keyName.length() - 1)); - - auto keyValue = std::format(L"\n{}={}", keyName.substr(sectionKeySeparatorPos + 1), outputKey.value().GetValue()); - configFileOutput.insert(newKeyValueInsertPos, keyValue); - outputKeyValueUpdated = true; - } - } - // parse [section] ([ is already parsed) if (updateConfigFile) { @@ -796,7 +800,6 @@ ValueDone: // Trim any trailing space. value.resize(trimmedLength); SetConfig(keys, key.c_str(), value.c_str(), flags & CFG_DEBUG, filePath, line); - newKeyValueInsertPos = configFileOutput.length(); } goto NewLine; diff --git a/test/windows/UnitTests.cpp b/test/windows/UnitTests.cpp index 0521431..5da907a 100644 --- a/test/windows/UnitTests.cpp +++ b/test/windows/UnitTests.cpp @@ -3517,6 +3517,208 @@ localhostForwarding=true configRead.close(); VERIFY_ARE_EQUAL(customWslConfigContentActual, customWslConfigContentExpected); } + + // Regression test for GitHub issue #12671: + // Ensure that section headers always appear BEFORE their key-value pairs. + // Bug: WSL Settings GUI was writing keys before the section header, causing "Unknown key" errors. + { + std::wstring bugScenarioConfig = + LR"([wsl2] +[experimental] +[wsl2] +)"; + WslConfigChange config{bugScenarioConfig.c_str()}; + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + // Write memory setting - this should NOT appear before the first [wsl2] + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 17825792000ULL; // Value from bug report + + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + // Read and verify + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Find FIRST occurrence of [wsl2] and memory= + auto firstWsl2Pos = fileContent.find(L"[wsl2]"); + auto memoryPos = fileContent.find(L"memory="); + + VERIFY_ARE_NOT_EQUAL(firstWsl2Pos, std::wstring::npos); + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos); + + // The critical assertion: memory= must NOT appear before [wsl2] + VERIFY_IS_TRUE(firstWsl2Pos < memoryPos); + + // Additional check: memory should appear after the first [wsl2], not after line 1 + auto firstLineEnd = fileContent.find(L'\n'); + VERIFY_IS_TRUE(memoryPos > firstLineEnd); + } + + // Test: Empty file - should create proper [wsl2] section structure + { + std::wofstream emptyConfig(apiWslConfigFilePath, std::ios::trunc); + emptyConfig.close(); + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 4294967296ULL; // 4GB + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Should create [wsl2] section and add memory key + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") != std::wstring::npos); + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos); + // Verify [wsl2] comes before memory= + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") < fileContent.find(L"memory=")); + } + + // Test: Multiple same-section instances - should update first occurrence + { + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc); + configFile << L"[wsl2]\n"; + configFile << L"processors=4\n"; + configFile << L"\n"; + configFile << L"[experimental]\n"; + configFile << L"autoProxy=true\n"; + configFile << L"\n"; + configFile << L"[wsl2]\n"; // Second [wsl2] section + configFile << L"swap=0\n"; + configFile.close(); + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 8589934592ULL; // 8GB + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Find first and second [wsl2] + auto firstWsl2 = fileContent.find(L"[wsl2]"); + auto secondWsl2 = fileContent.find(L"[wsl2]", firstWsl2 + 1); + auto memoryPos = fileContent.find(L"memory="); + + VERIFY_ARE_NOT_EQUAL(firstWsl2, std::wstring::npos); + VERIFY_ARE_NOT_EQUAL(secondWsl2, std::wstring::npos); + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos); + + // Memory should be added to FIRST [wsl2] section, not second + VERIFY_IS_TRUE(memoryPos > firstWsl2); + VERIFY_IS_TRUE(memoryPos < secondWsl2); + } + + // Test: EOF without trailing newline + { + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc); + configFile << L"[wsl2]\n"; + configFile << L"processors=2"; // No trailing newline + configFile.close(); + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 3221225472ULL; // 3GB + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Should properly append memory key even without trailing newline on last line + VERIFY_IS_TRUE(fileContent.find(L"processors=2") != std::wstring::npos); + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos); + + // Verify both keys are in the same section + auto wsl2Pos = fileContent.find(L"[wsl2]"); + auto processorsPos = fileContent.find(L"processors=2"); + auto memoryPos = fileContent.find(L"memory="); + VERIFY_IS_TRUE(wsl2Pos < processorsPos); + VERIFY_IS_TRUE(wsl2Pos < memoryPos); + + // Memory should come after processors in the same section + VERIFY_IS_TRUE(processorsPos < memoryPos); + } + + // Test: Empty section followed by another section + { + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc); + configFile << L"[wsl2]\n"; + configFile << L"[experimental]\n"; + configFile << L"autoProxy=true\n"; + configFile.close(); + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 5368709120ULL; // 5GB + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Should insert memory into empty [wsl2] section before [experimental] + auto wsl2Pos = fileContent.find(L"[wsl2]"); + auto memoryPos = fileContent.find(L"memory="); + auto experimentalPos = fileContent.find(L"[experimental]"); + + VERIFY_ARE_NOT_EQUAL(wsl2Pos, std::wstring::npos); + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos); + VERIFY_ARE_NOT_EQUAL(experimentalPos, std::wstring::npos); + + // Order should be: [wsl2], memory=, [experimental] + VERIFY_IS_TRUE(wsl2Pos < memoryPos); + VERIFY_IS_TRUE(memoryPos < experimentalPos); + } + + // Test: Section header at EOF with no content + { + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc); + configFile << L"[wsl2]"; // Section at EOF, no newline, no content + configFile.close(); + + wslConfig = createWslConfig(apiWslConfigFilePath); + VERIFY_IS_NOT_NULL(wslConfig); + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); }); + + WslConfigSetting memorySetting{}; + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes; + memorySetting.UInt64Value = 6442450944ULL; // 6GB + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS); + + std::wifstream configRead(apiWslConfigFilePath); + std::wstring fileContent{std::istreambuf_iterator(configRead), {}}; + configRead.close(); + + // Should properly add key to section at EOF + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") != std::wstring::npos); + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos); + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") < fileContent.find(L"memory=")); + } } TEST_METHOD(LaunchWslSettingsFromProtocol)