Fix memory leak in GetFileShares#25896
Conversation
xtqqczze
force-pushed
the
NetApiBufferFree
branch
from
b1c6fe2 to
da846eb
Compare
|
Bug fix and refactoring must be in separated PRs. While I agree with the bug fix I am not sure we want return back to unsafe code. |
xtqqczze
force-pushed
the
NetApiBufferFree
branch
from
da846eb to
b0f487a
Compare
rebased for bug fix only b0f487a |
| { | ||
|
|
||
| [LibraryImport("Netapi32.dll")] | ||
| internal static partial int NetApiBufferFree(nint Buffer); |
There was a problem hiding this comment.
| internal static partial int NetApiBufferFree(nint Buffer); | |
| internal static partial uint NetApiBufferFree(nint Buffer); |
There was a problem hiding this comment.
Yes, this is correct, but I kept int for consistency with NetShareEnum, rather than refactoring in this PR.
There was a problem hiding this comment.
Since it is new definition we should make it correct.
There was a problem hiding this comment.
The correct definition according to C#/Win32 P/Invoke Source Generator is:
internal static extern unsafe uint NetApiBufferFree([Optional] void* Buffer);So the Buffer parameter should also be typed as an unmanaged pointer, not an integer.
There was a problem hiding this comment.
We have no need to use unsafe context.
In original definition LPVOID is used. It is IntPtr in C# or nint.
There was a problem hiding this comment.
We have no need to use
unsafecontext. In original definitionLPVOIDis used. It isIntPtrin C# ornint.
I suggest using an unmanaged pointer (void*) rather than nint.
LPVOID means "any pointer", not a handle. Mapping it to void* better reflects the native contract, whereas nint is more appropriate for handles or opaque values. This keeps the API surface closer to Win32 and clearer for maintainers.
There was a problem hiding this comment.
(LPVOID is a native pointer. In C# it can be IntPtr or nint. )
The main thing here is that we should follow the style used earlier. It comes from what the standard source generator usually does.
microsoft-github-policy-service
Bot
added
the
Review - Needed
iSazonov
added
the
CL-General
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
microsoft-github-policy-service
Bot
removed
the
Review - Needed
…BufferFree.cs Co-authored-by: Ilya <darpa@yandex.ru>
|
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
📣 Hey @@xtqqczze, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
Refactoring is in #26155. |
2 participants
Documentation for
NetShareEnumstates: "This buffer is allocated by the system and must be freed using theNetApiBufferFreefunction."References: NetShareEnum function
Fix #26137