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 Socket array length to avoid possible out of bounds errors #1031

Merged
merged 2 commits into from Apr 14, 2021

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Apr 13, 2021

Following discussion from #1016..
This change fixes possible out of bounds errors and timeout cancellation issues.
Further improvements to be reviewed in #1029.

@Wraith2
Copy link
Contributor

Wraith2 commented Apr 14, 2021

Is GetHostAddresses required to return a non-null value even if it can't resolve? If not you need to add a null check and probably early exit.

Also, while you're there... any chance of hoisting that AddressFamily array out as a static readonly so we can avoid allocating and discarding it every single time we make a connection?

@cheenamalhotra
Copy link
Member Author

cheenamalhotra commented Apr 14, 2021

@Wraith2 regarding AddressFamily suggestion, there's another PR bringing in preferred family support: https://github.com/dotnet/SqlClient/pull/1015/files#diff-11cfbefb2cd1b54e3dfda7a8eeb5d1b447086f5dbb41204151929886c29ae99fR345 so it won't be a static.

I'll see improvement opportunity to reduce allocation with that PR.

@cheenamalhotra cheenamalhotra added this to In Progress in SqlClient v3.0 via automation Apr 14, 2021
@cheenamalhotra cheenamalhotra added this to the 3.0.0-preview2 milestone Apr 14, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Apr 14, 2021

Ok, LGTM then.

SqlClient v3.0 automation moved this from In Progress to Reviewer Approved Apr 14, 2021
@cheenamalhotra cheenamalhotra merged commit 2e773ae into dotnet:main Apr 14, 2021
SqlClient v3.0 automation moved this from Reviewer Approved to Done Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants