Fix SafeLsaMemoryHandle.InvalidHandle

[MarcoRossignoli]

fixes https://github.com/dotnet/corefx/issues/32636

/cc @bartonjs @GrabYourPitchforks @jkotas

[danmoseley]

Could you let it always run, and check that it either does not throw or throws IdentityNotMappedException or Win32EXception? That would prove it does not throw ObjectDisposedException any more and occasionally it would likely get run on a machine in such a state. Eg. we often work on laptops with machines that are joined to the domain and do not have connectivity.

Or the test could somehow check for the state (in domain but offline)

[danmoseley]

Somewhere someone has a domain or user named foobar. Best to use something like Guid.NewGuid().ToString("N")

[danmoseley]

Please sort. This helps avoid merge conflicts. It's how VS adds entries.

[MarcoRossignoli]

@danmosemsft @jkotas i don't know if this makes sense, maybe is extras.

[MarcoRossignoli]

@dotnet-bot test Linux arm64 Release Build please

[bartonjs]

I wonder if converting SafeLsaMemoryHandle / SafeLsaPolicyHandle / SafeLsaReturnBufferHandle to use the SafeHandleCache, like SafeX509ChainHandle does, is better.

corefx/src/System.Security.Cryptography.X509Certificates/src/Microsoft/Win32/SafeHandles/SafeX509ChainHandle.cs

Lines 19 to 35 in a10890f

	internal static SafeX509ChainHandle InvalidHandle
	{
	get { return SafeHandleCache<SafeX509ChainHandle>.GetInvalidHandle(() => new SafeX509ChainHandle()); }
	}
	protected override bool ReleaseHandle()
	{
	return ChainPal.ReleaseSafeX509ChainHandle(handle);
	}
	protected override void Dispose(bool disposing)
	{
	if (!SafeHandleCache<SafeX509ChainHandle>.IsCachedInvalidHandle(this))
	{
	base.Dispose(disposing);
	}
	}

[bartonjs]

As I get more caught up on email I see that it looks like my comment was already given and disrecommended, but it wasn't visible here.

Personally, I prefer the SafeHandleCache because it keeps writing new code easy (no "am I supposed to not call Dispose() here if IsInvalid?"). The interlocked calls are more expensive, but they are keeping the expense in the calls, not on the finalizer thread/finalizer queue/GC sweep.

Though, restructuring code to not allocate the invalid handle redundantly is also a viable path, and then get rid of the static property altogether.

[MarcoRossignoli]

Though, restructuring code to not allocate the invalid handle redundantly is also a viable path, and then get rid of the static property altogether.

Sorry @bartonjs i don't understand what are you proposing...now SafeLsaMemoryHandle is cached by private _invalidHandle , it's not clear how we get rid of the static property altogether.
The only other thing i could change is other two object SafeLsaPolicyHandle and SafeLsaReturnBufferHandle that allocate of every static property call.
Maybe i'm missing something.

[bartonjs]

it's not clear how we get rid of the static property altogether

Instead of SafeLsaMemoryHandle ReferencedDomainsPtr = SafeLsaMemoryHandle.InvalidHandle;, start it off as null. Change the P/Invoke that rewrites it to out instead of ref, and change the finally to ReferencedDomainsPtr?.Dispose(); Do the same for the other ReferencedDomainsPtr, and SidsPtr and NamesPtr. Now nothing uses the static property, so delete it.

[MarcoRossignoli]

@bartonjs PTAL!

[jkotas]

Thanks