Skip to content

Commit bbe259d

Browse files
rjmholtrjmholt
authored
Fix file recursion overflow problems (#795)
* Recurse directories manually with a stack * Add recursion limit + logging when it's hit * Change logging of exceptions * Add "handled" exception log method
1 parent f93653b commit bbe259d

File tree

3 files changed

+179
-54
lines changed

3 files changed

+179
-54
lines changed

‎src/PowerShellEditorServices/Utility/Logging.cs

+15
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,21 @@ void WriteException(
7878
[CallerMemberName] string callerName = null,
7979
[CallerFilePath] string callerSourceFile = null,
8080
[CallerLineNumber] int callerLineNumber = 0);
81+
82+
/// <summary>
83+
/// Log an exception that has been handled cleanly or is otherwise not expected to cause problems in the logs.
84+
/// </summary>
85+
/// <param name="errorMessage">The error message of the exception to be logged.</param>
86+
/// <param name="exception">The exception itself that has been thrown.</param>
87+
/// <param name="callerName">The name of the method in which the ILogger is being called.</param>
88+
/// <param name="callerSourceFile">The name of the source file in which the ILogger is being called.</param>
89+
/// <param name="callerLineNumber">The line number in the file where the ILogger is being called.</param>
90+
void WriteHandledException(
91+
string errorMessage,
92+
Exception exception,
93+
[CallerMemberName] string callerName = null,
94+
[CallerFilePath] string callerSourceFile = null,
95+
[CallerLineNumber] int callerLineNumber = 0);
8196
}
8297

8398

‎src/PowerShellEditorServices/Utility/PsesLogger.cs

+77-14
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,22 @@ namespace Microsoft.PowerShell.EditorServices.Utility
1111
/// </summary>
1212
public class PsesLogger : ILogger
1313
{
14+
/// <summary>
15+
/// The standard log template for all log entries.
16+
/// </summary>
17+
private static readonly string s_logMessageTemplate =
18+
"[{LogLevelName:l}] tid:{ThreadId} in '{CallerName:l}' {CallerSourceFile:l} (line {CallerLineNumber}):{IndentedLogMsg:l}";
19+
1420
/// <summary>
1521
/// The name of the ERROR log level.
1622
/// </summary>
1723
private static readonly string ErrorLevelName = LogLevel.Error.ToString().ToUpper();
1824

25+
/// <summary>
26+
/// The name of the WARNING log level.
27+
/// </summary>
28+
private static readonly string WarningLevelName = LogLevel.Warning.ToString().ToUpper();
29+
1930
/// <summary>
2031
/// The internal Serilog logger to log to.
2132
/// </summary>
@@ -51,31 +62,46 @@ public void Write(
5162
[CallerMemberName] string callerName = null,
5263
[CallerFilePath] string callerSourceFile = null,
5364
[CallerLineNumber] int callerLineNumber = 0)
65+
{
66+
Write(logLevel, new StringBuilder(logMessage), callerName, callerSourceFile, callerLineNumber);
67+
}
68+
69+
/// <summary>
70+
/// Write a message with the given severity to the logs. Takes a StringBuilder to allow for minimal allocation.
71+
/// </summary>
72+
/// <param name="logLevel">The severity level of the log message.</param>
73+
/// <param name="logMessage">The log message itself in StringBuilder form for manipulation.</param>
74+
/// <param name="callerName">The name of the calling method.</param>
75+
/// <param name="callerSourceFile">The name of the source file of the caller.</param>
76+
/// <param name="callerLineNumber">The line number where the log is being called.</param>
77+
private void Write(
78+
LogLevel logLevel,
79+
StringBuilder logMessage,
80+
[CallerMemberName] string callerName = null,
81+
[CallerFilePath] string callerSourceFile = null,
82+
[CallerLineNumber] int callerLineNumber = 0)
5483
{
5584
string indentedLogMsg = IndentMsg(logMessage);
5685
string logLevelName = logLevel.ToString().ToUpper();
5786

5887
int threadId = Thread.CurrentThread.ManagedThreadId;
5988

60-
string messageTemplate =
61-
"[{LogLevelName:l}] tid:{threadId} in '{CallerName:l}' {CallerSourceFile:l}:{CallerLineNumber}:{IndentedLogMsg:l}";
62-
6389
switch (logLevel)
6490
{
6591
case LogLevel.Diagnostic:
66-
_logger.Verbose(messageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
92+
_logger.Verbose(s_logMessageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
6793
return;
6894
case LogLevel.Verbose:
69-
_logger.Debug(messageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
95+
_logger.Debug(s_logMessageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
7096
return;
7197
case LogLevel.Normal:
72-
_logger.Information(messageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
98+
_logger.Information(s_logMessageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
7399
return;
74100
case LogLevel.Warning:
75-
_logger.Warning(messageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
101+
_logger.Warning(s_logMessageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
76102
return;
77103
case LogLevel.Error:
78-
_logger.Error(messageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
104+
_logger.Error(s_logMessageTemplate, logLevelName, threadId, callerName, callerSourceFile, callerLineNumber, indentedLogMsg);
79105
return;
80106
}
81107
}
@@ -95,26 +121,63 @@ public void WriteException(
95121
[CallerFilePath] string callerSourceFile = null,
96122
[CallerLineNumber] int callerLineNumber = 0)
97123
{
98-
string indentedException = IndentMsg(exception.ToString());
124+
StringBuilder body = FormatExceptionMessage("Exception", errorMessage, exception);
125+
Write(LogLevel.Error, body, callerName, callerSourceFile, callerLineNumber);
126+
}
99127

100-
_logger.Error("[{ErrorLevelName:l}] {CallerSourceFile:l}: In method '{CallerName:l}', line {CallerLineNumber}: {ErrorMessage:l}{IndentedException:l}",
101-
ErrorLevelName, callerSourceFile, callerName, callerLineNumber, errorMessage, indentedException);
128+
/// <summary>
129+
/// Log an exception that has been handled cleanly or is otherwise not expected to cause problems in the logs.
130+
/// </summary>
131+
/// <param name="errorMessage">The error message of the exception to be logged.</param>
132+
/// <param name="exception">The exception itself that has been thrown.</param>
133+
/// <param name="callerName">The name of the method in which the ILogger is being called.</param>
134+
/// <param name="callerSourceFile">The name of the source file in which the ILogger is being called.</param>
135+
/// <param name="callerLineNumber">The line number in the file where the ILogger is being called.</param>
136+
public void WriteHandledException(
137+
string errorMessage,
138+
Exception exception,
139+
[CallerMemberName] string callerName = null,
140+
[CallerFilePath] string callerSourceFile = null,
141+
[CallerLineNumber] int callerLineNumber = 0)
142+
{
143+
StringBuilder body = FormatExceptionMessage("Handled exception", errorMessage, exception);
144+
Write(LogLevel.Warning, body, callerName, callerSourceFile, callerLineNumber);
102145
}
103146

104147
/// <summary>
105148
/// Utility function to indent a log message by one level.
106149
/// </summary>
107-
/// <param name="logMessage">The log message to indent.</param>
150+
/// <param name="logMessageBuilder">Log message string builder to transform.</param>
108151
/// <returns>The indented log message string.</returns>
109-
private static string IndentMsg(string logMessage)
152+
private static string IndentMsg(StringBuilder logMessageBuilder)
110153
{
111-
return new StringBuilder(logMessage)
154+
return logMessageBuilder
112155
.Replace(Environment.NewLine, s_indentedPrefix)
113156
.Insert(0, s_indentedPrefix)
114157
.AppendLine()
115158
.ToString();
116159
}
117160

161+
/// <summary>
162+
/// Creates a prettified log message from an exception.
163+
/// </summary>
164+
/// <param name="messagePrelude">The user-readable tag for this exception entry.</param>
165+
/// <param name="errorMessage">The user-readable short description of the error.</param>
166+
/// <param name="exception">The exception object itself. Must not be null.</param>
167+
/// <returns>An indented, formatted string of the body.</returns>
168+
private static StringBuilder FormatExceptionMessage(
169+
string messagePrelude,
170+
string errorMessage,
171+
Exception exception)
172+
{
173+
var sb = new StringBuilder()
174+
.Append(messagePrelude).Append(": ").Append(errorMessage).Append(Environment.NewLine)
175+
.Append(Environment.NewLine)
176+
.Append(exception.ToString());
177+
178+
return sb;
179+
}
180+
118181
/// <summary>
119182
/// A newline followed by a single indentation prefix.
120183
/// </summary>

‎src/PowerShellEditorServices/Workspace/Workspace.cs

+87-40
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ public class Workspace
2222
{
2323
#region Private Fields
2424

25+
private static readonly string[] s_psFilePatterns = new []
26+
{
27+
"*.ps1",
28+
"*.psm1",
29+
"*.psd1"
30+
};
31+
2532
private ILogger logger;
2633
private Version powerShellVersion;
2734
private Dictionary<string, ScriptFile> workspaceFiles = new Dictionary<string, ScriptFile>();
@@ -292,67 +299,107 @@ public IEnumerable<string> EnumeratePSFiles()
292299

293300
#region Private Methods
294301

302+
/// <summary>
303+
/// Find PowerShell files recursively down from a given directory path.
304+
/// Currently returns files in depth-first order.
305+
/// Directory.GetFiles(folderPath, pattern, SearchOption.AllDirectories) would provide this,
306+
/// but a cycle in the filesystem will cause that to enter an infinite loop.
307+
/// </summary>
308+
/// <param name="folderPath">The absolute path of the base folder to search.</param>
309+
/// <returns>
310+
/// All PowerShell files in the recursive directory hierarchy under the given base directory, up to 64 directories deep.
311+
/// </returns>
295312
private IEnumerable<string> RecursivelyEnumerateFiles(string folderPath)
296313
{
297-
var foundFiles = Enumerable.Empty<string>();
298-
var patterns = new string[] { @"*.ps1", @"*.psm1", @"*.psd1" };
314+
var foundFiles = new List<string>();
315+
var dirStack = new Stack<string>();
299316

300-
try
317+
// Kick the search off with the base directory
318+
dirStack.Push(folderPath);
319+
320+
const int recursionDepthLimit = 64;
321+
while (dirStack.Any())
301322
{
302-
IEnumerable<string> subDirs = Directory.GetDirectories(folderPath);
303-
foreach (string dir in subDirs)
323+
string currDir = dirStack.Pop();
324+
325+
// Look for any PowerShell files in the current directory
326+
foreach (string pattern in s_psFilePatterns)
304327
{
305-
foundFiles =
306-
foundFiles.Concat(
307-
RecursivelyEnumerateFiles(dir));
328+
string[] psFiles;
329+
try
330+
{
331+
psFiles = Directory.GetFiles(currDir, pattern, SearchOption.TopDirectoryOnly);
332+
}
333+
catch (DirectoryNotFoundException e)
334+
{
335+
this.logger.WriteHandledException(
336+
$"Could not enumerate files in the path '{currDir}' due to it being an invalid path",
337+
e);
338+
339+
continue;
340+
}
341+
catch (PathTooLongException e)
342+
{
343+
this.logger.WriteHandledException(
344+
$"Could not enumerate files in the path '{currDir}' due to the path being too long",
345+
e);
346+
347+
continue;
348+
}
349+
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
350+
{
351+
this.logger.WriteHandledException(
352+
$"Could not enumerate files in the path '{currDir}' due to the path not being accessible",
353+
e);
354+
355+
continue;
356+
}
357+
358+
foundFiles.AddRange(psFiles);
308359
}
309-
}
310-
catch (DirectoryNotFoundException e)
311-
{
312-
this.logger.WriteException(
313-
$"Could not enumerate files in the path '{folderPath}' due to it being an invalid path",
314-
e);
315-
}
316-
catch (PathTooLongException e)
317-
{
318-
this.logger.WriteException(
319-
$"Could not enumerate files in the path '{folderPath}' due to the path being too long",
320-
e);
321-
}
322-
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
323-
{
324-
this.logger.WriteException(
325-
$"Could not enumerate files in the path '{folderPath}' due to the path not being accessible",
326-
e);
327-
}
328360

329-
foreach (var pattern in patterns)
330-
{
361+
// Prevent unbounded recursion here
362+
// If we get too deep, keep processing but go no deeper
363+
if (dirStack.Count >= recursionDepthLimit)
364+
{
365+
this.logger.Write(LogLevel.Warning, $"Recursion depth limit hit for path {folderPath}");
366+
continue;
367+
}
368+
369+
// Add the recursive directories to search next
370+
string[] subDirs;
331371
try
332372
{
333-
foundFiles =
334-
foundFiles.Concat(
335-
Directory.GetFiles(
336-
folderPath,
337-
pattern));
373+
subDirs = Directory.GetDirectories(currDir);
338374
}
339375
catch (DirectoryNotFoundException e)
340376
{
341-
this.logger.WriteException(
342-
$"Could not enumerate files in the path '{folderPath}' due to a path being an invalid path",
377+
this.logger.WriteHandledException(
378+
$"Could not enumerate directories in the path '{currDir}' due to it being an invalid path",
343379
e);
380+
381+
continue;
344382
}
345383
catch (PathTooLongException e)
346384
{
347-
this.logger.WriteException(
348-
$"Could not enumerate files in the path '{folderPath}' due to a path being too long",
385+
this.logger.WriteHandledException(
386+
$"Could not enumerate directories in the path '{currDir}' due to the path being too long",
349387
e);
388+
389+
continue;
350390
}
351391
catch (Exception e) when (e is SecurityException || e is UnauthorizedAccessException)
352392
{
353-
this.logger.WriteException(
354-
$"Could not enumerate files in the path '{folderPath}' due to a path not being accessible",
393+
this.logger.WriteHandledException(
394+
$"Could not enumerate directories in the path '{currDir}' due to the path not being accessible",
355395
e);
396+
397+
continue;
398+
}
399+
400+
foreach (string subDir in subDirs)
401+
{
402+
dirStack.Push(subDir);
356403
}
357404
}
358405

0 commit comments

Comments
 (0)