Skip to content

[Tools] Manual Validation - Fix Credential Manager native memory handling#401901

Merged
stephengillie merged 2 commits into
microsoft:masterfrom
Madhusudhan-MSFT:user/masudars/mv-cred-free-fix
Jul 14, 2026
Merged

[Tools] Manual Validation - Fix Credential Manager native memory handling#401901
stephengillie merged 2 commits into
microsoft:masterfrom
Madhusudhan-MSFT:user/masudars/mv-cred-free-fix

Conversation

@Madhusudhan-MSFT

@Madhusudhan-MSFT Madhusudhan-MSFT commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Two follow-up fixes to the CredManager.Util helper added in #396900. Its CredRead/CredWrite buffers were released with mismatched allocators and only on the success path. This corrects the frees and makes them exception-safe. The same changes are applied to both the PowerShell embedded C# and the .cs mirror so the two stay in sync.

Changes

  • Match allocators to APIs: release the CredRead buffer with the CredFree API (via a new CredRelease P/Invoke) instead of Marshal.FreeHGlobal; remove the duplicate free of credentialBlob, which is an interior pointer into that same buffer rather than a separate allocation; free the write blob with Marshal.FreeCoTaskMem to match its StringToCoTaskMemUni allocation.
  • Correct P/Invoke signature: declare CredRelease as void to match the native CredFree.
  • Always release once allocated: wrap both methods in try/finally so the buffer is freed on both success and exception paths, and guard the raw CredFree call with a non-null check.

How Validated

  • The embedded C# compiles and loads on Windows PowerShell 5.1 and pwsh 7. A standalone round-trip (write, read back, verify the returned values) succeeds, and the not-found path returns cleanly.

Context

Microsoft Reviewers: Open in CodeFlow

…tching allocators

## Problem

- The CredRead buffer was released with `Marshal.FreeHGlobal` instead of the `CredFree` API it requires, and `credentialBlob` -- an interior pointer into that same buffer -- was then freed a second time.
- The write path freed a `StringToCoTaskMemUni` allocation with `Marshal.FreeHGlobal` instead of `Marshal.FreeCoTaskMem`.

## Solution

- Add a `CredFree` P/Invoke (`CredRelease`) and release the CredRead buffer once with it in `GetUserCredential`; drop the duplicate `credentialBlob` free.
- Free the write blob with `Marshal.FreeCoTaskMem` to match its allocator.
- Apply the same fix to both the PowerShell embedded C# and the .cs mirror, and declare `CredRelease` as `void` to match the native signature.

## Impact

- Credential read/write no longer performs mismatched or duplicate frees; observable behavior is otherwise unchanged.

## How Validated

- Embedded C# compiles and loads on Windows PowerShell 5.1 and pwsh 7; standalone PoC round-trips a credential across repeated read/write with no crash.
## Problem

- `GetUserCredential` freed the CredRead buffer only after the marshaling steps succeeded, so an exception during `PtrToStructure`/`Copy`/construction would leak the native buffer.
- `SetUserCredential` freed the write blob only after a successful `CredWrite`; a write failure threw before the free, leaking the allocation on every failed write.

## Solution

- Wrap the credential body in `try/finally` in both methods so the buffer is always released once it has been allocated, on both success and exception paths.
- Guard the raw `CredFree` P/Invoke with a non-null check (`credPtr != IntPtr.Zero`) as defense-in-depth.
- Apply identically to the PowerShell embedded C# and the .cs mirror.

## How Validated

- Embedded C# compiles and loads on Windows PowerShell 5.1 and pwsh 7.
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs-Attention This work item needs to be reviewed by a member of the core team. label Jul 14, 2026
@Madhusudhan-MSFT
Madhusudhan-MSFT marked this pull request as ready for review July 14, 2026 00:31
@stephengillie
stephengillie merged commit 0464dd8 into microsoft:master Jul 14, 2026
3 checks passed
@microsoft-github-policy-service microsoft-github-policy-service Bot added Moderator-Approved One of the Moderators has reviewed and approved this PR and removed Needs-Attention This work item needs to be reviewed by a member of the core team. labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Moderator-Approved One of the Moderators has reviewed and approved this PR Project-File

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants