Add switch: Do not resolve URLs by defaults for XML #84169

krwq · 2023-03-31T14:27:45Z

The approach is adding new resolver which resolves URIs with file scheme (accessible with XmlResolver.FileSystemResolver) and switch which changes so that we use it rather than XmlUrlResolver. In most situation we never do any resolving for XMLs but XmlUrlResolver is still used to resolve URIs for root level APIs, the switch changes so that we use file resolver instead and never access the network.

This:

adds XmlFileSystemResolver (accessible with XmlResolver.FileSystemResolver)
adds a switch (System.Xml.XmlResolver.IsNetworkingEnabledByDefault) to use it

Creating PR with initial version since I wanted to collect feedback on this approach

Open questions:

current design explicitly does not prefix switch name with "Switch." but other XML switches do, other projects do not use that prefix, which convention should be followed? Are old switches mostly relevant to netfx?

TODOs:

API review
tests

Size impact:

Baseline: baseline-net8.0-private.mstat.dump.txt
After merge: net8.0-private.mstat.dump.txt
Size on disk: 7,165,952 bytes => 4,160,000

MStat numbers:

Types Total Size 883,620 => 502,900
Methods Total Size 3,628,957 => 1,835,091
Blobs Total Size 2,056,593 => 1,284,014

App for measurement (with switch set to false):

using System.Xml.Linq;

XDocument doc = XDocument.Load("file.xml");

string value = doc.Descendants("some-element").Single().Value;
if (value == "test-value")
{
    Console.WriteLine("SUCCESS");
    return 0;
}

return 1;

file.xml:

<?xml version="1.0" encoding="utf-8" ?>
<root>
  <some-element>test-value</some-element>
</root>

dotnet-issue-labeler · 2023-03-31T14:27:53Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-03-31T14:28:03Z

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

Issue Details

Fixes: #83495

This:

adds XmlFileSystemResolver
adds a switch to use it

Creating PR with initial version since I wanted to collect feedback on this approach

TODOs:

API review
tests

Size impact:

Baseline: baseline-net8.0-private.mstat.dump.txt
After merge: net8.0-private.mstat.dump.txt
Size on disk: 7,165,952 bytes => 4,160,000

MStat numbers:

Types Total Size 883,620 => 502,900
Methods Total Size 3,628,957 => 1,835,091
Blobs Total Size 2,056,593 => 1,284,014

Author:	krwq
Assignees:	-
Labels:	`area-System.Xml`, `new-api-needs-documentation`
Milestone:	-

MichalStrehovsky · 2023-04-03T07:05:09Z

I got intrigued by the LocalAppContextSwitches.AllowDefaultResolver and found https://stackoverflow.com/questions/54764271/different-behavior-for-full-framework-and-net-core-for-xml-schema-compilation. Is it true that the resolver in general is already an opt in thing? Is there any way we could hook into that? (Unless the user does the opt in gesture, this is trimmed.)

I guess if the actual API to enable the resolvers is AppContext.SetSwitch("Switch.System.Xml.AllowDefaultResolver", true); there's probably nothing we can do but it still feels like a loss that we need to introduce another gesture around this.

src/libraries/System.Private.Xml/src/System.Private.Xml.csproj

src/libraries/System.Private.Xml/src/System/Xml/AppContextSwitchHelper.cs

eerhardt · 2023-04-03T23:53:34Z

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs

+
+namespace System.Xml
+{
+    public abstract partial class XmlResolver


does this really need to be in a new file? Why can't it just be in the main XmlResolver.cs file?

I followed convention for the ThrowingResolver

eerhardt · 2023-04-03T23:59:09Z

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs

+                {
+                    if (absoluteUri.Scheme == "file")
+                    {
+                        return await Task.Run<Stream>(() => new FileStream(absoluteUri.LocalPath, FileMode.Open, FileAccess.Read, FileShare.Read, 1, useAsync: true))


This is interesting... Does FileStream's ctor really require a separate thread to run?

Why wouldn't this just be return Task.FromResult(new FileStream(...));?

I extracted this logic from XmlDownloadManager.GetStreamAsync, I think we should be able to change this without issues in both places though since I don't think ctors can be async (unless there is some magic behind the back to make that work)

@stephentoub - is there anything I'm missing here? I see you modified the XmlDownloadManager code in dotnet/corefx#41111, but the Task.Run<Stream>(...) code was already there. Is there a reason to create the FileStream on a separate thread?

cc @adamsitnik

Is there a reason to create the FileStream on a separate thread?

Maybe someone was concerned that in these scenarios the file would be living on a remote file share that could take a non-trivial amount of time to open?

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs

krwq · 2023-04-04T08:46:25Z

I got intrigued by the LocalAppContextSwitches.AllowDefaultResolver and found https://stackoverflow.com/questions/54764271/different-behavior-for-full-framework-and-net-core-for-xml-schema-compilation. Is it true that the resolver in general is already an opt in thing? Is there any way we could hook into that? (Unless the user does the opt in gesture, this is trimmed.)

I guess if the actual API to enable the resolvers is AppContext.SetSwitch("Switch.System.Xml.AllowDefaultResolver", true); there's probably nothing we can do but it still feels like a loss that we need to introduce another gesture around this.

It's true but there are 2 levels of default resolver:

root level APIs - since user explicitly uses specific path/url we assume it is safe - the old switch doesn't block that and also old switch would also block file names in those APIs making them effectively useless
references etc. - anything which loads when there is something in the payload - this is what old switch blocks for security reasons - for some places like schema it's a bit borderline that we blocked that because for schema you could argue that schema is constant and trusted while payload is not

also for this particular scenario when we use default resolver we might still want to opt-out of URLs which this switch enables.

krwq · 2023-04-04T10:51:02Z

src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs

-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get
-            {
-                return GetCachedSwitchValue("Switch.System.Xml.IgnoreEmptyKeySequencess", ref s_ignoreEmptyKeySequences);


NOTE: there used to be a typo in the switch name here, I have fixed it here according to how it's documented:
https://learn.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/runtime/appcontextswitchoverrides-element

krwq · 2023-04-04T10:51:58Z

src/libraries/System.Private.Xml/src/System.Private.Xml.csproj

-                      ReferenceOutputAssembly="false"
-                      SetTargetFramework="TargetFramework=netstandard2.0"
-                      OutputItemType="Analyzer" />
+    <ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj" ReferenceOutputAssembly="false" SetTargetFramework="TargetFramework=netstandard2.0" OutputItemType="Analyzer" />


I've tried reverting this but VS keeps on changing this back

krwq · 2023-04-04T10:54:37Z

src/libraries/System.Private.Xml/src/System/Xml/XmlUrlResolver.cs

            {
                return XmlDownloadManager.GetStream(absoluteUri, _credentials, _proxy);
            }

            throw new XmlException(SR.Xml_UnsupportedClass, string.Empty);
        }

+        // Maps a URI to an Object containing the actual resource.
+        public override async Task<object> GetEntityAsync(Uri absoluteUri, string? role, Type? ofObjectToReturn)


I moved this without changes, for some reason we had separate file for async version (same for XmlDownloadManager)

krwq · 2023-04-04T10:55:25Z

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs

+                {
+                    if (absoluteUri.Scheme == "file")
+                    {
+                        return Task.FromResult<object>(new FileStream(absoluteUri.LocalPath, FileMode.Open, FileAccess.Read, FileShare.Read, 1, useAsync: true));


note: this method is moved from *Async file which got removed. The only difference here is that we use Task.FromResult rather than Task.Run

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs

src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs

jkotas · 2023-04-04T17:20:01Z

src/libraries/System.Private.Xml/src/ILLink/ILLink.Substitutions.xml

+<linker>
+  <assembly fullname="System.Private.Xml">
+    <type fullname="System.Xml.LocalAppContextSwitches">
+      <method signature="System.Boolean get_AllowResolvingUrlsByDefault()" body="stub" value="false"


File name URLs are still valid URLs.

Should the name rather be AllowResolvingNonFileSystemUrlsByDefault?

API review decided on System.Xml.XmlResolver.IsNetworkingEnabledByDefault

ghost · 2023-05-04T23:01:56Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

ghost · 2023-06-04T11:01:44Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs

...ystem.Private.Xml/tests/TrimmingTests/XmlUrlResolverDefaults.IsNetworkingEnabledByDefault.cs

Co-authored-by: David Cantú <dacantu@microsoft.com>

…olverDefaults.IsNetworkingEnabledByDefault.cs Co-authored-by: David Cantú <dacantu@microsoft.com>

krwq · 2023-07-18T09:13:11Z

merging since the failures are unrelated to my changes

MichalStrehovsky · 2023-07-26T06:27:16Z

Real world example from these savings: #89489. The size of ilc.exe shrinking by almost 30% from 15.2 MB to 11.1 MB! Awesome savings!

(It also speeds up the inner loop of rebuilding the AOT compiler.)

krwq requested review from stephentoub, eerhardt and MichalStrehovsky March 31, 2023 14:27

dotnet-issue-labeler bot added area-System.Xml new-api-needs-documentation labels Mar 31, 2023

ghost assigned krwq Mar 31, 2023

krwq mentioned this pull request Mar 31, 2023

Make non-file URL support optional in XmlUrlResolver #83495

Closed

This was referenced Mar 31, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

eerhardt reviewed Apr 4, 2023

View reviewed changes

krwq commented Apr 4, 2023

View reviewed changes

jkotas reviewed Apr 4, 2023

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/XmlResolver.FileSystemResolver.cs Show resolved Hide resolved

jkotas reviewed Apr 4, 2023

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs Show resolved Hide resolved

jkotas reviewed Apr 4, 2023

View reviewed changes

build-analysis bot mentioned this pull request Apr 4, 2023

CI System.Text.RegularExpressions.Tests failure #84323

Closed

ghost closed this May 4, 2023

krwq reopened this May 5, 2023

This was referenced May 5, 2023

Tracking issue for CI build timeouts #76454

Closed

Several XML tests are sporadically failing in CI due to OOMs #82894

Closed

[mono][wasm] System.Numerics.Vectors.Tests assert equal failure #84775

Closed

ghost closed this Jun 4, 2023

ghost locked as resolved and limited conversation to collaborators Jul 4, 2023

krwq added 3 commits July 17, 2023 14:35

Add switch: Do not resolve URLs by defaults for XML

dc483c8

feedback, test

0dc5170

rename switch per API review

5c49b26

krwq reopened this Jul 17, 2023

krwq force-pushed the xml-trimming-not-pull-networking branch from 063d2a7 to 5c49b26 Compare July 17, 2023 13:23

krwq marked this pull request as ready for review July 17, 2023 14:07

Apply missed feedback, fix test failure

7dcbdbd

jozkee approved these changes Jul 18, 2023

View reviewed changes

src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs Outdated Show resolved Hide resolved

...ystem.Private.Xml/tests/TrimmingTests/XmlUrlResolverDefaults.IsNetworkingEnabledByDefault.cs Outdated Show resolved Hide resolved

krwq and others added 2 commits July 18, 2023 08:10

Fix typo in IgnoreEmptyKeySequences

3792282

Co-authored-by: David Cantú <dacantu@microsoft.com>

Update src/libraries/System.Private.Xml/tests/TrimmingTests/XmlUrlRes…

608da91

…olverDefaults.IsNetworkingEnabledByDefault.cs Co-authored-by: David Cantú <dacantu@microsoft.com>

krwq merged commit c2bd0c9 into dotnet:main Jul 18, 2023
100 of 103 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add switch: Do not resolve URLs by defaults for XML #84169

Add switch: Do not resolve URLs by defaults for XML #84169

krwq commented Mar 31, 2023 •

edited

Loading

dotnet-issue-labeler bot commented Mar 31, 2023

ghost commented Mar 31, 2023

MichalStrehovsky commented Apr 3, 2023

eerhardt Apr 3, 2023

krwq Apr 4, 2023

eerhardt Apr 3, 2023

krwq Apr 4, 2023 •

edited

Loading

eerhardt Apr 4, 2023

stephentoub Apr 4, 2023

krwq commented Apr 4, 2023 •

edited

Loading

krwq Apr 4, 2023

krwq Apr 4, 2023

krwq Apr 4, 2023

krwq Apr 4, 2023

jkotas Apr 4, 2023

krwq Jul 17, 2023

ghost commented May 4, 2023

ghost commented Jun 4, 2023

krwq commented Jul 18, 2023

MichalStrehovsky commented Jul 26, 2023

Add switch: Do not resolve URLs by defaults for XML #84169

Add switch: Do not resolve URLs by defaults for XML #84169

Conversation

krwq commented Mar 31, 2023 • edited Loading

dotnet-issue-labeler bot commented Mar 31, 2023

ghost commented Mar 31, 2023

MichalStrehovsky commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 4, 2023

ghost commented Jun 4, 2023

krwq commented Jul 18, 2023

MichalStrehovsky commented Jul 26, 2023

krwq commented Mar 31, 2023 •

edited

Loading

krwq Apr 4, 2023 •

edited

Loading

krwq commented Apr 4, 2023 •

edited

Loading