Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix access violation when using Express user instances #2101

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

emidah
Copy link
Contributor

@emidah emidah commented Jul 24, 2023

Fixes #1944

When SQL Server Express user instances are used, this constructor is called from a special code path at SQLConnectionFactory.cs#L98.

Because the _hostNameInCertificate option is not set in the constructor, the cloned connection at line 99 fails with an access violation somewhere on the native side.

Releases after PR #1608 (which initially added this option) are affected - Express user instances won't work at all in any of them.

This occurs due to hostNameInCertificate being set to null. This is called from SQLConnectionFactory.cs#98
@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 24, 2023

A test would be nice..

@emidah
Copy link
Contributor Author

emidah commented Jul 24, 2023

A test would be nice..

I would like to add some, but it seems difficult to do comprehensively. The local instances feature is currently completely untested, and it seems the closest public interface up the call stack is basically SqlConnection.Open()

If you think it's enough, I can add a test amounting to

Assert.Throws<SqlException>(() => connection.Open()); // hey, at least it doesn't throw an access violation anymore!

but that would not really add any guarantee of the feature working in the future - just that this one very specific issue doesn't occur.

As for integration tests, I am out of my depth there as to how they work in this project but I don't think any of the test pipelines run against Express, which is the only thing that supports this feature

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 24, 2023

Yeah, anything is better than no test at all, IMHO.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.11 🎉

Comparison is base (a709006) 69.97% compared to head (a644dce) 70.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2101      +/-   ##
==========================================
+ Coverage   69.97%   70.08%   +0.11%     
==========================================
  Files         306      306              
  Lines       61975    61976       +1     
==========================================
+ Hits        43368    43437      +69     
+ Misses      18607    18539      -68     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.59% <100.00%> (+0.17%) ⬆️
netfx 68.37% <100.00%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
...rc/Microsoft/Data/SqlClient/SqlConnectionString.cs 81.72% <100.00%> (+9.70%) ⬆️

... and 14 files with indirect coverage changes

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

@emidah
Copy link
Contributor Author

emidah commented Jul 25, 2023

@dotnet-policy-service agree

@David-Engel David-Engel merged commit 0b8680c into dotnet:main Sep 8, 2023
132 checks passed
@DavoudEshtehari DavoudEshtehari added the 🐛 Bug! Issues that are bugs in the drivers we maintain. label Sep 29, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug! Issues that are bugs in the drivers we maintain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Couldn't use Microsoft.EntityFrameworkCore.SqlServer > 6.0.14
6 participants