Fix issue with xml file line ending normalization

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

@@ -530,8 +530,8 @@ internal void AddToXml(ProjectElement child)
 
                         var parentIndentation = GetElementIndentation(XmlElement);
 
-                        var leadingWhitespaceNode = XmlDocument.CreateWhitespace("\n" + parentIndentation + DEFAULT_INDENT);
-                        var trailingWhiteSpaceNode = XmlDocument.CreateWhitespace("\n" + parentIndentation);
+                        var leadingWhitespaceNode = XmlDocument.CreateWhitespace(Environment.NewLine + parentIndentation + DEFAULT_INDENT);
+                        var trailingWhiteSpaceNode = XmlDocument.CreateWhitespace(Environment.NewLine + parentIndentation);
 
                         XmlElement.InsertBefore(leadingWhitespaceNode, child.XmlElement);
                         XmlElement.InsertAfter(trailingWhiteSpaceNode, child.XmlElement);

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

@@ -17,6 +17,7 @@
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
+using Microsoft.Build.Internal;
 #if (!STANDALONEBUILD)
 using Microsoft.Internal.Performance;
 #if MSBUILDENABLEVSPROFILING 
@@ -195,7 +196,7 @@ private ProjectRootElement(ProjectRootElementCache projectRootElementCache, NewP
 
             XmlReaderSettings xrs = new XmlReaderSettings();
             xrs.DtdProcessing = DtdProcessing.Ignore;
-            
+
             var emptyProjectFile = string.Format(EmptyProjectFileContent,
                 (projectFileOptions & NewProjectFileOptions.IncludeXmlDeclaration) != 0 ? EmptyProjectFileXmlDeclaration : string.Empty,
                 (projectFileOptions & NewProjectFileOptions.IncludeToolsVersion) != 0 ? EmptyProjectFileToolsVersion : string.Empty,
@@ -2038,19 +2039,10 @@ private XmlDocumentWithLocation LoadDocument(string fullPath, bool preserveForma
                     string beginProjectLoad = String.Format(CultureInfo.CurrentCulture, "Load Project {0} From File - Start", fullPath);
                     DataCollection.CommentMarkProfile(8806, beginProjectLoad);
 #endif
-
-                    XmlReaderSettings dtdSettings = new XmlReaderSettings();
-                    dtdSettings.DtdProcessing = DtdProcessing.Ignore;
-
-                    using (var stream = new FileStream(fullPath, FileMode.Open, FileAccess.Read, FileShare.Read))
-                    using (var stream2 = new StreamReader(stream, Encoding.UTF8, true))
-                    using (XmlReader xtr = XmlReader.Create(stream2, dtdSettings))
+                    using (var xtr = XmlReaderExtension.Create(fullPath))
                     {
-                        // Start the reader so it has an idea of what the encoding is.
-                        xtr.Read();
-                        var encoding = xtr.GetAttribute("encoding");
-                        _encoding = !string.IsNullOrEmpty(encoding) ? Encoding.GetEncoding(encoding) : stream2.CurrentEncoding;
-                        document.Load(xtr);
+                        _encoding = xtr.Encoding;
+                        document.Load(xtr.Reader);
                     }
 
                     document.FullPath = fullPath;

src/XMakeBuildEngine/ElementLocation/XmlDocumentWithLocation.cs

@@ -170,14 +170,9 @@ public override void Load(string fullPath)
 
             _fullPath = fullPath;
 
-            // For security purposes we need to disable DTD processing when loading an XML file
-            XmlReaderSettings rs = new XmlReaderSettings();
-            rs.DtdProcessing = DtdProcessing.Ignore;
-
-            using (var stream = new StreamReader(fullPath))
-            using(XmlReader xmlReader = XmlReader.Create(stream, rs))
+            using(var xtr = XmlReaderExtension.Create(fullPath))
             {
-                this.Load(xmlReader);
+                this.Load(xtr.Reader);
             }
         }
 #endif

src/XMakeBuildEngine/Evaluation/ProjectRootElementCache.cs

@@ -19,7 +19,7 @@
 using System.Diagnostics;
 using System.Globalization;
 using Microsoft.Build.BackEnd;
-
+using Microsoft.Build.Internal;
 using OutOfProcNode = Microsoft.Build.Execution.OutOfProcNode;
 
 namespace Microsoft.Build.Evaluation
@@ -238,13 +238,9 @@ internal ProjectRootElement Get(string projectFile, OpenProjectRootElement openP
                             XmlDocument document = new XmlDocument();
                             document.PreserveWhitespace = projectRootElement.XmlDocument.PreserveWhitespace;
 
-                            XmlReaderSettings dtdSettings = new XmlReaderSettings();
-                            dtdSettings.DtdProcessing = DtdProcessing.Ignore;
-
-                            using (var stream = new FileStream(projectRootElement.FullPath, FileMode.Open, FileAccess.Read, FileShare.Read))
-                            using (XmlReader xtr = XmlReader.Create(stream, dtdSettings))
+                            using (var xtr = XmlReaderExtension.Create(projectRootElement.FullPath))
                             {
-                                document.Load(xtr);
+                                document.Load(xtr.Reader);
                             }
 
                             string diskContent = document.OuterXml;

src/XMakeBuildEngine/Microsoft.Build.csproj

@@ -595,6 +595,7 @@
     <Compile Include="Xml\ProjectXmlUtilities.cs">
       <ExcludeFromStyleCop>true</ExcludeFromStyleCop>
     </Compile>
+    <Compile Include="Xml\XmlReaderExtension.cs" />
     <Compile Include="..\Shared\AssemblyLoadInfo.cs">
       <Link>SharedUtilities\AssemblyLoadInfo.cs</Link>
       <ExcludeFromStyleCop>true</ExcludeFromStyleCop>

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/WhiteSpacePreservation_Tests.cs

@@ -7,12 +7,14 @@
 
 using System;
 using System.IO;
-using System.Xml;
 using Microsoft.Build.Construction;
 
 using Xunit;
 using System.Linq;
+using System.Text.RegularExpressions;
+using System.Threading;
 using Microsoft.Build.Evaluation;
+using Microsoft.Build.Shared;
 
 
 namespace Microsoft.Build.UnitTests.OM.Construction
@@ -411,32 +413,99 @@ public void AddChildWithExistingSiblingsViaInsertBeforeChild(string projectConte
                 });
         }
 
-        private void AssertWhiteSpacePreservation(string projectContents, string updatedProject,
-            Action<ProjectRootElement, Project> act)
+        [Fact]
+        public void VerifySaveProjectContainsCorrectLineEndings()
         {
-            var projectElement =
-                ProjectRootElement.Create(
-                    XmlReader.Create(new StringReader(ObjectModelHelpers.CleanupFileContents(projectContents))),
-                    ProjectCollection.GlobalProjectCollection,
-                    true);
+            var project = @"<Project xmlns=`msbuildnamespace`>
+  <ItemGroup> <!-- comment here -->
+
+    <i Include=`a` /> <!--
+multi-line comment here
 
-            var project = new Project(projectElement);
+-->
 
-            act(projectElement, project);
+  </ItemGroup>
+</Project>
+";
+            string expected = @"<Project xmlns=`msbuildnamespace`>
+  <ItemGroup> <!-- comment here -->
+
+    <i2 Include=`b` />
 
-            var writer = new StringWriter();
-            project.Save(writer);
+    <i Include=`a` /> <!--
+multi-line comment here
 
-            var expected = @"<?xml version=""1.0"" encoding=""utf-16""?>" +
-                           ObjectModelHelpers.CleanupFileContents(updatedProject);
-            var actual = writer.ToString();
+-->
+
+  </ItemGroup>
+</Project>
+";
+            // Use existing test to add a sibling and verify the output is as expected (including comments)
+            AddChildWithExistingSiblingsViaInsertBeforeChild(project, expected);
+        }
+
+        private void AssertWhiteSpacePreservation(string projectContents, string updatedProject,
+            Action<ProjectRootElement, Project> act)
+        {
+            // Note: This test will write the project file to disk rather than using in-memory streams.
+            // Using streams can cause issues with CRLF characters being replaced by LF going in to
+            // ProjectRootElement. Saving to disk mimics the real-world behavior so we can specifically
+            // test issues with CRLF characters being normalized. Related issue: #1340
+            var file = FileUtilities.GetTemporaryFile();
+            var expected = ObjectModelHelpers.CleanupFileContents(updatedProject);
+            string actual;
+
+            try
+            {
+                // Write the projectConents to disk and load it
+                File.WriteAllText(file, ObjectModelHelpers.CleanupFileContents(projectContents));
+                var projectElement = ProjectRootElement.Open(file, ProjectCollection.GlobalProjectCollection, true);
+                var project = new Project(projectElement);
+
+                act(projectElement, project);
+
+                // Write the project to a UTF8 string writer to compare against
+                var writer = new EncodingStringWriter();
+                project.Save(writer);
+                actual = writer.ToString();
+            }
+            finally
+            {
+                FileUtilities.DeleteNoThrow(file);
+            }
 
             VerifyAssertLineByLine(expected, actual);
+
+#if FEATURE_XMLTEXTREADER
+            VerifyLineEndings(actual);
+#endif
         }
 
         private void VerifyAssertLineByLine(string expected, string actual)
         {
             Helpers.VerifyAssertLineByLine(expected, actual, false);
         }
+
+        /// <summary>
+        /// Ensure that all line-endings in the save result are correct for the current OS
+        /// </summary>
+        /// <param name="projectResults">Project file contents after save.</param>
+        private void VerifyLineEndings(string projectResults)
+        {
+            if (Environment.NewLine.Length == 2)
+            {
+                // Windows, ensure that \n doesn't exist by itself
+                var crlfCount = Regex.Matches(projectResults, @"\r\n", RegexOptions.Multiline).Count;
+                var nlCount = Regex.Matches(projectResults, @"\n").Count;
+
+                // Compare number of \r\n to number of \n, they should be equal.
+                Assert.Equal(crlfCount, nlCount);
+            }
+            else
+            {
+                // Ensure we did not add \r\n
+                Assert.Equal(0, Regex.Matches(projectResults, @"\r\n", RegexOptions.Multiline).Count);
+            }
+        }
     }
 }

src/XMakeBuildEngine/Xml/XmlReaderExtension.cs

@@ -0,0 +1,104 @@
+using System;
+using System.Diagnostics;
+using System.IO;
+using System.Reflection;
+using System.Text;
+using System.Threading;
+using System.Xml;
+
+namespace Microsoft.Build.Internal
+{
+    /// <summary>
+    ///     Disposable helper class to wrap XmlReader / XmlTextReader functionality.
+    /// </summary>
+    internal class XmlReaderExtension : IDisposable
+    {
+        /// <summary>
+        ///     Creates an XmlReaderExtension with handle to an XmlReader.
+        /// </summary>
+        /// <param name="filePath">Path to the file on disk.</param>
+        /// <returns>Disposable XmlReaderExtenion object.</returns>
+        internal static XmlReaderExtension Create(string filePath)
+        {
+            return new XmlReaderExtension(filePath);
+        }
+
+        private readonly Encoding _encoding;
+        private readonly Stream _stream;
+        private readonly StreamReader _streamReader;
+
+        private XmlReaderExtension(string file)
+        {
+            try
+            {
+                _stream = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.Read);
+                _streamReader = new StreamReader(_stream, Encoding.UTF8, true);
+                Reader = GetXmlReader(_streamReader, out _encoding);
+
+                // Override detected encoding if xml encoding attribute is specified
+                var encodingAttribute = Reader.GetAttribute("encoding");
+                _encoding = !string.IsNullOrEmpty(encodingAttribute)
+                    ? Encoding.GetEncoding(encodingAttribute)
+                    : _encoding;
+            }
+            catch
+            {
+                // GetXmlReader calls Read() to get Encoding and can throw. If it does, close 
+                // the streams as needed.
+                Dispose();
+                throw;
+            }
+        }
+
+        internal XmlReader Reader { get; }
+
+        internal Encoding Encoding => _encoding;
+
+        public void Dispose()
+        {
+            Reader?.Dispose();
+            _streamReader?.Dispose();
+            _stream?.Dispose();
+        }
+
+        private static XmlReader GetXmlReader(StreamReader input, out Encoding encoding)
+        {
+#if FEATURE_XMLTEXTREADER
+            var reader = new XmlTextReader(input) { DtdProcessing = DtdProcessing.Ignore };
+
+            reader.Read();
+            encoding = input.CurrentEncoding;
+
+            return reader;
+#else
+            var xr = XmlReader.Create(input, new XmlReaderSettings {DtdProcessing = DtdProcessing.Ignore});
+
+            // Set Normalization = false if possible. Without this, certain line endings will be normalized
+            // with \n (specifically in XML comments). Does not throw if if type or property is not found.
+            // This issue does not apply to XmlTextReader (above) which is not shipped with .NET Core yet.
+
+            // NOTE: This doesn't work in .NET Core.
+            //var xmlReaderType = typeof(XmlReader).GetTypeInfo().Assembly.GetType("System.Xml.XmlTextReaderImpl");
+
+            //// Works in full framework, not in .NET Core
+            //var normalization = xmlReaderType?.GetProperty("Normalization", BindingFlags.Instance | BindingFlags.NonPublic);
+            //normalization?.SetValue(xr, false);
+
+            //// Set _normalize = false, and _ps.eolNormalized = true
+            //var normalizationMember = xmlReaderType?.GetField("_normalize", BindingFlags.Instance | BindingFlags.NonPublic);
+            //normalizationMember?.SetValue(xr, false);
+
+            //var psField = xmlReaderType.GetField("_ps", BindingFlags.Instance | BindingFlags.NonPublic);
+            //var ps = psField.GetValue(xr);
+
+            //var eolField = ps.GetType().GetField("eolNormalized", BindingFlags.Instance | BindingFlags.NonPublic);
+            //eolField.SetValue(ps, true);
+
+            xr.Read();
+            encoding = input.CurrentEncoding;
+
+            return xr;
+#endif
+        }
+    }
+}