From dc7257ee04d0d69d1d2554b33f4901ed5c78ab25 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 1 Sep 2016 15:49:53 -0700 Subject: [PATCH 1/4] Fix #284: AbortException crashes when called more than once This change fixes an issue in PowerShellContext.AbortException which causes it to throw a NullReferenceException when called after the session has already been aborted. This issue manifests sometimes when the debug adapter finishes execution normally or when it terminates due to a script parsing error. It was initially reported in the following two issues for the PowerShell VS Code extension: PowerShell/vscode-powershell#243 PowerShell/vscode-powershell#273 The fix is to check whether the PowerShellContext.SessionState is Aborted or Disposed before proceeding with the abort. --- .../MessageProtocol/MessageDispatcher.cs | 6 ++++++ .../Server/DebugAdapter.cs | 11 ++++++++--- .../Session/PowerShellContext.cs | 6 ++++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/MessageProtocol/MessageDispatcher.cs b/src/PowerShellEditorServices.Protocol/MessageProtocol/MessageDispatcher.cs index d68123297..5b2be66b4 100644 --- a/src/PowerShellEditorServices.Protocol/MessageProtocol/MessageDispatcher.cs +++ b/src/PowerShellEditorServices.Protocol/MessageProtocol/MessageDispatcher.cs @@ -331,6 +331,12 @@ private void OnListenTaskCompleted(Task listenTask) { if (listenTask.IsFaulted) { + Logger.Write( + LogLevel.Error, + string.Format( + "MessageDispatcher loop terminated due to unhandled exception:\r\n\r\n{0}", + listenTask.Exception.ToString())); + this.OnUnhandledException(listenTask.Exception); } else if (listenTask.IsCompleted || listenTask.IsCanceled) diff --git a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs index 8cc3a56b1..70f5c8999 100644 --- a/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs +++ b/src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs @@ -201,15 +201,20 @@ protected Task HandleDisconnectRequest( if (e.NewSessionState == PowerShellContextState.Ready) { await requestContext.SendResult(null); - editorSession.PowerShellContext.SessionStateChanged -= handler; + this.editorSession.PowerShellContext.SessionStateChanged -= handler; // Stop the server await this.Stop(); } }; - editorSession.PowerShellContext.SessionStateChanged += handler; - editorSession.PowerShellContext.AbortExecution(); + // In some rare cases, the EditorSession will already be disposed + // so we shouldn't try to abort because PowerShellContext will be null + if (this.editorSession != null && this.editorSession.PowerShellContext != null) + { + this.editorSession.PowerShellContext.SessionStateChanged += handler; + this.editorSession.PowerShellContext.AbortExecution(); + } return Task.FromResult(true); } diff --git a/src/PowerShellEditorServices/Session/PowerShellContext.cs b/src/PowerShellEditorServices/Session/PowerShellContext.cs index 093d8c2c6..78c13798c 100644 --- a/src/PowerShellEditorServices/Session/PowerShellContext.cs +++ b/src/PowerShellEditorServices/Session/PowerShellContext.cs @@ -571,7 +571,8 @@ public async Task LoadHostProfiles() /// public void AbortExecution() { - if (this.SessionState != PowerShellContextState.Aborting) + if (this.SessionState != PowerShellContextState.Aborting && + this.SessionState != PowerShellContextState.Disposed) { Logger.Write(LogLevel.Verbose, "Execution abort requested..."); @@ -587,7 +588,8 @@ public void AbortExecution() { Logger.Write( LogLevel.Verbose, - "Execution abort requested while already aborting"); + string.Format( + $"Execution abort requested when already aborted (SessionState = {this.SessionState})")); } } From f08ad85c513b437c227a2385f5d7187e2fdba256 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Thu, 1 Sep 2016 16:24:16 -0700 Subject: [PATCH 2/4] Fix #285: User's PSScriptAnalyzer settings path not being used This change fixes an issue caused by the recent refactoring to use PSScriptAnalyzer as a module rather than a C# library. We forgot to pass along the user-specified settings path to the -Settings parameter of the Invoke-ScriptAnalyzer cmdlet. --- .../Server/LanguageServer.cs | 2 +- .../Analysis/AnalysisService.cs | 40 +++++++++++++++---- .../Session/EditorSession.cs | 10 ----- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index 0aaf479f9..38a12a5d7 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -366,7 +366,7 @@ protected async Task HandleDidChangeConfigurationNotification( string newSettingsPath = this.currentSettings.ScriptAnalysis.SettingsPath; if (!string.Equals(oldScriptAnalysisSettingsPath, newSettingsPath, StringComparison.OrdinalIgnoreCase)) { - this.editorSession.RestartAnalysisService(newSettingsPath); + this.editorSession.AnalysisService.SettingsPath = newSettingsPath; settingsPathChanged = true; } diff --git a/src/PowerShellEditorServices/Analysis/AnalysisService.cs b/src/PowerShellEditorServices/Analysis/AnalysisService.cs index 7ceceef1e..375af91e9 100644 --- a/src/PowerShellEditorServices/Analysis/AnalysisService.cs +++ b/src/PowerShellEditorServices/Analysis/AnalysisService.cs @@ -5,7 +5,6 @@ using Microsoft.PowerShell.EditorServices.Utility; using System; -using System.IO; using System.Linq; using System.Management.Automation.Runspaces; using System.Threading; @@ -44,6 +43,21 @@ public class AnalysisService : IDisposable #endregion // Private Fields + + #region Properties + + /// + /// Gets or sets the path to a settings file (.psd1) + /// containing PSScriptAnalyzer settings. + /// + public string SettingsPath + { + get; + set; + } + + #endregion + #region Constructors /// @@ -56,6 +70,7 @@ public AnalysisService(IConsoleHost consoleHost, string settingsPath = null) { try { + this.SettingsPath = settingsPath; this.analysisRunspace = RunspaceFactory.CreateRunspace(InitialSessionState.CreateDefault2()); this.analysisRunspace.ThreadOptions = PSThreadOptions.ReuseThread; this.analysisRunspace.Open(); @@ -219,17 +234,28 @@ private IEnumerable GetDiagnosticRecords(ScriptFile file) if (this.scriptAnalyzerModuleInfo != null) { - using (var ps = System.Management.Automation.PowerShell.Create()) + using (var powerShell = System.Management.Automation.PowerShell.Create()) { - ps.Runspace = this.analysisRunspace; + powerShell.Runspace = this.analysisRunspace; Logger.Write( LogLevel.Verbose, String.Format("Running PSScriptAnalyzer against {0}", file.FilePath)); - diagnosticRecords = ps.AddCommand("Invoke-ScriptAnalyzer") - .AddParameter("ScriptDefinition", file.Contents) - .AddParameter("IncludeRule", IncludedRules) - .Invoke(); + powerShell + .AddCommand("Invoke-ScriptAnalyzer") + .AddParameter("ScriptDefinition", file.Contents); + + // Use a settings file if one is provided, otherwise use the default rule list. + if (!string.IsNullOrWhiteSpace(this.SettingsPath)) + { + powerShell.AddParameter("Settings", this.SettingsPath); + } + else + { + powerShell.AddParameter("IncludeRule", IncludedRules); + } + + diagnosticRecords = powerShell.Invoke(); } } diff --git a/src/PowerShellEditorServices/Session/EditorSession.cs b/src/PowerShellEditorServices/Session/EditorSession.cs index 6352c046c..4bc539959 100644 --- a/src/PowerShellEditorServices/Session/EditorSession.cs +++ b/src/PowerShellEditorServices/Session/EditorSession.cs @@ -104,16 +104,6 @@ public void StartDebugSession(HostDetails hostDetails, ProfilePaths profilePaths this.Workspace = new Workspace(this.PowerShellContext.PowerShellVersion); } - /// - /// Restarts the AnalysisService so it can be configured with a new settings file. - /// - /// Path to the settings file. - public void RestartAnalysisService(string settingsPath) - { - this.AnalysisService?.Dispose(); - InstantiateAnalysisService(settingsPath); - } - internal void InstantiateAnalysisService(string settingsPath = null) { // Only enable the AnalysisService if the machine has PowerShell From 3308878f3f21a9266af5ca524b3d4134746e64c8 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 2 Sep 2016 11:39:20 -0700 Subject: [PATCH 3/4] Fix #287: Handle invalid path chars when resolving script references This change resolves an issue that appears when the user edits a script that has dot-sourced script references which contain invalid characters. The fix handles the exceptions that might be thrown in this case and writes out the path details to the log file. Related to PowerShell/vscode-powershell#274. --- .../Language/FindDotSourcedVisitor.cs | 4 +- .../Workspace/Workspace.cs | 55 ++++++++++++++++--- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs b/src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs index bbd0832e7..7ffda9c2a 100644 --- a/src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs +++ b/src/PowerShellEditorServices/Language/FindDotSourcedVisitor.cs @@ -34,9 +34,11 @@ public override AstVisitAction VisitCommand(CommandAst commandAst) { if (commandAst.InvocationOperator.Equals(TokenKind.Dot)) { - string fileName = commandAst.CommandElements[0].Extent.Text; + // Strip any quote characters off of the string + string fileName = commandAst.CommandElements[0].Extent.Text.Trim('\'', '"'); DotSourcedFiles.Add(fileName); } + return base.VisitCommand(commandAst); } } diff --git a/src/PowerShellEditorServices/Workspace/Workspace.cs b/src/PowerShellEditorServices/Workspace/Workspace.cs index ebe35d45b..24ca0932e 100644 --- a/src/PowerShellEditorServices/Workspace/Workspace.cs +++ b/src/PowerShellEditorServices/Workspace/Workspace.cs @@ -203,6 +203,12 @@ private void RecursivelyFindReferences( baseFilePath, referencedFileName); + // If there was an error resolving the string, skip this reference + if (resolvedScriptPath == null) + { + continue; + } + Logger.Write( LogLevel.Verbose, string.Format( @@ -287,18 +293,49 @@ private string GetBaseFilePath(string filePath) private string ResolveRelativeScriptPath(string baseFilePath, string relativePath) { - if (Path.IsPathRooted(relativePath)) + string combinedPath = null; + Exception resolveException = null; + + try { - return relativePath; + // If the path is already absolute there's no need to resolve it relatively + // to the baseFilePath. + if (Path.IsPathRooted(relativePath)) + { + return relativePath; + } + + // Get the directory of the original script file, combine it + // with the given path and then resolve the absolute file path. + combinedPath = + Path.GetFullPath( + Path.Combine( + baseFilePath, + relativePath)); + } + catch (NotSupportedException e) + { + // Occurs if the path is incorrectly formatted for any reason. One + // instance where this occurred is when a user had curly double-quote + // characters in their source instead of normal double-quotes. + resolveException = e; + } + catch (ArgumentException e) + { + // Occurs if the path contains invalid characters, specifically those + // listed in System.IO.Path.InvalidPathChars. + resolveException = e; } - // Get the directory of the original script file, combine it - // with the given path and then resolve the absolute file path. - string combinedPath = - Path.GetFullPath( - Path.Combine( - baseFilePath, - relativePath)); + if (resolveException != null) + { + Logger.Write( + LogLevel.Error, + $"Could not resolve relative script path\r\n" + + $" baseFilePath = {baseFilePath}\r\n " + + $" relativePath = {relativePath}\r\n\r\n" + + $"{resolveException.ToString()}"); + } return combinedPath; } From 70c7f7310b2ab16f0038840f5f637085258517da Mon Sep 17 00:00:00 2001 From: David Wilson Date: Fri, 2 Sep 2016 11:48:26 -0700 Subject: [PATCH 4/4] Bump version to 0.7.2, update CHANGELOG.md --- CHANGELOG.md | 7 +++++++ appveyor.yml | 2 +- .../PowerShellEditorServices/PowerShellEditorServices.psd1 | 2 +- test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs | 2 +- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46106ab70..970df874b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # PowerShell Editor Services Release History +## 0.7.2 +### Friday, September 2, 2016 + +- Fixed #284: PowerShellContext.AbortException crashes when called more than once +- Fixed #285: PSScriptAnalyzer settings are not being passed to Invoke-ScriptAnalyzer +- Fixed #287: Language service crashes when invalid path chars are used in dot-sourced script reference + ## 0.7.1 ### Tuesday, August 23, 2016 diff --git a/appveyor.yml b/appveyor.yml index 1660201d8..74b046bfb 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -5,7 +5,7 @@ clone_depth: 10 skip_tags: true environment: - core_version: '0.7.1' + core_version: '0.7.2' prerelease_name: '-beta' branches: diff --git a/module/PowerShellEditorServices/PowerShellEditorServices.psd1 b/module/PowerShellEditorServices/PowerShellEditorServices.psd1 index 060c3a52f..7d54617cc 100644 --- a/module/PowerShellEditorServices/PowerShellEditorServices.psd1 +++ b/module/PowerShellEditorServices/PowerShellEditorServices.psd1 @@ -12,7 +12,7 @@ RootModule = 'PowerShellEditorServices.psm1' # Version number of this module. -ModuleVersion = '0.7.1' +ModuleVersion = '0.7.2' # ID used to uniquely identify this module GUID = '9ca15887-53a2-479a-9cda-48d26bcb6c47' diff --git a/test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs b/test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs index 46baf3924..55eabdd65 100644 --- a/test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs +++ b/test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs @@ -34,7 +34,7 @@ protected async Task> LaunchService( string scriptPath = Path.Combine(modulePath, "Start-EditorServices.ps1"); // TODO: Need to determine the right module version programmatically! - string editorServicesModuleVersion = "0.7.1"; + string editorServicesModuleVersion = "0.7.2"; string scriptArgs = string.Format(