Skip to content

Windows: gate showOperationDialog against modal reentrancy - #429

Merged
coneilen merged 1 commit into
scgopi:mainfrom
coneilen:coneilen-microsoft-fix-repository-dialog-reentrancy
Sep 23, 2026
Merged

coneilen merged 1 commit into
scgopi:mainfrom
coneilen:coneilen-microsoft-fix-repository-dialog-reentrancy

Conversation

@coneilen

Copy link
Copy Markdown
Collaborator

Fixes #427.

operation_dialog_active in WindowsRepositoryDialogs.zig was written but never read as an entry gate. It was set at :1073 with no preceding check, cleared manually at two separate sites (:1092, :1115), and its only read was a wndproc message filter at :1146. Nothing prevented reentry.

That mattered because operation_dialog_state is a single module-level variable overwritten before the modal is entered (:1053, :1065, :1074, :1075). A second call therefore did not just open a second window: it repointed the already-running modal at a different allocator, parent HWND, and operation, and reset closed/cancelled underneath the first modal's while (!operation_dialog_state.closed) loop, which later frees through operation_dialog_state.allocator. The mismatched EnableWindow(parent, 0) / EnableWindow(parent, 1) pairing could also leave a window permanently disabled.

Approach

Follows the precedent established by PR #420 for NativeForms.zig:

fn acquireOperationDialog() !void {
    if (operation_dialog_active) return error.OperationDialogAlreadyOpen;
    operation_dialog_active = true;
}
fn releaseOperationDialog() void { operation_dialog_active = false; }

Call sites use try acquireOperationDialog(); defer releaseOperationDialog();. The two manual clears are removed. defer is the substantive part of the fix, not stylistic: clearing the flag at two hand-placed sites meant any added early return or error path would leak the flag and wedge the dialog permanently.

Failure semantic

Fail fast, rejecting the second call before any shared-state write or remote worker start. The clone caller cancels the newly started operation on presentation error, so the rejected request is explicitly terminated and reported rather than silently dropped.

Deferral was considered and rejected: it would require transferring live operation and allocator ownership beyond the lifetime of this synchronous API. This differs deliberately from #420, which retained a deferred update offer because that offer was durable, replayable data rather than a live resource handle.

The guards at WindowsCodespaceDialog.zig:252 and WindowsRepositoryDialogs.zig:619 are intentionally untouched; they are synchronous user-initiated opens with a disabled owner window and no durable event, so dropping a duplicate is already correct there.

RED: new reentrancy test on unfixed code -> failed with expected error.OperationDialogAlreadyOpen, found void, confirming a second entry proceeded straight through
GREEN: after adding the acquire/release guard -> all 14 tests pass under pinned Zig 0.15.2, including the 13 pre-existing ones
REGRESSION: the new test asserts the flag is set while held, that a second acquire returns error.OperationDialogAlreadyOpen, and that it is cleared on release -> a future removal of the guard or of the defer re-fails it

Verification

Run under pinned Zig 0.15.2. WindowsRepositoryDialogs.zig is one of the orphaned files from #424 whose 13 tests had never been executed by any harness; all 13 pass, so no pre-existing defects were uncovered here. This file is deliberately not wired into WindowsShell.Tests.ps1 in this PR to avoid colliding with #426, which is actively rewriting that file; wiring it should follow #426.

@coneilen coneilen closed this Sep 23, 2026
@coneilen coneilen reopened this Sep 23, 2026
@coneilen
coneilen force-pushed the coneilen-microsoft-fix-repository-dialog-reentrancy branch 2 times, most recently from c5e02f4 to 62b53c0 Compare September 23, 2026 04:34
Acquire the operation modal before shared state is written and release it with defer on every exit path. Add a mutation-sensitive acquisition test covering nested and sequential opens.

Signed-off-by: Colin Neilens <coneilen@microsoft.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@coneilen
coneilen force-pushed the coneilen-microsoft-fix-repository-dialog-reentrancy branch from 62b53c0 to 9f5d8e2 Compare September 23, 2026 05:16
@coneilen
coneilen merged commit ca59803 into scgopi:main Sep 23, 2026
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WindowsRepositoryDialogs: showOperationDialog has no reentrancy guard and can overwrite in-flight operation state

1 participant