Remove linq

[Wraith2]

helps with #1942

Removes all uses of linq operations in the main binary and addons allowing linq to be trimmed out if no other libraries are using it
I suggest reviewing 1 commit at a time. Some of the enclave stuff is a bit messy and this cleans it up as well as removing the linq.

/cc @kant2002

[Wraith2]

@David-Engel

Is there someone knowledgeable about enclave stuff in the codebase that could help here. I've found something odd that concerns me.

There's a piece of code that was using data.Skip(offset).Take(count) and i replaced it with a function call that does the same thing without using linq. My replacement differs in behaviour by requiring that you be able to copy exactly count bytes where the linq Take function will only copy up-to count bytes, if there aren't enough it won't fail.

This caused failures in this code:

int secureSessionBufferSize = Convert.ToInt32(secureSessionInfoResponseSize) - sizeof(uint);
byte[] secureSessionBuffer = attestationInfo.Skip(offset).Take(secureSessionBufferSize).ToArray();
EnclaveDHInfo = new EnclaveDiffieHellmanInfo(secureSessionBuffer);
offset += Convert.ToInt32(EnclaveDHInfo.Size);

if (secureSessionBufferSize != secureSessionBuffer.Length)
{
	throw new InvalidOperationException($"enclave data mismatch, expected {secureSessionBufferSize} got {EnclaveDHInfo.Size}");
}

Where we calculate how many bytes of data we expect to get and then try to take it and end up 4 bytes short. Either the calculation of how many bytes we should expect to get is wrong or the something else is going on.

[codecov]

Codecov Report

Patch coverage: 71.98% and project coverage change: -0.10 ⚠️

Comparison is base (1bcdb23) 70.54% compared to head (b3ddcb0) 70.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
- Coverage   70.54%   70.44%   -0.10%     
==========================================
  Files         306      306              
  Lines       61563    61657      +94     
==========================================
+ Hits        43429    43436       +7     
- Misses      18134    18221      +87     

Flag	Coverage Δ
addons	92.88% <100.00%> (+0.49%)	⬆️
netcore	73.21% <65.62%> (-0.09%)	⬇️
netfx	69.06% <72.34%> (-0.11%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../Microsoft/Data/SqlClient/SNI/SNIPhysicalHandle.cs	100.00% <ø> (ø)
...fx/src/Microsoft/Data/Common/DBConnectionString.cs	12.96% <ø> (ø)
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	65.13% <ø> (-0.21%)	⬇️
...c/Microsoft/Data/SqlClient/SqlSymmetricKeyCache.cs	100.00% <ø> (ø)
...ent/SqlAuthenticationProviderManager.NetCoreApp.cs	30.88% <14.28%> (-1.48%)	⬇️
...Data/SqlClient/SqlAuthenticationProviderManager.cs	49.03% <20.00%> (-0.97%)	⬇️
...Data/SqlClient/SqlAuthenticationProviderManager.cs	61.66% <22.22%> (+0.64%)	⬆️
...nt/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs	38.13% <35.00%> (-0.12%)	⬇️
...lClient/Reliability/SqlConfigurableRetryFactory.cs	93.54% <42.85%> (-6.46%)	⬇️
.../SqlClient/VirtualSecureModeEnclaveProviderBase.cs	81.54% <43.75%> (-4.17%)	⬇️
... and 15 more

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kant2002]

Maybe revert this condition and have fast exit? Code above looks a bit unnatural with this condition

ipv4Addresses = Array.Empty<IPAddress>();
ipv6Addresses = Array.Empty<IPAddress>();

[Wraith2]

It's like this because they're out parameters. I suppose it's a personal style choice but I always assign the default values for out parameters at the start of the method so that anywhere inside the body I can easily return without worrying about whether they're assigned or not.

[roji]

@Wraith2 what kind of size reduction are you seeing with this?

[Wraith2]

I haven't measured. I also think it's unlikely to help if sqlclient is used through EFCore because there's no way that linq is getting trimmed from EFCore. I just saw the list that @kant2002 posted in the aot thread and thought I'd see what I could so with one of the easier items.

[roji]

I also think it's unlikely to help if sqlclient is used through EFCore because there's no way that linq is getting trimmed from EFCore.

I'm sure that's true; EF hasn't yet undergone a real trimming effort - it's possible (though not sure) that when we do that in a serious way, LINQ wouldn't be needed.

But I'd assume that various dependencies of SqlClient are also bringing in LINQ (e.g. the Azure Identity libs). Personally, for cases where removing LINQ makes the code less maintainable, I'd avoid doing that until I see it provides a tangible gain (but I also think there are at least some cases in this PR where the code doesn't become less maintainable).

[Wraith2]

Agreed. I was talking with kant on discord about this and it's possible that the azure libs will be trimmable in future if you can work out the api to do so.

If a total removal isn't appropriate certain parts of this can be backed out. There's plenty of scope for discussion and this is only a draft.

[roji]

I was talking with kant on discord about this and it's possible that the azure libs will be trimmable in future if you can work out the api to do so.

I'd propose thinking about the approach we've taken in Npgsql (#1942 (comment)): a "slim" data source builder which, when used, doesn't have size-expensive functionality on by default, unless explicitly opted into.

However, specifically regarding the Azure libs, the very fact that SqlClient has a direct references on them is probably not ideal (#1108, highest-voted issue on the repo). Here specifically, I'd suggest SqlClient expose an auth API that allows 3rd-party packages (like the Azure ones) to register themselves. This would totally decouple SqlClient from Azure, and of course naturally strip them away size-wise unless used.

[Wraith2]

As long as backwards compatibility is maintained adding new apis around the DataSource abstraction seems fine to me. That could allow both advanced AE and Enclave functionality as well as extended authentication mechanisms to be trimmable.

[David-Engel]

Is there someone knowledgeable about enclave stuff in the codebase that could help here. I've found something odd that concerns me.

There's a piece of code that was using data.Skip(offset).Take(count) and i replaced it with a function call that does the same thing without using linq. My replacement differs in behaviour by requiring that you be able to copy exactly count bytes where the linq Take function will only copy up-to count bytes, if there aren't enough it won't fail.

This caused failures in this code:

int secureSessionBufferSize = Convert.ToInt32(secureSessionInfoResponseSize) - sizeof(uint);
byte[] secureSessionBuffer = attestationInfo.Skip(offset).Take(secureSessionBufferSize).ToArray();
EnclaveDHInfo = new EnclaveDiffieHellmanInfo(secureSessionBuffer);
offset += Convert.ToInt32(EnclaveDHInfo.Size);

if (secureSessionBufferSize != secureSessionBuffer.Length)
{
	throw new InvalidOperationException($"enclave data mismatch, expected {secureSessionBufferSize} got {EnclaveDHInfo.Size}");
}

Where we calculate how many bytes of data we expect to get and then try to take it and end up 4 bytes short. Either the calculation of how many bytes we should expect to get is wrong or the something else is going on.

@Wraith2

This doesn't fail outside of your PR, right? Wouldn't the original exception throw if there was some underlying issue with the original code? I've been looking at the code changes and I'd have to debug it to get a better idea of what might be happening but it feels like something is missing. I need to find an enclave environment set up to debug against. The team has just been horribly busy with support issues for the last month or so...

[Wraith2]

It isn't a failure outside my PR. I think because while the length calculations are incorrect it isn't meaningful and the code actually does the right thing.

This is the original code:

                    uint secureSessionInfoResponseSize = BitConverter.ToUInt32(attestationInfo, offset);
                    offset += sizeof(uint);

                    SessionId = BitConverter.ToInt64(attestationInfo, offset);
                    offset += sizeof(long);

                    int secureSessionBufferSize = Convert.ToInt32(secureSessionInfoResponseSize) - sizeof(uint);
                    byte[] secureSessionBuffer = attestationInfo.Skip(offset).Take(secureSessionBufferSize).ToArray();
                    EnclaveDHInfo = new EnclaveDiffieHellmanInfo(secureSessionBuffer);
                    offset += Convert.ToInt32(EnclaveDHInfo.Size);

I think the sessionId which is read as sizeof(long) is incorrectly subtracted from the remaining bytes as sizeof(uint) but since we've actually read 8 bytes and advanced by 8 bytes we're just calculating the remaining length incorrectly. When we do Skip(offset).Take(length) the length is 4 bytes longer than we have available but the offset is correct and the lazy behaviour of Take causes it to turn out correct.

So if the code is working should I change it? I think so. I think it should be changed because the comment above the function we're looking at doesn't accurately express what the code is doing and because the code is working accidentally. As I've proven if you try to change it you risk breaking it by interpreting the code and not carefully inspecting the results in a debugger. Since the only way to debug this is complex and requires enclave functionality to be available I think code would be better.

[David-Engel]

@Wraith2

Looking at the JDBC implementation, I think the existing code is just failing to account for an additional size field after the Session ID, which is not accounted for in the secureSessionInfoResponseSize when calculating the secureSessionBufferSize.
JDBC reference

You're right. It's an error, but one that fortunately works itself out due to the Take() behavior.

int secureSessionBufferSize = Convert.ToInt32(secureSessionInfoResponseSize) - sizeof(uint);

should be

// Account for 4 byte size of enclave ECDH public key followed by 4 byte size of the DH public key signature
int secureSessionBufferSize = Convert.ToInt32(secureSessionInfoResponseSize) - sizeof(uint) - sizeof(uint);

I still haven't debugged, so let me know if that works out correctly. 😄

[Wraith2]

I revised the code but the CI is apparently off for the weekend already.

[David-Engel]

Looks good to me. This takes us in a good direction with regard to trimming and potential AoT support. Thanks for the PR!

[lcheunglci]

We just merged #1943 and it created a small merge conflict with the namespace. Let us know if you would like us to push changes to your PR branch or if you have moment to fix it. Thanks!

[Wraith2]

conflicts resolved and pushed.

[lcheunglci]

Suggested change

	SplitIPv4AndIPv6(ipv4Addresses, out ipv4Addresses, out ipv6Addresses);
	SplitIPv4AndIPv6(ipAddresses, out ipv4Addresses, out ipv6Addresses);

I'm assuming you're passing in the ipAddresses from line 192 instead of ipv4Addresses which is null on line 194; otherwise, it would always be Array.Empty<IPAddress>().

[Wraith2]

Good catch. It is concerning that the CI didn't fail on this error.

[lcheunglci]

I agree. I was wondering about that as well. It turns out since this code is only run on Linux and requires sudo/administrative privileges to get the correct environment, there's currently no test coverage for this section unfortunately.

[lcheunglci]

LGTM, Thanks @Wraith2 for your contributions!