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

Restrict embedded XML #1675

Merged
merged 7 commits into from
Dec 10, 2020
Merged

Restrict embedded XML #1675

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
065cedb
Restrict embedded XML
sbomer Dec 4, 2020
4020a6f
Allow specifying resource name in referenced assembly
sbomer Dec 7, 2020
cbd01bf
Fix formatting
sbomer Dec 7, 2020
159e331
PR feedback
sbomer Dec 9, 2020
eaade7e
Fix test on mono
sbomer Dec 9, 2020
12d5f36
PR feedback
sbomer Dec 10, 2020
22d14e3
PR feedback
sbomer Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions docs/error-codes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1511,3 +1511,29 @@ This is technically possible if a custom assembly defines `DynamicDependencyAttr
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
object TestProperty { get; set; }
```

### 'IL2100': XML contains unsupported wildcard for assembly "fullname" attribute

- A wildcard "fullname" for an assembly in XML is only valid for link attribute XML on the command-line, not for descriptor or substitution XML or for embedded attribute XML. Specify a specific assembly name instead.

```XML
<!-- IL2100: XML contains unsupported wildcard for assembly "fullname" attribute -->
<linker>
<assembly fullname="*">
<type fullname="MyType" />
</assembly>
</linker>
```

### 'IL2101': Embedded XML in assembly 'assembly' contains assembly "fullname" attribute for another assembly 'assembly'

- Embedded attribute or substitution XML may only contain elements that apply to the containing assembly. Attempting to modify another assembly will not have any effect.

```XML
<!-- IL2101: Embedded XML in assembly 'ContainingAssembly' contains assembly "fullname" attribute for another assembly 'OtherAssembly' -->
<linker>
<assembly fullname="OtherAssembly">
Copy link
Contributor

Choose a reason for hiding this comment

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

fullname is now redundant for embedded XML format, should we just remove/ignore it instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's good to warn when it's used incorrectly in the embedded XML, otherwise people might try to embed existing XML and wonder why it doesn't work.

Since the assembly is redundant for embedded XML, I made it so that you can leave out the assembly entirely:

<linker>
  <type fullname="..." />
</linker>

I kept support for <assembly fullname="ThisAssembly>" since I think it will be easier to reuse existing XML this way, but I could force the embedded XML not to name the assembly if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I think this is more wording issue than anything else. Your "It is invalid to use these embedded XML files to modify other assemblies." seems to suggest this will be error where in reality this is a warning. Maybe we could go with even simple mode and merge 2100 and 2101 under the simple warning that warns that fullname value is ignored for embedded XML files (just suggestion)

Copy link
Member Author

Choose a reason for hiding this comment

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

I reworded the warnings - hopefully this is better. I'd like to keep them separate because the wildcard can produce a warning for non-embedded XML too.

<type fullname="MyType" />
</assembly>
</linker>
```
6 changes: 3 additions & 3 deletions src/linker/Linker.Steps/BodySubstituterStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ protected override void Process ()
ProcessXml (Context.StripSubstitutions, Context.IgnoreSubstitutions);
}

protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
{
ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
ProcessResources (assembly, iterator.Current.SelectChildren ("resource", ""));
ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
ProcessResources (assembly, nav.SelectChildren ("resource", ""));
}

protected override TypeDefinition ProcessExportedType (ExportedType exported, AssemblyDefinition assembly) => null;
Expand Down
24 changes: 20 additions & 4 deletions src/linker/Linker.Steps/LinkAttributesStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,14 +197,30 @@ protected override void Process ()
ProcessXml (Context.StripLinkAttributes, Context.IgnoreLinkAttributes);
}

protected override bool AllowAllAssembliesSelector { get => true; }
protected override AllowedAssemblies AllowedAssemblySelector {
get {
if (_resourceAssembly == null)
return AllowedAssemblies.AllAssemblies;

// Corelib XML may contain assembly wildcard to support compiler-injected attribute types
#if NETCOREAPP
var coreLib = "System.Private.CoreLib";
sbomer marked this conversation as resolved.
Show resolved Hide resolved
#else
var coreLib = "mscorlib";
#endif
if (_resourceAssembly.Name.Name == coreLib)
return AllowedAssemblies.AllAssemblies;

return AllowedAssemblies.ContainingAssembly;
}
}

protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
{
IEnumerable<CustomAttribute> attributes = ProcessAttributes (iterator.Current, assembly);
IEnumerable<CustomAttribute> attributes = ProcessAttributes (nav, assembly);
if (attributes.Any ())
Context.CustomAttributes.AddCustomAttributes (assembly, attributes);
ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
}

protected override void ProcessType (TypeDefinition type, XPathNavigator nav)
Expand Down
50 changes: 39 additions & 11 deletions src/linker/Linker.Steps/ProcessLinkerXmlStepBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;
Expand All @@ -7,6 +7,14 @@

namespace Mono.Linker.Steps
{
[Flags]
public enum AllowedAssemblies
{
ContainingAssembly = 0x1,
AnyAssembly = 0x2 | ContainingAssembly,
AllAssemblies = 0x4 | AnyAssembly
}

public abstract class ProcessLinkerXmlStepBase : LoadReferencesStep
{
const string FullNameAttributeName = "fullname";
Expand All @@ -25,7 +33,7 @@ public abstract class ProcessLinkerXmlStepBase : LoadReferencesStep
protected readonly string _xmlDocumentLocation;
readonly XPathDocument _document;
readonly EmbeddedResource _resource;
readonly AssemblyDefinition _resourceAssembly;
protected readonly AssemblyDefinition _resourceAssembly;

protected ProcessLinkerXmlStepBase (XPathDocument document, string xmlDocumentLocation)
{
Expand All @@ -44,6 +52,9 @@ protected ProcessLinkerXmlStepBase (XPathDocument document, EmbeddedResource res

protected virtual void ProcessXml (bool stripResource, bool ignoreResource)
{
if (!AllowedAssemblySelector.HasFlag (AllowedAssemblies.AnyAssembly) && _resourceAssembly == null)
throw new InvalidOperationException ("The containing assembly must be specified for XML which is restricted to modifying that assembly only.");

try {
XPathNavigator nav = _document.CreateNavigator ();

Expand All @@ -63,48 +74,65 @@ protected virtual void ProcessXml (bool stripResource, bool ignoreResource)

ProcessAssemblies (nav.SelectChildren ("assembly", ""));

// For embedded XML, allow not specifying the assembly explicitly in XML.
if (_resourceAssembly != null)
ProcessAssembly (_resourceAssembly, nav, warnOnUnresolvedTypes: true);

} catch (Exception ex) when (!(ex is LinkerFatalErrorException)) {
throw new LinkerFatalErrorException (MessageContainer.CreateErrorMessage ($"Error processing '{_xmlDocumentLocation}'", 1013), ex);
}
}

protected virtual bool AllowAllAssembliesSelector { get => false; }
protected virtual AllowedAssemblies AllowedAssemblySelector { get => _resourceAssembly != null ? AllowedAssemblies.ContainingAssembly : AllowedAssemblies.AnyAssembly; }

protected virtual void ProcessAssemblies (XPathNodeIterator iterator)
{
while (iterator.MoveNext ()) {
bool processAllAssemblies = AllowAllAssembliesSelector && GetFullName (iterator.Current) == AllAssembliesFullName;
bool processAllAssemblies = GetFullName (iterator.Current) == AllAssembliesFullName;
if (processAllAssemblies && AllowedAssemblySelector != AllowedAssemblies.AllAssemblies) {
Context.LogWarning ($"XML contains unsupported wildcard for assembly \"fullname\" attribute", 2100, _xmlDocumentLocation);
continue;
}

// Errors for invalid assembly names should show up even if this element will be
// skipped due to feature conditions.
var name = processAllAssemblies ? null : GetAssemblyName (iterator.Current);

AssemblyDefinition assemblyToProcess = null;
if (!AllowedAssemblySelector.HasFlag (AllowedAssemblies.AnyAssembly)) {
if (_resourceAssembly.Name.Name != name.Name) {
Context.LogWarning ($"Embedded XML in assembly '{_resourceAssembly.Name.Name}' contains assembly \"fullname\" attribute for another assembly '{name}'", 2101, _xmlDocumentLocation);
continue;
}
assemblyToProcess = _resourceAssembly;
}

if (!ShouldProcessElement (iterator.Current))
continue;

if (processAllAssemblies) {
foreach (AssemblyDefinition assembly in Context.GetAssemblies ())
ProcessAssembly (assembly, iterator, warnOnUnresolvedTypes: false);
ProcessAssembly (assembly, iterator.Current, warnOnUnresolvedTypes: false);
} else {
AssemblyDefinition assembly = GetAssembly (Context, name);
AssemblyDefinition assembly = assemblyToProcess ?? GetAssembly (Context, name);

if (assembly == null) {
Context.LogWarning ($"Could not resolve assembly '{name.Name}'", 2007, _xmlDocumentLocation);
continue;
}

ProcessAssembly (assembly, iterator, warnOnUnresolvedTypes: true);
ProcessAssembly (assembly, iterator.Current, warnOnUnresolvedTypes: true);
}
}
}

protected abstract void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes);
protected abstract void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes);

protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
{
iterator = iterator.Current.SelectChildren (TypeElementName, XmlNamespace);
var iterator = nav.SelectChildren (TypeElementName, XmlNamespace);
while (iterator.MoveNext ()) {
XPathNavigator nav = iterator.Current;
nav = iterator.Current;

if (!ShouldProcessElement (nav))
continue;
Expand Down
16 changes: 9 additions & 7 deletions src/linker/Linker.Steps/ResolveFromXmlStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,27 @@ protected override void Process ()
ProcessXml (Context.StripDescriptors, Context.IgnoreDescriptors);
}

protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNodeIterator iterator, bool warnOnUnresolvedTypes)
protected override AllowedAssemblies AllowedAssemblySelector { get => AllowedAssemblies.AnyAssembly; }

protected override void ProcessAssembly (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
{
#if !FEATURE_ILLINK
if (IsExcluded (iterator.Current))
if (IsExcluded (nav))
return;
#endif

if (GetTypePreserve (iterator.Current) == TypePreserve.All) {
if (GetTypePreserve (nav) == TypePreserve.All) {
foreach (var type in assembly.MainModule.Types)
MarkAndPreserveAll (type);
} else {
ProcessTypes (assembly, iterator, warnOnUnresolvedTypes);
ProcessNamespaces (assembly, iterator);
ProcessTypes (assembly, nav, warnOnUnresolvedTypes);
ProcessNamespaces (assembly, nav);
}
}

void ProcessNamespaces (AssemblyDefinition assembly, XPathNodeIterator iterator)
void ProcessNamespaces (AssemblyDefinition assembly, XPathNavigator nav)
{
iterator = iterator.Current.SelectChildren (NamespaceElementName, XmlNamespace);
var iterator = nav.SelectChildren (NamespaceElementName, XmlNamespace);
while (iterator.MoveNext ()) {
if (!ShouldProcessElement (iterator.Current))
continue;
Expand Down
17 changes: 15 additions & 2 deletions test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileAfterAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;

namespace Mono.Linker.Tests.Cases.Expectations.Metadata
{
Expand All @@ -8,13 +8,26 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
public class SetupCompileAfterAttribute : BaseMetadataAttribute
{
public SetupCompileAfterAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null)
public SetupCompileAfterAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null)
{
if (sourceFiles == null)
throw new ArgumentNullException (nameof (sourceFiles));

if (string.IsNullOrEmpty (outputName))
throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));

if (resources != null) {
foreach (var res in resources) {
if (res is string)
continue;
if (res is string[] stringArray) {
if (stringArray.Length != 2)
throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
continue;
}
throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
}
}
}
}
}
32 changes: 29 additions & 3 deletions test/Mono.Linker.Tests.Cases.Expectations/Metadata/SetupCompileBeforeAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;

namespace Mono.Linker.Tests.Cases.Expectations.Metadata
{
Expand All @@ -8,22 +8,48 @@ namespace Mono.Linker.Tests.Cases.Expectations.Metadata
[AttributeUsage (AttributeTargets.Class, AllowMultiple = true)]
public class SetupCompileBeforeAttribute : BaseMetadataAttribute
{
public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
public SetupCompileBeforeAttribute (string outputName, string[] sourceFiles, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
{
if (sourceFiles == null)
throw new ArgumentNullException (nameof (sourceFiles));

if (string.IsNullOrEmpty (outputName))
throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));

if (resources != null) {
foreach (var res in resources) {
if (res is string)
continue;
if (res is string[] stringArray) {
if (stringArray.Length != 2)
throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
continue;
}
throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
}
}
}

public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, string[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
public SetupCompileBeforeAttribute (string outputName, Type[] typesToIncludeSourceFor, string[] references = null, string[] defines = null, object[] resources = null, string additionalArguments = null, string compilerToUse = null, bool addAsReference = true, bool removeFromLinkerInput = false)
{
if (typesToIncludeSourceFor == null)
throw new ArgumentNullException (nameof (typesToIncludeSourceFor));

if (string.IsNullOrEmpty (outputName))
throw new ArgumentException ("Value cannot be null or empty.", nameof (outputName));

if (resources != null) {
foreach (var res in resources) {
if (res is string)
continue;
if (res is string[] stringArray) {
if (stringArray.Length != 2)
throw new ArgumentException ("Entry in object[] cannot be a string[] unless it has exactly two elements, for the resource path and name", nameof (resources));
continue;
}
throw new ArgumentException ("Each value in the object[] must be a string or a string[], either a resource path, or a path and name", nameof (resources));
}
}
}
}
}
4 changes: 2 additions & 2 deletions ...ases/DynamicDependencies/DynamicDependencyMethodInNonReferencedAssemblyWithEmbeddedXml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies;
using Mono.Linker.Tests.Cases.Expectations.Assertions;
Expand All @@ -15,7 +15,7 @@ namespace Mono.Linker.Tests.Cases.DynamicDependencies
"DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll",
new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.cs" },
references: new[] { "base.dll" },
resources: new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
resources: new object[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
addAsReference: false)]
[KeptAssembly ("base.dll")]
[RemovedMemberInAssembly ("DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll", "Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies.DynamicDependencyMethodInNonReferencedAssemblyLibrary", "UnusedMethod()")]
Expand Down
4 changes: 2 additions & 2 deletions ...amicDependencies/DynamicDependencyOnUnusedMethodInNonReferencedAssemblyWithEmbeddedXml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Mono.Linker.Tests.Cases.DynamicDependencies.Dependencies;
using Mono.Linker.Tests.Cases.Expectations.Assertions;
Expand All @@ -15,7 +15,7 @@ namespace Mono.Linker.Tests.Cases.DynamicDependencies
"DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll",
new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.cs" },
references: new[] { "base.dll" },
resources: new[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
resources: new object[] { "Dependencies/DynamicDependencyMethodInNonReferencedAssemblyLibrary.xml" },
addAsReference: false)]
[KeptAssembly ("base.dll")]
[RemovedAssembly ("DynamicDependencyMethodInNonReferencedAssemblyLibrary.dll")]
Expand Down
6 changes: 6 additions & 0 deletions test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/EmbeddedAttributeErrorCases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace Mono.Linker.Tests.Cases.LinkAttributes.Dependencies
{
public class EmbeddedAttributeErrorCases
{
}
}
10 changes: 10 additions & 0 deletions test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/EmbeddedAttributeErrorCases.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<linker>
<!-- IL2100 -->
<assembly fullname="*" />
<!-- IL2101 -->
<assembly fullname="test">
<type fullname="Mono.Linker.Tests.Cases.LinkAttributes.LinkAttributeErrorCases/ReferencedFromOtherAssembly">
<attribute fullname="Mono.Linker.Tests.Cases.LinkAttributes.LinkAttributeErrorCases/FirstAttribute" />
</type>
</assembly>
</linker>
14 changes: 14 additions & 0 deletions test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/MockCorelib.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#if INCLUDE_MOCK_CORELIB

using System;

[assembly: MockCorelibAttributeToRemove]

namespace System
{
public class MockCorelibAttributeToRemove : Attribute
{
}
}

#endif
7 changes: 7 additions & 0 deletions test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/MockCorelib.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<linker>
<assembly fullname="*">
<type fullname="System.MockCorelibAttributeToRemove">
<attribute internal="RemoveAttributeInstances" />
</type>
</assembly>
</linker>
Loading