Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

rjmholt · 2019-08-01T17:56:47Z

Fixes PowerShell/vscode-powershell#1994

rjmholt · 2019-08-01T20:22:09Z

Wait, it looks like we already have a test for a path like this and it's expected as a possible in-memory input path:

PowerShellEditorServices/test/PowerShellEditorServices.Test/Session/WorkspaceTests.cs

Line 163 in fd77c0d

    
           string shortUriForm = "git:/c%3A/Users/Keith/GitHub/dahlbyk/posh-git/src/PoshGitTypes.ps1?%7B%22path%22%3A%22c%3A%5C%5CUsers%5C%5CKeith%5C%5CGitHub%5C%5Cdahlbyk%5C%5Cposh-git%5C%5Csrc%5C%5CPoshGitTypes.ps1%22%2C%22ref%22%3A%22~%22%7D";

@rkeithhill is it possible this is already fixed?

TylerLeonhardt · 2019-08-01T22:28:56Z

@rjmholt multiple folks mentioned the debug adapter in the issues... it could be a different code path?

rkeithhill · 2019-08-02T01:19:43Z

src/PowerShellEditorServices/Workspace/Workspace.cs

@@ -174,6 +180,20 @@ public ScriptFile GetFile(string filePath)
        /// <param name="scriptFile">The out parameter that will contain the ScriptFile object.</param>
        public bool TryGetFile(string filePath, out ScriptFile scriptFile)
        {
+            try
+            {
+                if (filePath.Contains(":/") // Quick heuristic to determine if we might have a URI


I thought I had suggested a code change like this before but we eventually decided that the GetFile() call on line 199 would throw in this case and we'd return false. That said, I'd rather test this in a way that doesn't rely on catching an exception.

Sorry @rkeithhill, are you for this change?

My worry is that there seems to be a test that exists already for a git:/ schemed file. My change would just reject those (and anything else that isn't file, untitled or inmemory). The fact that we already have a test for this makes me feel like there is an expectation that we can understand and handle a URI like this -- does that seem right?

Those tests for git:/ and gitlens-git:/ look like my tests. I think the intent here was to have those uri schemes return true for IsInMemory which we used to signal that you can't do things like set breakpoints with an "in memory" document. In retrospect, perhaps that is not the best approach RE the way we used IsInMemory but the basic idea was to determine whether or not we had a file path we could provide to Set-PSBreakpoint.

In an ideal design, I could walk up to the ScriptFile/EditorDocument and query a property like CanHaveBreakpoints or something like that to determine whether or not an LSP text document could have a breakpoint set on it. But I think this is good as a stop-gap. Ship it!

rjmholt · 2019-08-02T12:38:54Z

multiple folks mentioned the debug adapter in the issues... it could be a different code path?

The stack trace I'm working against looks like this:

    System.AggregateException: One or more errors occurred. ---> System.ArgumentException: Illegal characters in path.
       at System.Security.Permissions.FileIOPermission.EmulateFileIOPermissionChecks(String fullPath)
       at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
       at Microsoft.PowerShell.EditorServices.Workspace.GetFile(String filePath)
       at Microsoft.PowerShell.EditorServices.Workspace.TryGetFile(String filePath, ScriptFile& scriptFile)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.DebugAdapter.<HandleSetBreakpointsRequest>d__30.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()
       --- End of inner exception stack trace ---
    ---> (Inner Exception #0) System.ArgumentException: Illegal characters in path.
       at System.Security.Permissions.FileIOPermission.EmulateFileIOPermissionChecks(String fullPath)
       at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
       at Microsoft.PowerShell.EditorServices.Workspace.GetFile(String filePath)
       at Microsoft.PowerShell.EditorServices.Workspace.TryGetFile(String filePath, ScriptFile& scriptFile)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.DebugAdapter.<HandleSetBreakpointsRequest>d__30.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()<---

So Workspace.TryGetFile seems like the right place. What I don't understand is that we seem to have already dealt with this kind of URI scheme elsewhere, but we're still throwing here...

TylerLeonhardt

LGTM

…hell#1000) * Fix git uri problem * Remove unused imports

…#1012) * Fix git uri problem * Remove unused imports

Fix git uri problem

Loading status checks…

34c7692

rjmholt added Issue-Bug Area-Debugging Backport-Legacy labels Aug 1, 2019

Remove unused imports

Loading status checks…

a90e8e2

rjmholt requested a review from TylerLeonhardt Aug 1, 2019

rkeithhill reviewed Aug 2, 2019

View changes

rkeithhill approved these changes Aug 6, 2019

View changes

TylerLeonhardt approved these changes Aug 6, 2019

View changes

rjmholt added a commit to rjmholt/PowerShellEditorServices that referenced this pull request Aug 21, 2019

Fix crash when setBreakpoint from VSCode sends a git:/ URI... (PowerS…

Loading status checks…

cd0e7bc

…hell#1000) * Fix git uri problem * Remove unused imports

rjmholt mentioned this pull request Aug 21, 2019

Fix crash when setBreakpoint from VSCode sends a git:/ URI... (#1000) #1012

Merged

rjmholt added Backport-Complete and removed Backport-Legacy labels Sep 10, 2019

rjmholt deleted the rjmholt:fix-git-uri branch Dec 11, 2019

PowerShell / PowerShellEditorServices

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

rjmholt commented Aug 1, 2019

rjmholt commented Aug 1, 2019

TylerLeonhardt commented Aug 1, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rjmholt commented Aug 2, 2019

TylerLeonhardt left a comment

PowerShell / PowerShellEditorServices

Join GitHub today

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

Fix crash when setBreakpoint from VSCode sends a git:/ URI... #1000

Conversation

rjmholt commented Aug 1, 2019

rjmholt commented Aug 1, 2019

TylerLeonhardt commented Aug 1, 2019

This comment has been minimized.

rkeithhill Aug 2, 2019 Collaborator

This comment has been minimized.

TylerLeonhardt Aug 6, 2019 Member

This comment has been minimized.

rkeithhill Aug 6, 2019 Collaborator

This comment has been minimized.

rjmholt Aug 6, 2019 Author Member

This comment has been minimized.

rkeithhill Aug 14, 2019 Collaborator

This comment has been minimized.

rkeithhill Aug 14, 2019 • edited Collaborator

rjmholt commented Aug 2, 2019

TylerLeonhardt left a comment

Essential cookies

Always active

Analytics cookies

rkeithhill Aug 2, 2019
Collaborator

TylerLeonhardt Aug 6, 2019
Member

rkeithhill Aug 6, 2019
Collaborator

rjmholt Aug 6, 2019
Author Member

rkeithhill Aug 14, 2019
Collaborator

rkeithhill Aug 14, 2019 •

edited

Collaborator