@samitouri / QOSAMI-WSL / commits / 180d8110

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 <benhill@ntdev.microsoft.com>

Ben Hillis committed Dec 15, 2025 at 13:03 UTC 180d811099bcb42e4f1aaecc015bc8410ac2d41a
3 files changed +251 -28
.github/copilot-instructions.md
+18
@@ -66,12 +66,30 @@ Build parameters:
66 ## Testing
67
68 ### Unit Tests (Windows Only - TAEF Framework)
69 +
70 +**CRITICAL: ALWAYS build the ENTIRE project before running tests:**
71 +```powershell
72 +# Build everything first - this is required!
73 +cmake --build . -- -m
74 +
75 +# Then run tests
76 +bin\<platform>\<target>\test.bat
77 +```
78 +
79 +**Why full build is required:**
80 +- Tests depend on multiple components (libwsl.dll, wsltests.dll, wslservice.exe, etc.)
81 +- Partial builds (e.g., only `configfile` or `wsltests`) will cause test failures
82 +- Changed components must be built together to ensure compatibility
83 +- **DO NOT skip the full build step even if only one file changed**
84 +
85 +Test execution:
86 - Run all tests: `bin\<platform>\<target>\test.bat`
87 - **NEVER CANCEL: Full test suite takes 30-60 minutes. Set timeout to 90+ minutes.**
88 - Run subset: `bin\<platform>\<target>\test.bat /name:*UnitTest*`
89 - Run specific test: `bin\<platform>\<target>\test.bat /name:<class>::<test>`
90 - WSL1 tests: Add `-Version 1` flag
91 - Fast mode (after first run): Add `-f` flag (requires `wsl --set-default test_distro`)
92 +- **Requires Administrator privileges** - test.bat will fail without admin rights
93
94 Test debugging:
95 - Wait for debugger: `/waitfordebugger`
src/shared/configfile/configfile.cpp
+31 -28
@@ -240,7 +240,6 @@ int ParseConfigFile(std::vector<ConfigKey>& keys, FILE* file, int flags, const w
240 {
241 wint_t ch = 0;
242 unsigned long line = 0;
243 - size_t newKeyValueInsertPos = 0;
243 bool trailingComment = false;
244 bool inQuote = false;
245 size_t trimmedLength = 0;
@@ -328,8 +327,6 @@ NewLine:
327
328 if (trailingComment)
329 {
331 - // Subtract 1 to account for ch being '\n' or WEOF.
332 - newKeyValueInsertPos = configFileOutput.length() - 1;
330 trailingComment = false;
331 }
332 }
@@ -390,6 +387,37 @@ NewLine:
387 break;
388
389 case '[':
390 + // We're about to parse a new section. If we have an unwritten key-value
391 + // and the current section matches, write it now before moving to the new section.
392 + if (updateConfigFile && !outputKeyValueUpdated && !removeKey && sectionLength > 0)
393 + {
394 + const auto& outputConfigKey = outputKey.value();
395 + if (outputConfigKey.Matches(key.c_str(), sectionLength))
396 + {
397 + const auto& keyNames = outputConfigKey.GetNames();
398 + // Config key without name.
399 + FAIL_FAST_IF(keyNames.empty());
400 + const auto keyNameUtf8 = keyNames.front();
401 + const auto keyName = wsl::shared::string::MultiByteToWide(keyNameUtf8);
402 + const auto sectionKeySeparatorPos = keyName.find('.');
403 + // Config key without separated section/key name
404 + FAIL_FAST_IF(sectionKeySeparatorPos == std::string_view::npos);
405 + // Config key without section name
406 + FAIL_FAST_IF(sectionKeySeparatorPos == 0);
407 + // Config key without key name
408 + FAIL_FAST_IF(sectionKeySeparatorPos == (keyName.length() - 1));
409 +
410 + // Remove any trailing newlines before inserting the new key-value
411 + while (!configFileOutput.empty() && configFileOutput.back() == L'\n')
412 + {
413 + configFileOutput.pop_back();
414 + }
415 +
416 + auto keyValue = std::format(L"\n{}={}\n\n", keyName.substr(sectionKeySeparatorPos + 1), outputKey.value().GetValue());
417 + configFileOutput += keyValue;
418 + outputKeyValueUpdated = true;
419 + }
420 + }
421 goto ParseSection;
422
423 default:
@@ -418,30 +446,6 @@ NewLine:
446 }
447
448 ParseSection:
421 - if (updateConfigFile && !outputKeyValueUpdated && !removeKey && sectionLength > 0)
422 - {
423 - const auto& outputConfigKey = outputKey.value();
424 - if (outputConfigKey.Matches(key.c_str(), sectionLength))
425 - {
426 - const auto& keyNames = outputConfigKey.GetNames();
427 - // Config key without name.
428 - FAIL_FAST_IF(keyNames.empty());
429 - const auto keyNameUtf8 = keyNames.front();
430 - const auto keyName = wsl::shared::string::MultiByteToWide(keyNameUtf8);
431 - const auto sectionKeySeparatorPos = keyName.find('.');
432 - // Config key without separated section/key name
433 - FAIL_FAST_IF(sectionKeySeparatorPos == std::string_view::npos);
434 - // Config key without section name
435 - FAIL_FAST_IF(sectionKeySeparatorPos == 0);
436 - // Config key without key name
437 - FAIL_FAST_IF(sectionKeySeparatorPos == (keyName.length() - 1));
438 -
439 - auto keyValue = std::format(L"\n{}={}", keyName.substr(sectionKeySeparatorPos + 1), outputKey.value().GetValue());
440 - configFileOutput.insert(newKeyValueInsertPos, keyValue);
441 - outputKeyValueUpdated = true;
442 - }
443 - }
444 -
449 // parse [section] ([ is already parsed)
450 if (updateConfigFile)
451 {
@@ -796,7 +800,6 @@ ValueDone:
800 // Trim any trailing space.
801 value.resize(trimmedLength);
802 SetConfig(keys, key.c_str(), value.c_str(), flags & CFG_DEBUG, filePath, line);
799 - newKeyValueInsertPos = configFileOutput.length();
803 }
804
805 goto NewLine;
test/windows/UnitTests.cpp
+202
@@ -3517,6 +3517,208 @@ localhostForwarding=true
3517 configRead.close();
3518 VERIFY_ARE_EQUAL(customWslConfigContentActual, customWslConfigContentExpected);
3519 }
3520 +
3521 + // Regression test for GitHub issue #12671:
3522 + // Ensure that section headers always appear BEFORE their key-value pairs.
3523 + // Bug: WSL Settings GUI was writing keys before the section header, causing "Unknown key" errors.
3524 + {
3525 + std::wstring bugScenarioConfig =
3526 + LR"([wsl2]
3527 +[experimental]
3528 +[wsl2]
3529 +)";
3530 + WslConfigChange config{bugScenarioConfig.c_str()};
3531 +
3532 + wslConfig = createWslConfig(apiWslConfigFilePath);
3533 + VERIFY_IS_NOT_NULL(wslConfig);
3534 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3535 +
3536 + // Write memory setting - this should NOT appear before the first [wsl2]
3537 + WslConfigSetting memorySetting{};
3538 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3539 + memorySetting.UInt64Value = 17825792000ULL; // Value from bug report
3540 +
3541 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3542 +
3543 + // Read and verify
3544 + std::wifstream configRead(apiWslConfigFilePath);
3545 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3546 + configRead.close();
3547 +
3548 + // Find FIRST occurrence of [wsl2] and memory=
3549 + auto firstWsl2Pos = fileContent.find(L"[wsl2]");
3550 + auto memoryPos = fileContent.find(L"memory=");
3551 +
3552 + VERIFY_ARE_NOT_EQUAL(firstWsl2Pos, std::wstring::npos);
3553 + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos);
3554 +
3555 + // The critical assertion: memory= must NOT appear before [wsl2]
3556 + VERIFY_IS_TRUE(firstWsl2Pos < memoryPos);
3557 +
3558 + // Additional check: memory should appear after the first [wsl2], not after line 1
3559 + auto firstLineEnd = fileContent.find(L'\n');
3560 + VERIFY_IS_TRUE(memoryPos > firstLineEnd);
3561 + }
3562 +
3563 + // Test: Empty file - should create proper [wsl2] section structure
3564 + {
3565 + std::wofstream emptyConfig(apiWslConfigFilePath, std::ios::trunc);
3566 + emptyConfig.close();
3567 +
3568 + wslConfig = createWslConfig(apiWslConfigFilePath);
3569 + VERIFY_IS_NOT_NULL(wslConfig);
3570 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3571 +
3572 + WslConfigSetting memorySetting{};
3573 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3574 + memorySetting.UInt64Value = 4294967296ULL; // 4GB
3575 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3576 +
3577 + std::wifstream configRead(apiWslConfigFilePath);
3578 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3579 + configRead.close();
3580 +
3581 + // Should create [wsl2] section and add memory key
3582 + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") != std::wstring::npos);
3583 + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos);
3584 + // Verify [wsl2] comes before memory=
3585 + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") < fileContent.find(L"memory="));
3586 + }
3587 +
3588 + // Test: Multiple same-section instances - should update first occurrence
3589 + {
3590 + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc);
3591 + configFile << L"[wsl2]\n";
3592 + configFile << L"processors=4\n";
3593 + configFile << L"\n";
3594 + configFile << L"[experimental]\n";
3595 + configFile << L"autoProxy=true\n";
3596 + configFile << L"\n";
3597 + configFile << L"[wsl2]\n"; // Second [wsl2] section
3598 + configFile << L"swap=0\n";
3599 + configFile.close();
3600 +
3601 + wslConfig = createWslConfig(apiWslConfigFilePath);
3602 + VERIFY_IS_NOT_NULL(wslConfig);
3603 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3604 +
3605 + WslConfigSetting memorySetting{};
3606 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3607 + memorySetting.UInt64Value = 8589934592ULL; // 8GB
3608 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3609 +
3610 + std::wifstream configRead(apiWslConfigFilePath);
3611 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3612 + configRead.close();
3613 +
3614 + // Find first and second [wsl2]
3615 + auto firstWsl2 = fileContent.find(L"[wsl2]");
3616 + auto secondWsl2 = fileContent.find(L"[wsl2]", firstWsl2 + 1);
3617 + auto memoryPos = fileContent.find(L"memory=");
3618 +
3619 + VERIFY_ARE_NOT_EQUAL(firstWsl2, std::wstring::npos);
3620 + VERIFY_ARE_NOT_EQUAL(secondWsl2, std::wstring::npos);
3621 + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos);
3622 +
3623 + // Memory should be added to FIRST [wsl2] section, not second
3624 + VERIFY_IS_TRUE(memoryPos > firstWsl2);
3625 + VERIFY_IS_TRUE(memoryPos < secondWsl2);
3626 + }
3627 +
3628 + // Test: EOF without trailing newline
3629 + {
3630 + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc);
3631 + configFile << L"[wsl2]\n";
3632 + configFile << L"processors=2"; // No trailing newline
3633 + configFile.close();
3634 +
3635 + wslConfig = createWslConfig(apiWslConfigFilePath);
3636 + VERIFY_IS_NOT_NULL(wslConfig);
3637 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3638 +
3639 + WslConfigSetting memorySetting{};
3640 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3641 + memorySetting.UInt64Value = 3221225472ULL; // 3GB
3642 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3643 +
3644 + std::wifstream configRead(apiWslConfigFilePath);
3645 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3646 + configRead.close();
3647 +
3648 + // Should properly append memory key even without trailing newline on last line
3649 + VERIFY_IS_TRUE(fileContent.find(L"processors=2") != std::wstring::npos);
3650 + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos);
3651 +
3652 + // Verify both keys are in the same section
3653 + auto wsl2Pos = fileContent.find(L"[wsl2]");
3654 + auto processorsPos = fileContent.find(L"processors=2");
3655 + auto memoryPos = fileContent.find(L"memory=");
3656 + VERIFY_IS_TRUE(wsl2Pos < processorsPos);
3657 + VERIFY_IS_TRUE(wsl2Pos < memoryPos);
3658 +
3659 + // Memory should come after processors in the same section
3660 + VERIFY_IS_TRUE(processorsPos < memoryPos);
3661 + }
3662 +
3663 + // Test: Empty section followed by another section
3664 + {
3665 + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc);
3666 + configFile << L"[wsl2]\n";
3667 + configFile << L"[experimental]\n";
3668 + configFile << L"autoProxy=true\n";
3669 + configFile.close();
3670 +
3671 + wslConfig = createWslConfig(apiWslConfigFilePath);
3672 + VERIFY_IS_NOT_NULL(wslConfig);
3673 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3674 +
3675 + WslConfigSetting memorySetting{};
3676 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3677 + memorySetting.UInt64Value = 5368709120ULL; // 5GB
3678 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3679 +
3680 + std::wifstream configRead(apiWslConfigFilePath);
3681 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3682 + configRead.close();
3683 +
3684 + // Should insert memory into empty [wsl2] section before [experimental]
3685 + auto wsl2Pos = fileContent.find(L"[wsl2]");
3686 + auto memoryPos = fileContent.find(L"memory=");
3687 + auto experimentalPos = fileContent.find(L"[experimental]");
3688 +
3689 + VERIFY_ARE_NOT_EQUAL(wsl2Pos, std::wstring::npos);
3690 + VERIFY_ARE_NOT_EQUAL(memoryPos, std::wstring::npos);
3691 + VERIFY_ARE_NOT_EQUAL(experimentalPos, std::wstring::npos);
3692 +
3693 + // Order should be: [wsl2], memory=, [experimental]
3694 + VERIFY_IS_TRUE(wsl2Pos < memoryPos);
3695 + VERIFY_IS_TRUE(memoryPos < experimentalPos);
3696 + }
3697 +
3698 + // Test: Section header at EOF with no content
3699 + {
3700 + std::wofstream configFile(apiWslConfigFilePath, std::ios::trunc);
3701 + configFile << L"[wsl2]"; // Section at EOF, no newline, no content
3702 + configFile.close();
3703 +
3704 + wslConfig = createWslConfig(apiWslConfigFilePath);
3705 + VERIFY_IS_NOT_NULL(wslConfig);
3706 + auto cleanupWslConfig = wil::scope_exit([&] { freeWslConfig(wslConfig); });
3707 +
3708 + WslConfigSetting memorySetting{};
3709 + memorySetting.ConfigEntry = WslConfigEntry::MemorySizeBytes;
3710 + memorySetting.UInt64Value = 6442450944ULL; // 6GB
3711 + VERIFY_ARE_EQUAL(setWslConfigSetting(wslConfig, memorySetting), ERROR_SUCCESS);
3712 +
3713 + std::wifstream configRead(apiWslConfigFilePath);
3714 + std::wstring fileContent{std::istreambuf_iterator<wchar_t>(configRead), {}};
3715 + configRead.close();
3716 +
3717 + // Should properly add key to section at EOF
3718 + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") != std::wstring::npos);
3719 + VERIFY_IS_TRUE(fileContent.find(L"memory=") != std::wstring::npos);
3720 + VERIFY_IS_TRUE(fileContent.find(L"[wsl2]") < fileContent.find(L"memory="));
3721 + }
3722 }
3723
3724 TEST_METHOD(LaunchWslSettingsFromProtocol)