Publish the first version of the Windows 7 Region and Language Restorer mod - #5387
Publish the first version of the Windows 7 Region and Language Restorer mod#5387babamohammed2022 wants to merge 14 commits into
Conversation
|
/ai-review |
|
Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review. To get started, comment See the pull request review process for the full details. |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. An impressive amount of careful work — the PE mapping, the pinned-digest verification, the 1. The entire setup runs synchronously in
Your own merged mod already solves exactly this: in win7-display-control-panel-restorer.wh.cpp 2. The advertised "redirect-only, no downloads" Explorer mode is unreachable.
if (host == Host::Explorer) return true;so 3. Unload can hang indefinitely.
while (g_active.load(...) != 0 || g_jobs.load(...) != 0) {
CloseOwnedWindows();
HANDLE event = g_active.load() != 0 ? g_idle : g_jobsIdle;
if (event) WaitForSingleObject(event, 100);
...
}and the log line inside it ("Close child dialogs or the elevation prompt; code will not be unmapped while in use") acknowledges that the user has to intervene. Two concrete problems:
A hang on unload hangs the user's Windhawk operation, so this is worth designing around rather than documenting. Deferring the mapping until a sheet is actually requested (item 1) shrinks the window a lot; the 4. Resuming from arbitrary hardware faults by
5. A Windows 7
6. Runtime download of executable code from an external server. Windhawk's stated principle is that a mod is self-contained and doesn't fetch remote resources; 7. The file still carries the private development ledger and acceptance checklist. Lines 112-218 are a ~110-line "ANALYSIS LEDGER" / "ACCEPTANCE CHECKLIST" block with per-version proof lists. Please strip it down to a short provenance note (the pinned hashes/URLs/PE contract facts are genuinely useful; the per-version
8. The README has no screenshot. This mod restores a visible UI, and your other mods ( Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
Updated metadata and improved clarity in documentation.
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Impressive amount of work, and the pinning/verification of the payloads is careful. The findings below are concentrated in the unload path, which currently has a reachable crash and a reachable hang, and in the init path, which does a lot of blocking work in every Explorer session. 1.
if (!exists) for (auto& w : g_owned) if (!w) { w = window; break; } // line 5389Every 2. Forced teardown after 5 s while legacy code is still executing → use-after-free in the host.
if (GetTickCount64() - start > TIMEOUT_MS) {
Wh_Log(L"[IntlRestore] TIMEOUT: forcing unload after 5 seconds");
break; // lines 6456-6459
}and This is reachable in normal use, not a corner case: the Region sheet is modal, so There is no safe "force" here — the mod image cannot be unmapped while its code is on a stack. The wait needs to be unbounded once the close requests actually work (finding 1), and any remaining timeout should be a diagnostic, not a licence to tear down. 3.
while (g_jobs.load(std::memory_order_acquire) != 0) {
DWORD status = MsgWaitForMultipleObjectsEx(1, &g_jobsIdle, 100, QS_ALLINPUT, MWMO_INPUTAVAILABLE);
... // lines 5738-5747
}If a private Win7 worker never returns, Windhawk's unload never completes. That is a hang of the host process, and it is inconsistent with the bounded wait one callback earlier. Note this also pumps arbitrary messages on the Windhawk teardown thread. 4. Private worker threads are never joined; EndJob();
return result; // lines 5673-5674
}
Per the unloadability contract, keep the thread handles and 5. The whole provider setup — including a network download — runs synchronously in
All of that for a Control Panel page the user may never open in that session. Concrete fix: in 6. The crash guard does not guard anything, and its recovery machinery is dead code.
Correspondingly, nothing in the file ever writes
This matters beyond dead code: the safety argument for running foreign Win7 binaries inside explorer.exe rests on this guard. Either make it real (frame-based 7. Logging from an exception handler on stack overflow / heap corruption.
8. if (message == CPL_EXIT) WaitForJobs(); // line 6174
9. Runtime download of external binaries. The repository guideline is that a mod must be self-contained and must not fetch remote files or contact external servers; this mod downloads Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
|
It seems like that changing the architecture to amd64 makes the validation script fail while having it to x86-64 makes it work but the compatibility checks stop working. If it's a problem on my side, I'll try to fix it but I have not understood what is it in this case. |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The mapping/verification pipeline is careful work — the SHA-256 + pinned-handle + 1. The cold-cache setup (two HTTPS downloads) runs synchronously on the host UI thread.
if (g_useLegacy.load() && !g_legacyInitialized.load()) EnsurePrepared();
Your own merged 2.
version.dwBuildNumber < 10240 || version.wProductType != VER_NT_WORKSTATION) return false;On 1507/1511 the mod DLL fails to load outright (unresolved import) rather than degrading, so the advertised support range doesn't match reality. Either resolve it with 3. Unload waits are unbounded and the in-flight download isn't cancellable.
while (g_active.load(...) != 0 || g_jobs.load(...) != 0) {
CloseOwnedWindows();
...
HANDLE event = g_active.load() != 0 ? g_idle : g_jobsIdle;
if (event) WaitForSingleObject(event, 100);
}and
Please bound the wait (log and escalate after a few seconds), and make Related: 4. The crash guard doesn't contain crashes, and most of it is unreachable.
// DO NOT longjmp - let the exception continue search naturally.
// The system will handle it or ignore it. Explorer continues running.is not what happens: an access violation inside the mapped Windows 7 Consequently a large block of scaffolding is dead: nothing in the file calls Please either implement real containment or delete the machinery and the claims — right now a reader (and a future maintainer) is told the mod is crash-safe when it isn't, which matters given it runs unmodified Win7 binaries inside 5. Legacy void Cleanup() {
...
if (g_legacyInitialized.exchange(false) && g_image.cpl) {
GuardCpl(g_image.cpl, nullptr, CPL_EXIT, 0, 0, ignored, exception);
}
if (g_image.attached && g_image.entry) {
GuardEntry(g_image.entry, ..., DLL_PROCESS_DETACH, ignored, exception);
}
6. if (Equal(leaf, L"input.dll")) {
...
HMODULE input = LoadLibraryExW(g_inputPath.c_str(), nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32);
if (input != g_inputModule) { ... }
return input; // refcount incremented, handed to legacy code
}
Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
It's asking for a manual review, because often it is a mistake. In this case, it's probably OK. GitHub doesn't have a way to show warnings, so alerting it by returning an error is the only way. I'll think about improving it in the future, but for now, you can ignore it as long as there are no other failures. |
The mod seems to work fine for me at the moment. If the maintainer believes that these problems need to be addressed, I'll do it. In alternative, I can fix them in a future update. |
|
Fix 1 and 4 from the last review. Regarding "Vectored crash guard" - why do you need it? What crashes without it? Generally just swallowing all crashes in a system process is not a good idea, and I prefer not having mods that do that. |
I'll fix the points 1 and 4 and thanks for clarifying, I tried this approach to try to make the mod less likely to crash but I'll remove it to adhere to the policy. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The two things the maintainer asked for are done: 1. The 12-second "abandon" path converts an unload hang into a host crash.
This is reachable in ordinary use, not a corner case. The previous unbounded wait was recoverable — it ended as soon as the user closed the dialog. This isn't. Note also that the maintainer only asked for items 1 and 4 of the last review; the bounded wait wasn't requested. Suggested direction, in order of value:
Two smaller things on the same path:
2.
if (g_useLegacy.load() && !g_legacyInitialized.load()) EnsurePrepared();
It also defeats itself: const bool activation = message == CPL_DBLCLK || message == CPL_STARTWPARMSA || message == CPL_STARTWPARMSW;
...
if (g_useLegacy.load() && !g_legacyInitialized.load()) {
if (activation) EnsurePrepared();
else StartPrefetch(); // kick the fetch off, don't wait for it
}Relatedly, the README says the download "runs in the background and never blocks the Control Panel thread", which isn't accurate while this wait exists — worth rewording either way. 3. void StartPrefetch() {
if (g_stopping.load(std::memory_order_acquire)) return;
...
HANDLE thread = CreateThread(nullptr, 0, PrefetchThread, nullptr, 0, nullptr);
...
RegisterThread(thread);The AcquireSRWLockShared(&g_gate);
const bool allowed = !g_stopping.load(std::memory_order_acquire);
if (allowed) { /* CreateThread + RegisterThread here */ }
ReleaseSRWLockShared(&g_gate);Optional improvements
Minor polish — none of this affects users, so it's your call. The first five are unchanged from previous rounds.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The three findings from the last round are genuinely addressed: the 1. Hooks registered after
The eager path in Fix: apply after a successful registration. Your own merged Display restorer already does exactly this for its deferred hook surface (win7-display-control-panel-restorer.wh.cpp#L14831 and #L13009), and explorer-frame-classic.wh.cpp#L843-L845 shows the same shape from inside a loader hook: if (pinned && HookNativeCpl(pinned) && Wh_ApplyHookOperations()) {
g_explorerCplHooked.store(true, std::memory_order_release);
StartPrefetch();
Wh_Log(L"intl.cpl loaded in Explorer; CPlApplet hook installed");
} else if (pinned) {
FreeLibrary(pinned);
}and the same after 2. Runtime download of external binaries - noted for the record, not a request to you. The repository guideline is that a mod is self-contained and does not fetch remote resources; this one downloads Optional improvements
Minor polish - none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
Added error handling for CPlApplet hook registration and improved logging for various conditions related to the Control Panel integration.
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The last round's finding is properly fixed: both deferred registration sites now apply ( 1. Lines 8326-8350: if (code == HC_ACTION && !g_shutdownRan.exchange(true)) {
try { ShutdownLegacyOnCurrentThread(); } catch (...) { ... }
if (g_shutdownWorked) SetEvent(g_shutdownWorked); // <- releases the unload thread
}
return CallNextHookEx(nullptr, code, wParam, lParam); // <- still mod code, still runs
The fix is one line - do the chain call first, signal last, so that the only mod code after the signal is a LRESULT CALLBACK ShutdownHookProc(int code, WPARAM wParam, LPARAM lParam) {
const bool ran = code == HC_ACTION && !g_shutdownRan.exchange(true);
if (ran) {
try { ShutdownLegacyOnCurrentThread(); } catch (...) { /* as today */ }
}
const LRESULT result = CallNextHookEx(nullptr, code, wParam, lParam);
if (ran && g_shutdownWorked) SetEvent(g_shutdownWorked);
return result;
}That plus the existing sentinel-thread wake latency closes the window to the point where it stops being a realistic concern. As written, the window is "however long the rest of the hook chain takes". 2. Merely browsing the Control Panel folder maps the Win7 provider into Line 8111: if (!enumeration || g_prefetchSucceeded.load(std::memory_order_acquire)) EnsurePrepared();
else StartPrefetch();
Two consequences for a user who opens the Control Panel folder and never clicks Region:
You already have the machinery to avoid it: if (!enumeration) EnsurePrepared();
else StartPrefetch();
3. Runtime download of external binaries - noted for the record, not a request to you. The repository guideline is that a mod is self-contained and does not fetch remote resources; this one downloads Optional improvements
Minor polish - none of this affects users, so it's your call. Most of these are unchanged from previous rounds.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
@m417z I've tried to address the first problem reported by the AI reviewer. However, the amount of AI reviews for this day has reached its limit, so I can't receive another. Is the mod ready? |
Changelog
If this pull request updates an existing mod, describe the changes below:
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.