') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests by jeffhandley · Pull Request #81878 · dotnet/runtime · GitHub
Skip to content

Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests - #81878

Merged
jeffhandley merged 7 commits into
dotnet:mainfrom
jeffhandley:jeffhandley/xmlsqlbinaryreader
Feb 13, 2023
Merged

Fix XmlSqlBinaryReader and introduce a corpus of SqlXml tests#81878
jeffhandley merged 7 commits into
dotnet:mainfrom
jeffhandley:jeffhandley/xmlsqlbinaryreader

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Feb 9, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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!

@jeffhandleyjeffhandley added this to the 8.0.0 milestone Feb 9, 2023
@jeffhandleyjeffhandley self-assigned this Feb 9, 2023
@ghost

ghost commented Feb 9, 2023

Copy link
Copy Markdown

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

@vitek-karasvitek-karas left a comment

Copy link
Copy Markdown
Member

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

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

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-analysisbuild-analysisBot mentioned this pull request Feb 9, 2023
Closed
stephentoub
stephentoub reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/libraries/System.Data.Common/System.Data.Common.sln Outdated
stephentoub
stephentoub reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/libraries/System.Data.Common/tests/System/Data/SqlTypes/SqlXmlTest.cs Outdated
stephentoub
stephentoub reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/libraries/System.Data.Common/tests/System/Data/SqlTypes/SqlXmlTest.cs Outdated
stephentoub
stephentoub reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/libraries/System.Data.Common/tests/System/Data/SqlTypes/SqlXmlTest.cs Outdated
stephentoub
stephentoub reviewed Feb 9, 2023
View reviewed changes
Comment threadsrc/libraries/System.Data.Common/tests/System/Data/SqlTypes/SqlXmlTest.cs Outdated
@jkotas

jkotas commented Feb 9, 2023

Copy link
Copy Markdown
Member

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

premun commented Feb 9, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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

@jeffhandleyjeffhandley mentioned this pull request Feb 9, 2023
Merged
@jeffhandley

jeffhandley commented Feb 11, 2023

Copy link
Copy Markdown
MemberAuthor

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
jeffhandleyforce-pushed the jeffhandley/xmlsqlbinaryreader branch from b69923b to fda630aCompareFebruary 11, 2023 01:30
@jeffhandley
jeffhandleyforce-pushed the jeffhandley/xmlsqlbinaryreader branch from fda630a to 030ba31CompareFebruary 11, 2023 01:32
@jeffhandley

jeffhandley commented Feb 11, 2023

Copy link
Copy Markdown
MemberAuthor

@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

carlossanlop commented Feb 12, 2023

Copy link
Copy Markdown
Contributor

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
carlossanlop
carlossanlop reviewed Feb 13, 2023
View reviewed changes
Comment threadeng/Version.Details.xml
@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

jeffhandley commented Feb 13, 2023

Copy link
Copy Markdown
MemberAuthor

Failures are unrelated:

@jeffhandley
jeffhandley merged commit a9bf05b into dotnet:mainFeb 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
@ghostghost locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas left review comments
@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes
@ViktorHoferViktorHoferViktorHofer approved these changes
@vitek-karasvitek-karasvitek-karas approved these changes
@krwqkrwqAwaiting requested review from krwq
+1 more reviewer
@carlossanlopcarlossanlopcarlossanlop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jeffhandleyjeffhandley

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

9 participants

@jeffhandley@jkotas@premun@carlossanlop@stephentoub@eiriktsarpalis@ViktorHofer@vitek-karas@layomia