Add missing tests for NamedPipes #72956

jozkee · 2022-07-27T18:54:58Z

Some tests were missing on PipeOptions.CurrentUserOnly scenarios.
This PR extend the test to account for the following (under the OS defaults, which allows readonly access on different users):

Same user asking for read access
Same user asking for read/write access
Different user asking for read access
Different user asking for read/write access

ghost · 2022-07-27T18:55:14Z

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

Issue Details

Some tests were missing on PipeOptions.CurrentUserOnly scenarios.
This PR extend the test to account for the following (under the OS defaults, which allows readonly access on different users):

Same user asking for read access
Same user asking for read/write access
Different user asking for read access
Different user asking for read/write access

Author:	Jozkee
Assignees:	Jozkee
Labels:	`area-System.IO`
Milestone:	7.0.0

danmoseley · 2022-07-27T19:02:32Z

My original comment on these tests (for Windows) so I can page back in:

These tests only run when the user is elevated. They create a new machine scope user and run the other half of the test impersonated as that user. The first test tests with and without PipeOptions.CurrentUserOnly on both ends, and verifies that the client connection is rejected in every case with UnauthorizedAccessException. This includes when both ends have PipeOptions.None, because the pipe is being created with the default ACL, which allows Read access to everyone, but not Write, and the client is being created with PipeDirection.InOut, so it passes GENERIC_WRITE to CreateFile, which then fails. The test below it verifies that client can connect to server when server is elevated (and a different user, in this case) and it succeeds because it's requesting PipeDirection.In. Because the first test fails even when PipeOptions.None is being passed on both sides, the PipeOptions.CurrentUser cases are not actually testing that PipeOptions.CurrentUser is implemented.
It seems we are missing several cases
• CurrentUserOnly on client, server, both, and neither end, with different users on other end, neither elevated, with the client requesting PipeDirection.In. Should throw UnauthorizedAccessException on client connection but pass when CurrentUserOnly is on neither end
• Same but with the same users on both ends, one or the other side elevated.

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

danmoseley · 2022-07-27T19:11:12Z

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

+                });
+            });
+
+            Thread.Sleep(1000); // give it a sec to allow above tasks to perform some work before this.


I've gotten pretty suspicious of sleeps in tests! I guess it's OK since the test won't fail on a slow machine, but the ideal pattern is to wait on some event set in the task eg after it's looped.

in fact, rather than having one task looping, you could consider kicking off N tasks all trying to connect once. That will give you some concurrency, and you can just do Task.WaitAll for them to complete, no sleep and no event.

the WaitAll would be after the await serverWait;.

As an improvement, I suggest to have the tasks all wait on an event before they try to connect, and then set that event on the line before you do client.Connect(). That will make it most likely they do their work concurrently with the succesful connect.

With serverWait = server.WaitForConnectionAsync(), we can't do anything after awaiting it, the method blocks until a client connects; the event should be set before awaiting serverWait. Please check latest commit.

Not sure I follow. How is 100 better than 1? The ideal is to not have any guesses.

I'n not very good explaining so, here's how the test would look like:

[OuterLoop] [ConditionalFact(nameof(IsAdminOnSupportedWindowsVersions))] public async Task Connection_UnderDifferentUsers_CurrentUserOnlyOnServer_InvalidClientConnectionAttempts_DoNotBlockSuccessfulClient() { string name = PipeStreamConformanceTests.GetUniquePipeName(); bool invalidClientShouldStop = false; + int invalidClientAttempts = 0; using var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly); Task serverWait = server.WaitForConnectionAsync(); Task invalidClient = Task.Run(() => { // invalid non-current user tries to connect to server. _testAccountImpersonator.RunImpersonated(() => { while (!Volatile.Read(ref invalidClientShouldStop)) { using var client = new NamedPipeClientStream(".", name, PipeDirection.In, PipeOptions.Asynchronous); Assert.Throws<UnauthorizedAccessException>(() => client.Connect()); + Volatile.Write(ref invalidClientAttempts, invalidClientAttempts + 1); } }); }); - Thread.Sleep(1000); // give it a sec to allow above tasks to perform some work before this. + while (Volatile.Read(ref invalidClientAttempts) < 100) ; // Wait until the invalid client has tried multiple times. Assert.False(serverWait.IsCompleted); // valid client tries to connect and succeeds. using var client = new NamedPipeClientStream(".", name, PipeDirection.In, PipeOptions.Asynchronous); client.Connect(); await serverWait; Volatile.Write(ref invalidClientShouldStop, true); }

This way we can at least ensure that the other two threads were running prior to the valid client thread, and it won't take 1 second, will take whatever 100 iterations take, which is probably less.
100 was just a placeholder, it can be 20 or 5 or 1.

What I am arguing is that we can have the loop with the event, and without the thread.sleep.

Yes to the loop without the sleep, but what I'm asking is why you need a counter. As long as it's gone through the loop once, you know the "interference" is in progress, right? We don't expect any new behavior to emerge after 100 times.

Yes, I agree there's no need for 100 times.

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

danmoseley · 2022-07-27T19:33:18Z

Should we also have Windows tests where there are different users on each end, and neither is elevated? this would probably be easiest to do by creating two different users.

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Unix.cs

eerhardt · 2022-07-27T21:16:10Z

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

+                });
+            });
+
+            Thread.Sleep(1000); // give it a sec to allow above tasks to perform some work before this.


jozkee · 2022-07-28T00:28:38Z

Should we also have Windows tests where there are different users on each end, and neither is elevated? this would probably be easiest to do by creating two different users.

It is doable, the _testAccountImpersonator is populated using xUnit's IClassFixture<TestAccountImpersonator>; I tried adding an extra parameter of the same type to the test class ctor expecting to get a new instance (i.e: a new user) but xUnit passes the same instance twice, so this probably requires switching IClassFixture<TestAccountImpersonator> to a IClassFixture<TestAccountImpersonator**Set**>, containing the two impersonators.

My point is that it may be too cumbersome for this PR. I also don't think there's too much value on it.

jozkee · 2022-07-28T00:36:27Z

Actually, we can create the second TestAccountImpersonator inside the test method, that should be easier. We just need to make sure that the theory data does not create a new one everytime.

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Unix.cs

stephentoub · 2022-08-01T02:23:41Z

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

+            });
+
+            using var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly);
+            @event.Set();


There's a very high liklihood that both Connects will be initiated before the server begins waiting. There's also a high liklihood that the valid client will connect before the invalid one does.

In the original test I'd proposed, the invalid client was looping, trying to connect over and over, and only once it started that loop would the valid client try to connect, the goal of the test being to ensure the invalid client couldn't starve out the valid client. That's no longer happening here, so I'm not sure what this test is actually testing. With the invalid client making a single connection attempt, eventually that attempt will end and the valid client can connect. And if that were broken, that would be caught by just a simple test with no client/client concurrency that simply tried to connect from an invalid client and then sequentially tried to connect from a valid client.

I can bring the original test back and keep this version as well.
fwiw: this was the old version that issued invalid connects in a loop:
c94d060#diff-23b3a1e5a562e7712c4adff12019bbc8485dd0566627dc77808efa97efee2b30R134

I agree that this test is not worth keeping, because it will so rarely do anything useful. If you can tweak the test you brought back below, I think it will do it correctly, and you can delete this.

adamsitnik

It's almost ready, thank you for improving the tests @jozkee !

adamsitnik · 2022-10-10T08:00:15Z

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Unix.cs

        {
+            bool isRoot = AdminHelpers.IsProcessElevated();


This test is executed only for "super user":

runtime/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Unix.cs

Lines 26 to 28 in c1fab9c

private static bool IsRemoteExecutorSupportedAndOnUnixAndSuperUser => RemoteExecutor.IsSupported && PlatformDetection.IsUnixAndSuperUser;

[ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]

runtime/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs

Line 58 in d196d85

public static bool IsSuperUser => IsBrowser || IsWindows ? false : libc.geteuid() == 0;

and the helper that checks whether the process is elevated performs same check:

runtime/src/libraries/Common/tests/TestUtilities/System/AdminHelpers.cs

Lines 35 to 40 in d196d85

public static unsafe bool IsProcessElevated()

{

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

{

uint userId = Interop.Sys.GetEUid();

return(userId == 0);

It seems that this check is redundant and adding PipeOptions.CurrentUserOnly test cases has no effect?

IIRC, the reason I did this was to complete the test matrix and have explicit behaviors of all combinations. But this could be represented by just having the theorydata where clientPipeOptions = CurrentUserOnly commented-out.

@jozkee IMO it would be better to remove the test cases that are always skipped and just add a comment to the test explaining why given scenarios are not covered.

Actually, the right thing to do is remove IsRemoteExecutorSupportedAndOnUnixAndSuperUser that way we can run all the inline data when dotnet is not running as sudo/root and skip the clientPipeOptions == PipeOptions.CurrentUserOnly when it is.

This worked as I just described on my local wsl, I hope it works in CI as well.

@jozkee but we need to keep the IsRemoteExecutorSupported condition as the test is using it.

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs

jozkee · 2022-10-13T15:09:43Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-10-13T15:09:53Z

Azure Pipelines successfully started running 1 pipeline(s).

jozkee · 2022-10-18T21:45:03Z

CI errors are #76755

adamsitnik

LGTM, thank you @jozkee !

Add missing tests for NamedPipes

c94d060

jozkee added the area-System.IO label Jul 27, 2022

jozkee added this to the 7.0.0 milestone Jul 27, 2022

jozkee requested review from stephentoub, adamsitnik, danmoseley and eerhardt July 27, 2022 18:54

jozkee self-assigned this Jul 27, 2022

danmoseley reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs Show resolved Hide resolved

danmoseley reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs Outdated Show resolved Hide resolved

danmoseley reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs Show resolved Hide resolved

danmoseley reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jul 27, 2022

View reviewed changes

Address feedback

832f8ec

danmoseley reviewed Jul 28, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Windows.cs Outdated Show resolved Hide resolved

Move event wait to happen one line before Connect.

3b82836

jozkee requested review from eerhardt and danmoseley July 28, 2022 15:04

stephentoub reviewed Aug 1, 2022

View reviewed changes

src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Aug 1, 2022

View reviewed changes

jozkee added 3 commits August 5, 2022 16:37

Bring back previous version of test

c394782

Reword SkipTestException message

6c23b54

Move NamedPipeTest.CurrentUserOnly.cs to compile for all platforms

5b7e9a7

jozkee removed this from the 7.0.0 milestone Aug 16, 2022

jozkee added this to the 8.0.0 milestone Aug 16, 2022

jozkee added 2 commits August 26, 2022 17:54

Address feedback

0118605

Merge branch 'main' of https://github.com/dotnet/runtime into pipe_tests

d57a8df

adamsitnik reviewed Oct 10, 2022

View reviewed changes

jozkee added 2 commits October 12, 2022 16:26

Remove IsRemoteExecutorSupportedAndOnUnixAndSuperUser

699f819

Keep RemoteExecutor.IsSupported

1f1aabe

build-analysis bot mentioned this pull request Oct 13, 2022

System.Data.OleDb.Test - timeout and hangs #74488

Open

jozkee requested review from adamsitnik and stephentoub October 18, 2022 21:45

adamsitnik approved these changes Oct 19, 2022

View reviewed changes

jozkee merged commit 88aea31 into dotnet:main Oct 20, 2022

jozkee deleted the pipe_tests branch October 20, 2022 04:35

ghost locked as resolved and limited conversation to collaborators Nov 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing tests for NamedPipes #72956

Add missing tests for NamedPipes #72956

jozkee commented Jul 27, 2022

ghost commented Jul 27, 2022

danmoseley commented Jul 27, 2022

danmoseley Jul 27, 2022

danmoseley Jul 27, 2022

danmoseley Jul 27, 2022

eerhardt Jul 27, 2022

jozkee Jul 28, 2022

danmoseley Aug 16, 2022

jozkee Aug 16, 2022 •

edited

Loading

jozkee Aug 16, 2022

danmoseley Aug 16, 2022

jozkee Aug 16, 2022

danmoseley commented Jul 27, 2022

eerhardt Jul 27, 2022

jozkee commented Jul 28, 2022 •

edited

Loading

jozkee commented Jul 28, 2022 •

edited

Loading

stephentoub Aug 1, 2022

jozkee Aug 5, 2022 •

edited

Loading

danmoseley Aug 15, 2022

adamsitnik left a comment

adamsitnik Oct 10, 2022

jozkee Oct 10, 2022

adamsitnik Oct 11, 2022

jozkee Oct 12, 2022

adamsitnik Oct 13, 2022

jozkee commented Oct 13, 2022

azure-pipelines bot commented Oct 13, 2022

jozkee commented Oct 18, 2022

adamsitnik left a comment

	private static bool IsRemoteExecutorSupportedAndOnUnixAndSuperUser => RemoteExecutor.IsSupported && PlatformDetection.IsUnixAndSuperUser;

	[ConditionalTheory(nameof(IsRemoteExecutorSupportedAndOnUnixAndSuperUser))]

	public static unsafe bool IsProcessElevated()
	{
	if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
	{
	uint userId = Interop.Sys.GetEUid();
	return(userId == 0);

Add missing tests for NamedPipes #72956

Add missing tests for NamedPipes #72956

Conversation

jozkee commented Jul 27, 2022

ghost commented Jul 27, 2022

danmoseley commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jul 27, 2022

Choose a reason for hiding this comment

jozkee commented Jul 28, 2022 • edited Loading

jozkee commented Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

jozkee Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee commented Oct 13, 2022

azure-pipelines bot commented Oct 13, 2022

jozkee commented Oct 18, 2022

adamsitnik left a comment

Choose a reason for hiding this comment

jozkee Aug 16, 2022 •

edited

Loading

jozkee commented Jul 28, 2022 •

edited

Loading

jozkee commented Jul 28, 2022 •

edited

Loading

jozkee Aug 5, 2022 •

edited

Loading