Runtime: Possible copy-paste mistake in SqlConnection introduced by AAD Authentication work

Created on 21 Dec 2018 · 14Comments · Source: dotnet/runtime

The work done for dotnet/runtime#19366 may have introduced a copy-paste mistake in SqlConnection. The change is present in the already released 2.2 (see dotnet/corefx#31039) and the planned release 3.0 (see dotnet/corefx#30342).

The possible mistake was noticed by @steji113 in the 2.2 PR. It appears that the change at https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1363 was copied to https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1406 and https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1444

Was this change meant to be done, or is it a mistake?

area-System.Data.SqlClient

Source

stijnherreman

👍3

Most helpful comment

I am going to close this since the fix is now both in master and Release/2.2 branch. Thank you @stijnherreman for finding this issue 👍

AfsanehR on 22 Feb 2019

🎉1 👍1

All 14 comments

From what I can see I think this means there will be a lot of connection pool misses. Is that what you're seeing? lots of new connection throughput?

Wraith2 on 21 Dec 2018

I haven't switched that part of our code to CoreFX yet, so I don't know the impact. I created this issue to ensure it's being tracked, seeing as there's a lack of response in the PR itself.

stijnherreman on 21 Dec 2018

Tagging @AfsanehR @keeratsingh since as you point out, this already got into 2.2 bits so we should move quickly if it's going to cause a problem.

danmosemsft on 21 Dec 2018

Replied https://github.com/dotnet/corefx/pull/31039#discussion_r243665371 based on the code in NetFx at https://github.com/Microsoft/referencesource/blob/master/System.Data/System/Data/SqlClient/SqlConnection.cs

Impact of this bug based on my understanding of the area:

The wrong connection pool will be cleared on a Change password using SecureString. As a result there will be impact on the functionality of the connections being used from the pool being cleared. (In case the wrong connection pool exists)
The right connection pool will not be cleared after a password change. That would lead to unused connections in the pool which will be cleared out eventually (3-5 minutes) to bring the connection pool count, down to Min Pool Size in case the connections from the pool is unused.

We should consider this for servicing.

saurabh500 on 21 Dec 2018

Maybe it's fortunate then that few people have consumed this change yet, due to our bug: https://github.com/dotnet/corefx/pull/33697

danmosemsft on 21 Dec 2018

These don't cause a problem because they're all in dead code. All three ChangePassword overloads have no callers other than themselves. I've just commented them out and compiled the library just fine. The three real call sites through Connection_Set all pass both credential and token references are expected. So it looks buggy but isn't.

Wraith2 on 21 Dec 2018

@Wraith2 It is buggy. You dont see any callers because these are public APIs

saurabh500 on 21 Dec 2018

Which also means that we don't have tests for these APIs
I synced up with @AfsanehR and she is taking care of this issue and boosting up the tests for this area.
Edit: I found some tests for successful password change, but nothing that checks the clearing of connection pool

saurabh500 on 21 Dec 2018

Oh, that's a weird api. a static method that changes a connection string parameter password and affects the pool at the same time.

Wraith2 on 21 Dec 2018

@steji113 and @stijnherreman Thanks for pointing this out.
Also it is good that you opened a new issue. A lot of times comments on Closed PRs and Closed Issue go un-monitored.

saurabh500 on 21 Dec 2018

@saurabh500 absolutely, I should have thought to just open an issue initially. I also thought it seemed like it indicated a testing gap, as well. Thanks for the evaluation!