Fix up buffer management getting network roots#21068
Fix up buffer management getting network roots#21068jborean93 wants to merge 2 commits into
Conversation
iSazonov
left a comment
There was a problem hiding this comment.
@jborean93 Thanks for investigating the issue!
Since you removed #if Debug it would be great to add test for fallback code branch (name length > MAXPATH).
| { | ||
| // exclude null terminator | ||
| uncPath = uncBuffer.Slice(0, (int)bufferSize - 1).ToString(); | ||
| uncBuffer = rentedArray = ArrayPool<char>.Shared.Rent((int)bufferSize); |
There was a problem hiding this comment.
There's a problem here if there are multiple iterations (> 2). We must return the previous array.
In my opinion, this loop is unnecessary. But if you still want to keep it, the pattern should be similar to:
char[]? toReturn = rentedArray;
uncBuffer = rentedArray = ArrayPool<char>.Shared.Rent((int)bufferSize);
if (toReturn is not null)
{
ArrayPool<char>.Shared.Return(toReturn);
}
There was a problem hiding this comment.
I've updated the code to return the rented array in all cases, I've kept the loop for now but if others object I can change it again.
I've kept the loop because now it only calls the function once and shared the same logic for handling errors and how to read the output on success.
Fixes the logic used when getting the PSDrive DisplayRoot for network PSDrive paths to exclude null terminators.
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
I would love to but seems like the underlying Win32 API WNetAddConnection3 is unable to handle paths that exceed MAX_PATH. If someone knows how to do this I'm happy to add a test but for now the current test will check the most common scenario unlike before. |
If Windows 10+ support long pathы the function could do the same. I do not know, but it is possible. Or it could even be added in the future. |
I tested by calling the WNet API directly with both a normal and NT path prefix |
We should either remove the loop if this event is improbable or add a test. |
|
Someone is free to continue on with this PR if they wish to do so. I’ve got no desire to continue to work on something that most likely will continue to sit for weeks before it is even looked at by the pwsh team. |
|
Removed backport label as this is not merged |
PR Summary
Fixes the logic used when getting the PSDrive DisplayRoot for network PSDrive paths to exclude null terminators.
There are no tests added because the original issue was that we ran with DEBUG mode that didn't go across the problematic code. In this case we are now testing the problematic scenario which is the more common of the two.
PR Context
Fixes: #21064
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).