Skip to content

Replace CR and new line with a 0x23CE character #10616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityapatwardhan merged 8 commits into from
Oct 7, 2019
Merged

Replace CR and new line with a 0x23CE character #10616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f4e6e0e
Replace CR and new line with a 0x23CE character
TravisEz13 Sep 24, 2019
cf598e2
fix tests
TravisEz13 Sep 25, 2019
8c70dce
update logging
TravisEz13 Sep 25, 2019
1b8ebe2
fix codefactor issues
TravisEz13 Sep 25, 2019
9cfcc60
fix test issues
TravisEz13 Sep 25, 2019
07d23e2
address PR issues
TravisEz13 Oct 3, 2019
5947deb
try making tests more percise
TravisEz13 Oct 3, 2019
9ea47ab
remove dead code
TravisEz13 Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,11 +1520,7 @@ private static bool WriteScriptBlockToLog(ScriptBlock scriptBlock, int segment,

Comment thread
TravisEz13 marked this conversation as resolved.
Comment thread
TravisEz13 marked this conversation as resolved.
Comment thread
TravisEz13 marked this conversation as resolved.
Comment thread
TravisEz13 marked this conversation as resolved.
if (!wasEncoded)
{
// The logging mechanism(s) cannot handle null and rendering may not be able to handle
// null as we have the string defined as a null terminated string in the manifest.
// So, replace null characters with the Unicode `SYMBOL FOR NULL`
// We don't just remove the characters to preserve the fact that a null character was there.
textToLog = textToLog.Replace('\u0000', '\u2400');
textToLog = FormatLogString(textToLog);
}

if (scriptBlock._scriptBlockData.HasSuspiciousContent)
Expand Down Expand Up @@ -1557,6 +1553,50 @@ private static bool WriteScriptBlockToLog(ScriptBlock scriptBlock, int segment,
return true;
}

private static string FormatLogString(string textToLog)
{
const char NullControlChar = '\u0000';

// The null symbol - `␀`
const char NullSymbolChar = '\u2400';

// No logging mechanism(s) cannot handle null and rendering may not be able to handle
// null as we have the string defined as a null terminated string in the manifest.
// So, replace null characters with the Unicode `SYMBOL FOR NULL`
// We don't just remove the characters to preserve the fact that a null character was there.
#if UNIX
const char LinefeedControlChar = '\u000A';
const char CarriageReturnControlChar = '\u000D';

// We chose the return symbol because we believe it is more associated with these concepts
// than the carriage return (␍), line feed (␊), or new line (␤) symbols.
// The return symbol - `⏎`
const char ReturnSymbolChar = '\u23CE';

if (Platform.IsLinux)
{
// Because the creation of the string builder is expensive
// We only do this on Linux where we are doing multiple replace operations
StringBuilder logBuilder = new StringBuilder(textToLog);

logBuilder.Replace(NullControlChar, NullSymbolChar);

// Syslog (only used on Linux) encodes CR and NL to their octal values.
// We will replace them with a unicode 'RETURN SYMBOL' (U+23CE) charater for easier viewing
logBuilder.Replace(LinefeedControlChar, ReturnSymbolChar);
logBuilder.Replace(CarriageReturnControlChar, ReturnSymbolChar);

return logBuilder.ToString();
}
else
{
return textToLog.Replace(NullControlChar, NullSymbolChar);
}
#else
return textToLog.Replace(NullControlChar, NullSymbolChar);
#endif
}

private static bool GetAndValidateEncryptionRecipients(
ScriptBlock scriptBlock,
ProtectedEventLogging logSetting)
Expand Down
10 changes: 5 additions & 5 deletions test/powershell/Host/Logging.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,9 @@ Describe 'Basic SysLog tests on Linux' -Tag @('CI','RequireSudoOnUnix') {
$IsSupportedEnvironment = $false
}
[string] $powershell = Join-Path -Path $PSHome -ChildPath 'pwsh'
$scriptBlockCreatedRegExTemplate = @'
Creating Scriptblock text \(1 of 1\):#012{0}(#012)*ScriptBlock ID: [0-9a-z\-]*#012Path:.*
'@
$scriptBlockCreatedRegExTemplate = @"
Creating Scriptblock text \(1 of 1\):#012{0}(⏎|#012)*ScriptBlock ID: [0-9a-z\-]*#012Path:.*
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? `u{23CE} looks more clear for me.

Copy link
Copy Markdown
Member Author

@TravisEz13 TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests fail if we use the escape sequence.

Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to copy and paster the code in console and lost the char. Also the tests become unreadable. I wonder why doesn't the Unicode syntax sugar work?

Copy link
Copy Markdown
Member Author

@TravisEz13 TravisEz13 Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSazonov that's going to vary by the font you use. I worked with @rjmholt to come up with this solution. The previous solution that was working, was actually just looking for a ? which is less specific.

Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that we had Unicode chars in files and it was a headache - editors silently break them. After that we made a conclusion to keep all files in ASCII.

"@

}
}
Expand Down Expand Up @@ -211,7 +211,7 @@ $pid
$createdEvents[0].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f ".*/$testFileName")

# Verify we log that we are the script to create the scriptblock
$createdEvents[1].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f (Get-RegEx -SimpleMatch $Script.Replace([System.Environment]::NewLine,'#012')))
$createdEvents[1].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f (Get-RegEx -SimpleMatch $Script.Replace([System.Environment]::NewLine,"⏎")))

# Verify we log that we are excuting the created scriptblock
$createdEvents[2].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f "Write\-Verbose 'testheader123' ;Write\-verbose 'after'")
Expand Down Expand Up @@ -240,7 +240,7 @@ $pid
$createdEvents[0].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f ".*/$testFileName")

# Verify we log that we are the script to create the scriptblock
$createdEvents[1].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f (Get-RegEx -SimpleMatch $Script.Replace([System.Environment]::NewLine,'#012')))
$createdEvents[1].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f (Get-RegEx -SimpleMatch $Script.Replace([System.Environment]::NewLine,"⏎")))

# Verify we log that we are excuting the created scriptblock
$createdEvents[2].Message | Should -Match ($scriptBlockCreatedRegExTemplate -f "Write\-Verbose 'testheader123␀' ;Write\-verbose 'after'")
Expand Down