SOS crash fixes (#2733)

* Fix issue https://github.com/dotnet/diagnostics/issues/2695

The exception in the service provider is caused by the recursion of the ImageMappingMemoryService memory
service. Remove the recursion by passing the "raw" memory service (the one without any "mapping" memory
service wrappers) to the module service to use to get the build id/PE reader, etc.

* Fix Windows debugger SOS termination exception

* Code review feedback

diff --git a/src/Microsoft.Diagnostics.DebugServices.Implementation/Module.cs b/src/Microsoft.Diagnostics.DebugServices.Implementation/Module.cs
index ab0e2ff3881390a8f9a5d49625ce7c98d2fa2d86..b614a2fd56f3c3f7d41509cca6d20c916fa7ed0a 100644 (file)
--- a/src/Microsoft.Diagnostics.DebugServices.Implementation/Module.cs
+++ b/src/Microsoft.Diagnostics.DebugServices.Implementation/Module.cs
@@ -90,8 +90,16 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         {
             get
             {
-                GetPEInfo();
-                return (_flags & Flags.IsPEImage) != 0;
+                // For Windows targets, we can always assume that all the modules are PEs.
+                if (Target.OperatingSystem == OSPlatform.Windows)
+                {
+                    return true;
+                }
+                else
+                {
+                    GetPEInfo();
+                    return (_flags & Flags.IsPEImage) != 0;
+                }
             }
         }
 
@@ -172,7 +180,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         {
             if (Target.OperatingSystem == OSPlatform.Windows)
             {
-                Stream stream = Target.Services.GetService<IMemoryService>().CreateMemoryStream(ImageBase, ImageSize);
+                Stream stream = ModuleService.MemoryService.CreateMemoryStream(ImageBase, ImageSize);
                 PEFile image = new(new StreamAddressSpace(stream), isDataSourceVirtualAddressSpace: true);
                 if (image.IsValid())
                 { 
@@ -189,7 +197,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
             {
                 try
                 {
-                    Stream stream = Target.Services.GetService<IMemoryService>().CreateMemoryStream(ImageBase, ImageSize);
+                    Stream stream = ModuleService.MemoryService.CreateMemoryStream(ImageBase, ImageSize);
                     ElfFile elfFile = new(stream, position: ImageBase, leaveOpen: false, isVirtual: true);
                     if (elfFile.Header.IsValid)
                     {

diff --git a/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs b/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs
index 1d85c0debfb15cc63306fdbe21501a782f267672..253a2be6e1a61a6de168705c7ef6a66852dc15cc 100644 (file)
--- a/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs
+++ b/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleService.cs
@@ -39,6 +39,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         const uint VmProtWrite = 0x02;
 
         internal protected readonly ITarget Target;
+        internal protected IMemoryService RawMemoryService;
         private IMemoryService _memoryService;
         private ISymbolService _symbolService;
         private ReadVirtualCache _versionCache;
@@ -48,10 +49,11 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         private static readonly byte[] s_versionString = Encoding.ASCII.GetBytes("@(#)Version ");
         private static readonly int s_versionLength = s_versionString.Length;
 
-        public ModuleService(ITarget target)
+        public ModuleService(ITarget target, IMemoryService rawMemoryService)
         {
             Debug.Assert(target != null);
             Target = target;
+            RawMemoryService = rawMemoryService;
 
             target.OnFlushEvent.Register(() => {
                 _versionCache?.Clear();
@@ -118,7 +120,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         /// <returns>module or null</returns>
         IModule IModuleService.GetModuleFromAddress(ulong address)
         {
-            Debug.Assert((address & ~MemoryService.SignExtensionMask()) == 0);
+            Debug.Assert((address & ~RawMemoryService.SignExtensionMask()) == 0);
             IModule[] modules = GetSortedModules();
             int min = 0, max = modules.Length - 1;
 
@@ -129,7 +131,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
                 IModule module = modules[mid];
 
                 ulong start = module.ImageBase;
-                Debug.Assert((start & ~MemoryService.SignExtensionMask()) == 0);
+                Debug.Assert((start & ~RawMemoryService.SignExtensionMask()) == 0);
                 ulong end = start + module.ImageSize;
 
                 if (address >= start && address < end) {
@@ -238,7 +240,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         /// <returns>PEImage instance or null</returns>
         private PEImage GetPEInfo(bool isVirtual, ulong address, ulong size, ref PdbFileInfo pdbFileInfo, ref Module.Flags flags)
         {
-            Stream stream = MemoryService.CreateMemoryStream(address, size);
+            Stream stream = RawMemoryService.CreateMemoryStream(address, size);
             try
             {
                 stream.Position = 0;
@@ -521,7 +523,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         /// <returns>build id or null</returns>
         internal byte[] GetBuildId(ulong address)
         {
-            Stream stream = MemoryService.CreateMemoryStream();
+            Stream stream = RawMemoryService.CreateMemoryStream();
             byte[] buildId = null;
             try
             {
@@ -556,7 +558,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
         /// <returns>version string or null</returns>
         protected string GetVersionString(ulong address)
         {
-            Stream stream = MemoryService.CreateMemoryStream();
+            Stream stream = RawMemoryService.CreateMemoryStream();
             try
             {
                 if (Target.OperatingSystem == OSPlatform.Linux)
@@ -566,7 +568,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
                     {
                         foreach (ELFProgramHeader programHeader in elfFile.Segments.Select((segment) => segment.Header))
                         {
-                            uint flags = MemoryService.PointerSize == 8 ? programHeader.Flags : programHeader.Flags32;
+                            uint flags = RawMemoryService.PointerSize == 8 ? programHeader.Flags : programHeader.Flags32;
                             if (programHeader.Type == ELFProgramHeaderType.Load &&
                                (flags & (uint)ELFProgramHeaderAttributes.Writable) != 0)
                             {
@@ -625,7 +627,8 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
             byte[] buffer = new byte[s_versionString.Length];
 
             if (_versionCache == null) {
-                _versionCache = new ReadVirtualCache(MemoryService);
+                // We use the possibly mapped memory service to find the version string in case it isn't in the dump.
+                _versionCache = new ReadVirtualCache(Target.Services.GetService<IMemoryService>());
             }
             _versionCache.Clear();
 
@@ -687,11 +690,11 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
             else {
                 return string.Equals(Path.GetFileName(module.FileName), moduleName);
             }
-        }
+        } 
 
-        protected IMemoryService MemoryService => _memoryService ??= Target.Services.GetService<IMemoryService>();
+        internal protected IMemoryService MemoryService => _memoryService ??= Target.Services.GetService<IMemoryService>();
 
-        protected ISymbolService SymbolService => _symbolService ??= Target.Services.GetService<ISymbolService>(); 
+        internal protected ISymbolService SymbolService => _symbolService ??= Target.Services.GetService<ISymbolService>(); 
 
         /// <summary>
         /// Search memory helper class

diff --git a/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleServiceFromDataReader.cs b/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleServiceFromDataReader.cs
index 8480ca4e6573d2a3525ab77b7b7c27c4bc91f998..7fd2e230ffa0c348935e4cd9bd25f96e448ba6cf 100644 (file)
--- a/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleServiceFromDataReader.cs
+++ b/src/Microsoft.Diagnostics.DebugServices.Implementation/ModuleServiceFromDataReader.cs
@@ -62,7 +62,7 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
                     {
                         ImmutableArray<byte> buildId = _moduleService._dataReader.GetBuildId(ImageBase);
                         // If the data reader can't get the build id, it returns a empty (instead of default) immutable array.
-                        _buildId = buildId.IsEmpty ? base.BuildId : buildId;
+                        _buildId = buildId.IsDefaultOrEmpty ? base.BuildId : buildId;
                     }
                     return _buildId;
                 }
@@ -122,8 +122,8 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
 
         private readonly IDataReader _dataReader;
 
-        public ModuleServiceFromDataReader(ITarget target, IDataReader dataReader)
-            : base(target)
+        public ModuleServiceFromDataReader(ITarget target, IMemoryService rawMemoryService, IDataReader dataReader)
+            : base(target, rawMemoryService)
         {
             _dataReader = dataReader;
         }

diff --git a/src/Microsoft.Diagnostics.DebugServices.Implementation/TargetFromDataReader.cs b/src/Microsoft.Diagnostics.DebugServices.Implementation/TargetFromDataReader.cs
index 3679c460aac3db219ab76437fc966edd3d6fed17..55dbe636de6e4aada6b097a3d4c82ba58d67278e 100644 (file)
--- a/src/Microsoft.Diagnostics.DebugServices.Implementation/TargetFromDataReader.cs
+++ b/src/Microsoft.Diagnostics.DebugServices.Implementation/TargetFromDataReader.cs
@@ -47,10 +47,11 @@ namespace Microsoft.Diagnostics.DebugServices.Implementation
             }
 
             // Add the thread, memory, and module services
+            IMemoryService rawMemoryService = new MemoryServiceFromDataReader(_dataReader);
             ServiceProvider.AddServiceFactory<IThreadService>(() => new ThreadServiceFromDataReader(this, _dataReader));
-            ServiceProvider.AddServiceFactory<IModuleService>(() => new ModuleServiceFromDataReader(this, _dataReader));
+            ServiceProvider.AddServiceFactory<IModuleService>(() => new ModuleServiceFromDataReader(this, rawMemoryService, _dataReader));
             ServiceProvider.AddServiceFactory<IMemoryService>(() => {
-                IMemoryService memoryService = new MemoryServiceFromDataReader(_dataReader);
+                IMemoryService memoryService = rawMemoryService;
                 if (IsDump)
                 {
                     memoryService = new ImageMappingMemoryService(this, memoryService);

diff --git a/src/SOS/SOS.Extensions/HostServices.cs b/src/SOS/SOS.Extensions/HostServices.cs
index 8ca01b708c73f968ae626baf2c2f15ec5c653b95..29572e36ba9941064879eb1742f95dd66620043e 100644 (file)
--- a/src/SOS/SOS.Extensions/HostServices.cs
+++ b/src/SOS/SOS.Extensions/HostServices.cs
@@ -302,9 +302,16 @@ namespace SOS.Extensions
             IntPtr self)
         {
             Trace.TraceInformation("HostServices.DestroyTarget #{0}", _target != null ? _target.Id : "<none>");
-            if (_target != null)
+            try
+            {
+                if (_target != null)
+                {
+                    DestroyTarget(_target);
+                }
+            }
+            catch (Exception ex)
             {
-                DestroyTarget(_target);
+                Trace.TraceError(ex.ToString());
             }
         }
 
@@ -350,22 +357,29 @@ namespace SOS.Extensions
             IntPtr self)
         {
             Trace.TraceInformation("HostServices.Uninitialize");
-            DestroyTarget(self);
-
-            if (DebuggerServices != null)
+            try
             {
-                DebuggerServices.Release();
-                DebuggerServices = null;
-            }
+                DestroyTarget(self);
+
+                if (DebuggerServices != null)
+                {
+                    DebuggerServices.Release();
+                    DebuggerServices = null;
+                }
 
-            // Send shutdown event on exit
-            OnShutdownEvent.Fire();
+                // Send shutdown event on exit
+                OnShutdownEvent.Fire();
 
-            // Release the host services wrapper
-            Release();
+                // Release the host services wrapper
+                Release();
 
-            // Clear HostService instance
-            Instance = null;
+                // Clear HostService instance
+                Instance = null;
+            }
+            catch (Exception ex)
+            {
+                Trace.TraceError(ex.ToString());
+            }
         }
 
         #endregion

diff --git a/src/SOS/SOS.Extensions/ModuleServiceFromDebuggerServices.cs b/src/SOS/SOS.Extensions/ModuleServiceFromDebuggerServices.cs
index 0e91804e183550aa7fddfb66b936c5aa2e0342f1..cbd533e41ab77466f4547ed28b2c221703289051 100644 (file)
--- a/src/SOS/SOS.Extensions/ModuleServiceFromDebuggerServices.cs
+++ b/src/SOS/SOS.Extensions/ModuleServiceFromDebuggerServices.cs
@@ -133,8 +133,8 @@ namespace SOS.Extensions
 
         private readonly DebuggerServices _debuggerServices;
 
-        internal ModuleServiceFromDebuggerServices(ITarget target, DebuggerServices debuggerServices)
-            : base(target)
+        internal ModuleServiceFromDebuggerServices(ITarget target, IMemoryService rawMemoryService, DebuggerServices debuggerServices)
+            : base(target, rawMemoryService)
         {
             Debug.Assert(debuggerServices != null);
             _debuggerServices = debuggerServices;

diff --git a/src/SOS/SOS.Extensions/TargetFromFromDebuggerServices.cs b/src/SOS/SOS.Extensions/TargetFromFromDebuggerServices.cs
index 3a44ba2f805735ae98ba62ea34e676da95eb06e6..3ce6225dae067a48ff9aac1020ea9ff727fe9848 100644 (file)
--- a/src/SOS/SOS.Extensions/TargetFromFromDebuggerServices.cs
+++ b/src/SOS/SOS.Extensions/TargetFromFromDebuggerServices.cs
@@ -72,18 +72,19 @@ namespace SOS.Extensions
             }
 
             // Add the thread, memory, and module services
-            ServiceProvider.AddServiceFactory<IModuleService>(() => new ModuleServiceFromDebuggerServices(this, debuggerServices));
+            IMemoryService rawMemoryService = new MemoryServiceFromDebuggerServices(this, debuggerServices);
+            ServiceProvider.AddServiceFactory<IModuleService>(() => new ModuleServiceFromDebuggerServices(this, rawMemoryService, debuggerServices));
             ServiceProvider.AddServiceFactory<IThreadService>(() => new ThreadServiceFromDebuggerServices(this, debuggerServices));
             ServiceProvider.AddServiceFactory<IMemoryService>(() => {
-                IMemoryService memoryService = new MemoryServiceFromDebuggerServices(this, debuggerServices);
                 Debug.Assert(Host.HostType != HostType.DotnetDump);
+                IMemoryService memoryService = rawMemoryService;
                 if (IsDump && Host.HostType == HostType.Lldb)
                 {
                     // This is a special memory service that maps the managed assemblies' metadata into the address 
                     // space. The lldb debugger returns zero's (instead of failing the memory read) for missing pages
                     // in core dumps that older (< 5.0) createdumps generate so it needs this special metadata mapping 
                     // memory service. dotnet-dump needs this logic for clrstack -i (uses ICorDebug data targets).
-                    memoryService = new MetadataMappingMemoryService(this, memoryService);
+                    return new MetadataMappingMemoryService(this, memoryService);
                 }
                 return memoryService;
             });