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.

Sign up for GitHub

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

Jump to bottom

Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests #81878

Merged
merged 7 commits into from
Feb 13, 2023
Merged

Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests #81878

merged 7 commits into from
Feb 13, 2023

Conversation

jeffhandley
Copy link
Member

@jeffhandley jeffhandley commented Feb 9, 2023 •
edited
Loading

Fixes #74852

As reported in #74852, a regression was introduced in .NET 6 with #43379 that prevents SqlXml.CreateReader() from properly processing SQL Binary XML content (see MS-BINXML). The regression is caused by seeking beyond the end of the current token when validating the token, due to a simple mistake of using _end instead of _pos when _end represents the end of the buffer and _pos represents the position 1 byte beyond the end of the current token.

This regression slipped through because we didn't have any test coverage of SqlXml.CreateReader() that hit the code path of using the XmlSqlBinaryReader; all tests were text-based and using the XmlTextReader implementation. This PR introduces a corpus of test files resurrected from the .NET Framework tests (that had not been previously ported over), and each test is validated through both SQL Binary XML and Text formats.

Many thanks to @WaynePaiMS for finding the root cause of this regression and providing a repro app!

@jeffhandley jeffhandley added this to the 8.0.0 milestone Feb 9, 2023
@jeffhandley jeffhandley self-assigned this Feb 9, 2023
@ghost
Copy link

ghost commented Feb 9, 2023

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #74852

As reported in #74852, a regression was introduced that prevents SqlXml.CreateReader() from properly processing SQL Binary XML content (see MS-BINXML). The regression is caused by seeking beyond the end of the current token when validating the token, due to a simple mistake of using _end instead of _pos when _end represents the end of the buffer and _pos represents the position 1 byte beyond the end of the current token.

This regression slipped through because we didn't have any test coverage of SqlXml.CreateReader() that hit the code path of using the XmlSqlBinaryReader; all tests were text-based and using the XmlTextReader implementation. This PR introduces a corpus of test files resurrected from the .NET Framework tests (that had not been previously ported over), and each test is validated through both SQL Binary XML and Text formats.

Many thanks to @WaynePaiMS for finding the root cause of this regression and providing a repro app!

Author: jeffhandley
Assignees: jeffhandley
Labels:

area-System.Xml
Milestone: 8.0.0

vitek-karas
vitek-karas approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Based on your description the change looks good.

eiriktsarpalis
eiriktsarpalis approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks. Given the large number of old tests introduced, may I suggest running the CI pipeline a few times ahead of merging to weed out any potential flakiness?

@build-analysis build-analysis bot mentioned this pull request Feb 9, 2023
Closed
@jkotas
Copy link
Member

jkotas commented Feb 9, 2023

The .bmx binary files are going to be a problem for VMR and source build. @premun Anything you would like to see to be done about them in this PR?

@premun
Copy link
Member

premun commented Feb 9, 2023 •
edited
Loading

Thanks for tagging me. For binaries included for test purposes, we let them flow in the VMR and they will get flagged by a binary scan. We can then allow those through this file: https://github.com/dotnet/installer/blob/main/src/VirtualMonoRepo/allowed-binaries.txt

So if these fall into the same category, it would be great we added that there by opening a PR against installer.

EDIT: That being said, if these could go into runtime-assets it would be preferential

@jeffhandley jeffhandley mentioned this pull request Feb 9, 2023
Merged
@jeffhandley
Copy link
Member Author

dotnet/runtime-assets#313 was merged, moving the binary and text xml test files into that repo. I'm converting this PR to a draft until the new System.Data.Common.TestData package is available and its version can be referenced.

I've iterated on the other feedback though as well, and the tests are more robust/meaningful now. Those changes can be reviewed at your convenience, @stephentoub.

@jeffhandley jeffhandley marked this pull request as draft February 11, 2023 01:27
@jeffhandley jeffhandley force-pushed the jeffhandley/xmlsqlbinaryreader branch from b69923b to fda630a Compare February 11, 2023 01:30
@jeffhandley jeffhandley force-pushed the jeffhandley/xmlsqlbinaryreader branch from fda630a to 030ba31 Compare February 11, 2023 01:32
@jeffhandley
Copy link
Member Author

@carlossanlop Heads-up that once this is merged, I'll be pursuing backports to release/7.0 and release/6.0. I'll seek your assistance next week to see if we can get those in for the March servicing releases.

@carlossanlop
Copy link
Member

I'll be pursuing backports to release/7.0 and release/6.0

Sounds good, @jeffhandley, thanks for the heads up. If we want this in the March Servicing Release, this needs to be merged before EOD tomorrow Monday 13th, which is the Code Complete date.

@jeffhandley jeffhandley marked this pull request as ready for review February 12, 2023 22:05
This was referenced Feb 13, 2023
Merged
Merged
@jeffhandley

This comment was marked as resolved.

Sign in to view
@jeffhandley

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@github-actions

This comment was marked as resolved.

Sign in to view
@jeffhandley
Copy link
Member Author

Failures are unrelated:

@jeffhandley jeffhandley merged commit a9bf05b into dotnet:main Feb 13, 2023
@jeffhandley jeffhandley deleted the jeffhandley/xmlsqlbinaryreader branch February 13, 2023 23:04
jeffhandley added a commit to jeffhandley/runtime that referenced this pull request Feb 13, 2023
@jeffhandley
Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests (dotnet…
070ddb1 
…#81878)

* Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests

* Revert solution file changes made by VS

* Use runtime-assets test files for Text and SQL Binary XML tests

* Remove System.Data.Common.TestData package reference

* Reference the System.Data.Common.TestData package for SqlXml tests

* Add System.Common.Data.TestData to Version.Details.xml for automated dependency flow
jeffhandley added a commit to jeffhandley/runtime that referenced this pull request Feb 14, 2023
@jeffhandley
Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests (dotnet…
6fcec01 
…#81878)

* Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests

* Revert solution file changes made by VS

* Use runtime-assets test files for Text and SQL Binary XML tests

* Remove System.Data.Common.TestData package reference

* Reference the System.Data.Common.TestData package for SqlXml tests

* Add System.Common.Data.TestData to Version.Details.xml for automated dependency flow
This was referenced Feb 14, 2023
Merged
Merged
carlossanlop pushed a commit that referenced this pull request Feb 14, 2023
@jeffhandley
Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests (#81878) (
9d02343 
#82063)

* Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests

* Revert solution file changes made by VS

* Use runtime-assets test files for Text and SQL Binary XML tests

* Remove System.Data.Common.TestData package reference

* Reference the System.Data.Common.TestData package for SqlXml tests

* Add System.Common.Data.TestData to Version.Details.xml for automated dependency flow
carlossanlop pushed a commit that referenced this pull request Feb 14, 2023
@jeffhandley
Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests (#81878) (
c5b9524 
#82062)

* Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests

* Revert solution file changes made by VS

* Use runtime-assets test files for Text and SQL Binary XML tests

* Remove System.Data.Common.TestData package reference

* Reference the System.Data.Common.TestData package for SqlXml tests

* Add System.Common.Data.TestData to Version.Details.xml for automated dependency flow
@ghost ghost locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@stephentoub stephentoub stephentoub left review comments

@jkotas jkotas jkotas left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@vitek-karas vitek-karas vitek-karas approved these changes

@krwq krwq Awaiting requested review from krwq

Assignees

@jeffhandley jeffhandley

Labels
area-System.Xml
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

XmlSqlBinaryReader is not working in .Net6
8 participants
@jeffhandley @jkotas @premun @carlossanlop @stephentoub @eiriktsarpalis @ViktorHofer @vitek-karas