#98441 - Ignore invalid handle/access when setting console cp #98641

nick-beer · 2024-02-18T22:41:09Z

There are situations where setting the console code page is expected to fail. For instance, if the application was started with DETACHED_PROCESS, the attempting to set the code page will fail with ERROR_INVALID_HANDLE. In these cases, ignore the returned error.

Fixes #98441

There are situations where setting the console code page is expected to fail. For instance, if the application was started with DETACHED_PROCESS, the attempting to set the code page will fail with ERROR_INVALID_HANDLE. In these cases, ignore the returned error. Fix dotnet#98441

ghost · 2024-02-18T22:41:18Z

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

There are situations where setting the console code page is expected to fail. For instance, if the application was started with DETACHED_PROCESS, the attempting to set the code page will fail with ERROR_INVALID_HANDLE. In these cases, ignore the returned error.

Fixes #98441

Author:	nick-beer
Assignees:	-
Labels:	`area-System.Console`
Milestone:	-

nick-beer · 2024-02-18T22:43:28Z

@dotnet-policy-service agree company="National Instruments"

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

jkotas · 2024-02-19T05:07:17Z

Is it possible to add a test?

coding conventions: use explicit Type rather than var Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/libraries/System.Console/src/System/ConsolePal.Windows.cs

This helps ensure GetLastError is called immediately after the failing win32 function.

jhudsoncedaron · 2024-02-20T14:56:28Z

@jkotas : Yes, but only if the test is allowed to start a new project. I put a short crash sequence in the work item thread.

nick-beer · 2024-02-20T15:12:55Z

Yeah - I should have updated this thread. I'm currently working on a couple tests for this issue. It took a little bit of time to aquaint myself with the test infrastrucure. It looks to me like the existing RemoteExecutor infrastructure only uses Process.Start to run the external process, so it's not easy to start a detached process. I now have tests that use FreeConsole to simulate a similar scenario, but I'm getting unexpected results. Trying to debug the failures is resulting in Visual Studio crashes, so it's taking a little longer than I'd hoped.

Do we agree that tests with FreeConsole to simulate a detached console are valuable?

Edit: to be clear, the FreeConsole call happens within the context of a RemoteExecutor test - so it should not impact other tests.

jkotas · 2024-02-20T16:59:21Z

Either FreeConsole or launching a process with DETACHED_PROCESS flags should work for a test.

If you would like to go with DETACHED_PROCESS, ProcessorCount_Windows_RespectsJobCpuRateAndConfigurationSetting has an example for how to launch child test process with custom Windows-specific settings.

nick-beer · 2024-02-20T19:43:43Z

Tests added. I went with FreeConsole. I like the way the test you linked to re ran the process with custom flags - pretty cool.

jhudsoncedaron · 2024-02-20T19:52:32Z

src/libraries/System.Console/tests/ConsoleEncoding.Windows.cs

+            // The internal state of Console should have updated, despite the failure to change the console's input encoding
+            Assert.Equal(encoding, Console.InputEncoding);
+            // Operations on the console are no longer valid - GetConsoleCP fails.
+            Assert.Equal(0u, GetConsoleCP());


I would advise testing that Console.WriteLine() still works here. The caller would need to use StandardOutputRedirected = true for the test case.

jkotas

LGTM! Thank you

jkotas · 2024-02-20T22:00:16Z

The test is failing on Windows Nano Server:

Unhandled exception. Xunit.Sdk.NotEqualException: Assert.NotEqual() Failure: Values are equal
Expected: Not 65001
Actual:       65001
   at Xunit.Assert.NotEqual[T](T expected, T actual, IEqualityComparer`1 comparer) in /_/src/Microsoft.DotNet.XUnitAssert/src/EqualityAsserts.cs:line 696
   at Xunit.Assert.NotEqual[T](T expected, T actual) in /_/src/Microsoft.DotNet.XUnitAssert/src/EqualityAsserts.cs:line 610
   at ConsoleEncoding.<>c.<InputEncoding_SetEncodingWhenDetached_ErrorIsSilentlyIgnored>b__12_0() in /_/src/libraries/System.Console/tests/ConsoleEncoding.Windows.cs:line 48
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs:line 36
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs:line 57
--- End of stack trace from previous location ---
   at Microsoft.DotNet.RemoteExecutor.Program.Main(String[] args) in /_/src/Microsoft.DotNet.RemoteExecutor/src/Program.cs:line 97
    ConsoleEncoding.InputEncoding_SetEncodingWhenDetached_ErrorIsSilentlyIgnored [FAIL]
      Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception.
      Stack Trace:
        
        Child exception:
          Xunit.Sdk.NotEqualException: Assert.NotEqual() Failure: Values are equal
        Expected: Not 65001
        Actual:       65001
        /_/src/libraries/System.Console/tests/ConsoleEncoding.Windows.cs(48,0): at ConsoleEncoding.<>c.<InputEncoding_SetEncodingWhenDetached_ErrorIsSilentlyIgnored>b__12_0()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
        /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(57,0): at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
        
        Child process:
          System.Console.Tests, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 ConsoleEncoding+<>c Void <InputEncoding_SetEncodingWhenDetached_ErrorIsSilentlyIgnored>b__12_0()

jhudsoncedaron · 2024-02-20T22:04:20Z

I see why the test is failing. The code asserts that the current output codepage is not the one it's going to try to test by changing it to. So the test knows it doesn't work and bails.

Looks like the test harness needs to handle that case. Clearly the implementation needs to be if already 65001 use some other codepage instead of 65001. I'm not sure which code page it should change to though. Code page 437 would be a sane choice everywhere else but I'm not sure about nano.

jkotas · 2024-02-20T22:30:42Z

Any one of the built-in .NET encodings should work:

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs

Lines 208 to 215 in 1b1d26a

    
           case CodePageDefault: return Default;            // 0 
        
           case CodePageUnicode: return Unicode;            // 1200 
        
           case CodePageBigEndian: return BigEndianUnicode; // 1201 
        
           case CodePageUTF32: return UTF32;                // 12000 
        
           case CodePageUTF32BE: return BigEndianUTF32;     // 12001 
        
           case CodePageUTF8: return UTF8;                  // 65001 
        
           case CodePageASCII: return ASCII;                // 20127 
        
           case ISO_8859_1: return Latin1;                  // 28591

nick-beer · 2024-02-21T00:37:05Z

Updated to ensure we set an encoding that's different from whatever is existing. Also updated two existing tests that were not performing such a check.

src/libraries/System.Console/tests/ConsoleEncoding.Windows.cs

…rent encoding

adamsitnik

LGTM, thank you @nick-beer for providing not just the fix but also great tests!

dotnet-issue-labeler bot added the area-System.Console label Feb 18, 2024

nick-beer mentioned this pull request Feb 18, 2024

dotnet CLI commands no longer work in .NET 7/8 with detached console #98441

Closed

build-analysis bot mentioned this pull request Feb 19, 2024

[browser][MT] Assert failed: Cannot find Promise for JSHandle -2 #98406

Closed

jkotas reviewed Feb 19, 2024

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.Windows.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Console/src/System/ConsolePal.Windows.cs

2bb4275

coding conventions: use explicit Type rather than var Co-authored-by: Jan Kotas <jkotas@microsoft.com>

danmoseley reviewed Feb 19, 2024

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.Windows.cs Outdated Show resolved Hide resolved

Pass last error into error handling function

f735bbc

This helps ensure GetLastError is called immediately after the failing win32 function.

danmoseley approved these changes Feb 19, 2024

View reviewed changes

Add tests validating new behavior

afcf0a9

jhudsoncedaron reviewed Feb 20, 2024

View reviewed changes

jkotas approved these changes Feb 20, 2024

View reviewed changes

jkotas requested a review from adamsitnik February 20, 2024 21:56

build-analysis bot mentioned this pull request Feb 20, 2024

[wasm] System.Text.Json.Tests running out of memory #98578

Closed

build-analysis bot mentioned this pull request Feb 20, 2024

System.Text.Json failing some large file tests #59678

Closed

Ensure we set an encoding that is different than the existing encoding

f01d18b

danmoseley reviewed Feb 21, 2024

View reviewed changes

src/libraries/System.Console/tests/ConsoleEncoding.Windows.cs Outdated Show resolved Hide resolved

Revert 'default encoding' tests to use default even if that's the cur…

3f75482

…rent encoding

This was referenced Feb 21, 2024

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

Segmentation fault in System.Text.RegularExpressions.Tests #93206

Closed

adamsitnik approved these changes Feb 22, 2024

View reviewed changes

adamsitnik merged commit 831ca1e into dotnet:main Feb 22, 2024
104 of 111 checks passed

adamsitnik added this to the 9.0.0 milestone Feb 22, 2024

github-actions bot locked and limited conversation to collaborators Mar 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#98441 - Ignore invalid handle/access when setting console cp #98641

#98441 - Ignore invalid handle/access when setting console cp #98641

nick-beer commented Feb 18, 2024

ghost commented Feb 18, 2024

nick-beer commented Feb 18, 2024

jkotas commented Feb 19, 2024

jhudsoncedaron commented Feb 20, 2024

nick-beer commented Feb 20, 2024 •

edited

Loading

jkotas commented Feb 20, 2024

nick-beer commented Feb 20, 2024

jhudsoncedaron Feb 20, 2024

jkotas left a comment

jkotas commented Feb 20, 2024

jhudsoncedaron commented Feb 20, 2024

jkotas commented Feb 20, 2024

nick-beer commented Feb 21, 2024

adamsitnik left a comment

#98441 - Ignore invalid handle/access when setting console cp #98641

#98441 - Ignore invalid handle/access when setting console cp #98641

Conversation

nick-beer commented Feb 18, 2024

ghost commented Feb 18, 2024

nick-beer commented Feb 18, 2024

jkotas commented Feb 19, 2024

jhudsoncedaron commented Feb 20, 2024

nick-beer commented Feb 20, 2024 • edited Loading

jkotas commented Feb 20, 2024

nick-beer commented Feb 20, 2024

jhudsoncedaron Feb 20, 2024

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Feb 20, 2024

jhudsoncedaron commented Feb 20, 2024

jkotas commented Feb 20, 2024

nick-beer commented Feb 21, 2024

adamsitnik left a comment

Choose a reason for hiding this comment

nick-beer commented Feb 20, 2024 •

edited

Loading