Skip to content

Harden Windows macros against dangling-else ambiguity - #41513

Merged
Ben Hillis (benhillis) merged 2 commits into
microsoft:masterfrom
Eamon2009:patch-8
Sep 3, 2026
Merged

Harden Windows macros against dangling-else ambiguity#41513
Ben Hillis (benhillis) merged 2 commits into
microsoft:masterfrom
Eamon2009:patch-8

Conversation

@Eamon2009

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Hardens EMIT_USER_WARNING and THROW_HR_WITH_USER_ERROR_IF in src/windows/common/ExecutionContext.h by wrapping them in do { ... } while (false) statement wrappers. This is the Windows counterpart to the Linux fix in #41504.

PR Checklist

  • Closes: Fixes dangling-else vulnerability on Windows (follow-up to Fix dangling-else bugs in FAIL_FAST_IF and EMIT_USER_WARNING macros #41504)
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->

Detailed Description of the Pull Request / Additional comments

Both EMIT_USER_WARNING and THROW_HR_WITH_USER_ERROR_IF were defined as bare if statements, leaving Windows callers vulnerable to dangling-else misassociation when the macros are used without braces:

if (condition)
    EMIT_USER_WARNING(L"msg");
else
    // This else incorrectly binds to the macro's inner if

This change wraps both macros in do { ... } while (false) so they always expand to a single statement. This brings the Windows header into parity with the already-hardened Linux definition from #41504 and prevents the outer else from binding to the inner if generated by the macro.

Validation Steps Performed

@Eamon2009
Eamon (Eamon2009) requested a review from a team as a code owner September 3, 2026 13:36
Copilot AI lite review requested due to automatic review settings September 3, 2026 13:36
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new do { ... } while (false) macro expansions are missing the trailing semicolons, making them inconsistent with other macros in the header and potentially breaking existing call sites that omitted ; previously.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens Windows-side user warning / user-error helper macros in ExecutionContext.h by wrapping them in do { ... } while (false) so they behave as a single statement and avoid dangling-else misassociation in if/else constructs.

Changes:

  • Wrap THROW_HR_WITH_USER_ERROR_IF in a do { ... } while (false) guard.
  • Wrap EMIT_USER_WARNING in a do { ... } while (false) guard.
File summaries
File Description
src/windows/common/ExecutionContext.h Wrapes the two macros in do { ... } while (false) to prevent dangling-else ambiguity in caller code.
Review details

Suppressed comments (1)

src/windows/common/ExecutionContext.h:47

  • This do { ... } while (false) wrapper is missing the trailing semicolon. Adding it makes the macro a complete statement (matching other macros in this header) and avoids forcing all call sites to include a trailing ; where the previous if (...) { ... } form did not require one.
#define EMIT_USER_WARNING(Warning) \
    do \
    { \
        if (::wsl::windows::common::ExecutionContext* context = ::wsl::windows::common::ExecutionContext::Current(); context != nullptr) \
        { \
            context->EmitUserWarning(Warning); \
        } \
    } while (false)
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/windows/common/ExecutionContext.h Outdated
@Eamon2009
Eamon (Eamon2009) marked this pull request as draft September 3, 2026 13:40
Copilot AI review requested due to automatic review settings September 3, 2026 13:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is a small, well-scoped macro hardening that preserves behavior while eliminating a confirmed control-flow ambiguity.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@Eamon2009
Eamon (Eamon2009) marked this pull request as ready for review September 3, 2026 13:52
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@benhillis

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@benhillis Ben Hillis (benhillis) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me, thank you for the contribution!

@benhillis
Ben Hillis (benhillis) merged commit 4896a69 into microsoft:master Sep 3, 2026
9 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.

3 participants