Skip to content

[6.1] Fix rare multi-packet string corruption #3513

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

Merged
merged 1 commit into from
Jul 25, 2025

Conversation

cheenamalhotra
Copy link
Member

Ports #3505 to fix #3504 by @Wraith2

* add test and simplify code to fix problems

* review feedback

* review feedback

* review feedback
@cheenamalhotra cheenamalhotra added this to the 6.1.0 milestone Jul 24, 2025
@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 17:01
@cheenamalhotra cheenamalhotra requested a review from a team as a code owner July 24, 2025 17:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a rare issue with multi-packet string corruption by correcting the logic that determines when to store and retrieve snapshot buffers during TDS protocol operations. The fix addresses cases where strings could become corrupted when they span multiple network packets.

Key changes:

  • Replaces complex condition (isStarting || isContinuing) with simpler canContinue check
  • Adds debug assertions to ensure buffer consistency when continuing operations
  • Removes unnecessary nested conditional blocks for improved code clarity

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
TdsParserStateObject.cs Updates buffer management logic in byte array and string reading methods
netfx/TdsParser.cs Fixes Unicode character reading logic for .NET Framework implementation
netcore/TdsParser.cs Fixes Unicode character reading logic for .NET Core implementation

@paulmedynski paulmedynski changed the title [6.1] Fix rare multi-packet string corruption (#3505) [6.1] Fix rare multi-packet string corruption Jul 24, 2025
@Wraith2
Copy link
Contributor

Wraith2 commented Jul 24, 2025

Any chance of a preview 3 that contains this?

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 24, 2025

@Wraith2 Looks like 6.1.0 is planned for tomorrow.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 24, 2025

Oh. Well that's interesting. I thought my weekend would be free but now it'll be filled with worrying that an unknown bug might be making it out to the general public and breaking all sorts of things.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 24, 2025

@Wraith2 preview 2 has been out for a month, is that not sufficient?

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 24, 2025

Not when we found two fairly important bugs in it. No. But it's not my choice.

@cheenamalhotra
Copy link
Member Author

cheenamalhotra commented Jul 24, 2025

Due to schedule constraints another preview is not planned right now. You can build, and test the NuGet package off the main branch if needed and propose a fix for any other issue for the next Hotfix.

Since this change mainly affects the large string read usecases, we've considered bringing the fix in.

Do you think leaving the edge case identified in #3504 open in 6.1 is safer than fixing it with this PR?

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 24, 2025

Absolutely not, better with the fix.

Copy link

codecov bot commented Jul 25, 2025

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes missing coverage. Please review.

Project coverage is 66.06%. Comparing base (6a37506) to head (08b8db9).
Report is 3 commits behind head on release/6.1.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 4 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/6.1    #3513      +/-   ##
===============================================
+ Coverage        66.04%   66.06%   +0.02%     
===============================================
  Files              281      281              
  Lines            62416    62401      -15     
===============================================
+ Hits             41221    41225       +4     
+ Misses           21195    21176      -19     
Flag Coverage Δ
addons 90.82% <ø> (-0.23%) ⬇️
netcore 68.95% <57.14%> (+<0.01%) ⬆️
netfx 68.15% <50.00%> (+0.12%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saurabh500 saurabh500 self-requested a review July 25, 2025 13:56
@paulmedynski paulmedynski merged commit 1af7327 into release/6.1 Jul 25, 2025
129 checks passed
@paulmedynski paulmedynski deleted the dev/cheena/backport-3505 branch July 25, 2025 13:57
samsharma2700 added a commit that referenced this pull request Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants