Fix ServiceController name population perf (dotnet/corefx#32072)
authorDan Moseley <danmose@microsoft.com>
Tue, 4 Sep 2018 22:21:28 +0000 (15:21 -0700)
committerGitHub <noreply@github.com>
Tue, 4 Sep 2018 22:21:28 +0000 (15:21 -0700)
* Fix ServiceController name population perf

* Split tests

* Remove dead field

* Remove new use of DangerousGetHandle

* SafeHandle all the things!

* VSB dotnet/corefx#1

* VSB dotnet/corefx#2

* Fix GLE

* Initialize machineName in ctor

* Test for empty name ex

* Null names

* Inadvertent edit

* Unix build

* Move interop into class

* Reverse SafeHandle for HAllocGlobal

* Fix tests

* Disable test for NETFX

* CR feedback

* Pattern matching on VSB

* Direct call

* typo

Commit migrated from https://github.com/dotnet/corefx/commit/76c8587e935fb501d57b10de44826e1f99b69236

22 files changed:
src/libraries/Common/src/Interop/Windows/Interop.Errors.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.ChangeServiceConfig2.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.ControlService.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.CreateService.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.DeleteService.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.EnumDependentServices.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.EnumServicesStatusEx.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs [new file with mode: 0644]
src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs [new file with mode: 0644]
src/libraries/Common/src/Interop/Windows/advapi32/Interop.OpenService.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.QueryServiceConfig.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.QueryServiceStatus.cs
src/libraries/Common/src/Interop/Windows/advapi32/Interop.StartService.cs
src/libraries/System.ServiceProcess.ServiceController/src/Microsoft/Win32/SafeHandles/SafeServiceHandle.cs
src/libraries/System.ServiceProcess.ServiceController/src/Resources/Strings.resx
src/libraries/System.ServiceProcess.ServiceController/src/System.ServiceProcess.ServiceController.csproj
src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs
src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs
src/libraries/System.ServiceProcess.ServiceController/tests/SafeServiceControllerTests.cs
src/libraries/System.ServiceProcess.ServiceController/tests/ServiceControllerTests.cs
src/libraries/System.ServiceProcess.ServiceController/tests/System.ServiceProcess.ServiceController.TestService/System.ServiceProcess.ServiceController.TestService.csproj
src/libraries/System.ServiceProcess.ServiceController/tests/System.ServiceProcess.ServiceController.TestService/TestServiceInstaller.cs

index 7836b04..71e6052 100644 (file)
@@ -57,6 +57,7 @@ internal partial class Interop
         internal const int ERROR_IO_INCOMPLETE = 0x3E4;
         internal const int ERROR_IO_PENDING = 0x3E5;
         internal const int ERROR_NO_TOKEN = 0x3f0;
+        internal const int ERROR_SERVICE_DOES_NOT_EXIST = 0x424;
         internal const int ERROR_DLL_INIT_FAILED = 0x45A;
         internal const int ERROR_COUNTER_TIMEOUT = 0x461;
         internal const int ERROR_NO_ASSOCIATION = 0x483;
index 9b92147..42891ba 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,9 +11,9 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        public static extern bool ChangeServiceConfig2(IntPtr serviceHandle, uint infoLevel, ref SERVICE_DESCRIPTION serviceDesc);
+        public static extern bool ChangeServiceConfig2(SafeServiceHandle serviceHandle, uint infoLevel, ref SERVICE_DESCRIPTION serviceDesc);
 
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        public static extern bool ChangeServiceConfig2(IntPtr serviceHandle, uint infoLevel, ref SERVICE_DELAYED_AUTOSTART_INFO serviceDesc);
+        public static extern bool ChangeServiceConfig2(SafeServiceHandle serviceHandle, uint infoLevel, ref SERVICE_DELAYED_AUTOSTART_INFO serviceDesc);
     }
 }
index 94cda12..a4c345b 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,7 +11,7 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        internal extern unsafe static bool ControlService(IntPtr serviceHandle, int control, SERVICE_STATUS* pStatus);
+        internal extern unsafe static bool ControlService(SafeServiceHandle serviceHandle, int control, SERVICE_STATUS* pStatus);
 
     }
 }
index 9bb9a73..c56d554 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,7 +11,7 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        public extern static IntPtr CreateService(IntPtr databaseHandle, string serviceName, string displayName, int access, int serviceType,
+        public extern static IntPtr CreateService(SafeServiceHandle databaseHandle, string serviceName, string displayName, int access, int serviceType,
             int startType, int errorControl, string binaryPath, string loadOrderGroup, IntPtr pTagId, string dependencies,
             string servicesStartName, string password);
 
index 4bd1b98..705e3a9 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,6 +11,6 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        public extern static bool DeleteService(IntPtr serviceHandle);
+        public extern static bool DeleteService(SafeServiceHandle serviceHandle);
     }
 }
index 878dc4f..67a1cf7 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -11,7 +12,7 @@ internal partial class Interop
     {
         [DllImport(Libraries.Advapi32, EntryPoint = "EnumDependentServicesW", CharSet = CharSet.Unicode, SetLastError = true)]
         internal extern static bool EnumDependentServices(
-            IntPtr serviceHandle,
+            SafeServiceHandle serviceHandle,
             int serviceState,
             IntPtr bufferOfENUM_SERVICE_STATUS,
             int bufSize,
index 84c7988..346adb3 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -11,7 +12,7 @@ internal partial class Interop
     {
         [DllImport(Libraries.Advapi32, EntryPoint = "EnumServicesStatusExW", CharSet = CharSet.Unicode, SetLastError = true)]
         internal extern static bool EnumServicesStatusEx(
-            IntPtr databaseHandle,
+            SafeServiceHandle databaseHandle,
             int infolevel,
             int serviceType,
             int serviceState,
diff --git a/src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs b/src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs
new file mode 100644 (file)
index 0000000..c2937fb
--- /dev/null
@@ -0,0 +1,19 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Microsoft.Win32.SafeHandles;
+using System;
+using System.Buffers;
+using System.ComponentModel;
+using System.Runtime.InteropServices;
+using System.Text;
+
+internal partial class Interop
+{
+    internal partial class Advapi32
+    {
+        [DllImport(Libraries.Advapi32, EntryPoint = "GetServiceDisplayNameW", CharSet = System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
+        internal static extern unsafe bool GetServiceDisplayName(SafeServiceHandle SCMHandle, string serviceName, char* displayName, ref int displayNameLength);
+    }
+}
diff --git a/src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs b/src/libraries/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs
new file mode 100644 (file)
index 0000000..de5256f
--- /dev/null
@@ -0,0 +1,19 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Microsoft.Win32.SafeHandles;
+using System;
+using System.Buffers;
+using System.ComponentModel;
+using System.Runtime.InteropServices;
+using System.Text;
+
+internal partial class Interop
+{
+    internal partial class Advapi32
+    {
+        [DllImport(Libraries.Advapi32, EntryPoint = "GetServiceKeyNameW", CharSet = System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
+        internal static extern unsafe bool GetServiceKeyName(SafeServiceHandle SCMHandle, string displayName, char* KeyName, ref int KeyNameLength);
+    }
+}
index 5ef71c1..813c322 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,6 +11,6 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, EntryPoint = "OpenServiceW", CharSet = CharSet.Unicode, SetLastError = true)]
-        internal extern static IntPtr OpenService(IntPtr databaseHandle, string serviceName, int access);
+        internal extern static IntPtr OpenService(SafeServiceHandle databaseHandle, string serviceName, int access);
     }
 }
index 73bfe71..c91db2f 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,7 +11,7 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, EntryPoint = "QueryServiceConfigW", CharSet = CharSet.Unicode, SetLastError = true)]
-        internal extern static bool QueryServiceConfig(IntPtr serviceHandle, IntPtr queryServiceConfigPtr, int bufferSize, out int bytesNeeded);
+        internal extern static bool QueryServiceConfig(SafeServiceHandle serviceHandle, IntPtr queryServiceConfigPtr, int bufferSize, out int bytesNeeded);
 
     }
 }
index a271f7b..6badc4c 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,7 +11,7 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
-        internal static extern unsafe bool QueryServiceStatus(IntPtr serviceHandle, SERVICE_STATUS* pStatus);
+        internal static extern unsafe bool QueryServiceStatus(SafeServiceHandle serviceHandle, SERVICE_STATUS* pStatus);
 
     }
 }
index cf29770..0695320 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Runtime.InteropServices;
 
@@ -10,6 +11,6 @@ internal partial class Interop
     internal partial class Advapi32
     {
         [DllImport(Libraries.Advapi32, EntryPoint = "StartServiceW", CharSet = CharSet.Unicode, SetLastError = true)]
-        internal extern static bool StartService(IntPtr serviceHandle, int argNum, IntPtr argPtrs);
+        internal extern static bool StartService(SafeServiceHandle serviceHandle, int argNum, IntPtr argPtrs);
     }
 }
index 3cba464..e8ed13d 100644 (file)
@@ -9,6 +9,9 @@ using System.Runtime.InteropServices;
 
 namespace Microsoft.Win32.SafeHandles
 {
+    /// <summary>
+    /// Used to wrap handles gotten from OpenSCManager or OpenService
+    /// </summary>
     internal class SafeServiceHandle : SafeHandle
     {
         internal SafeServiceHandle(IntPtr handle) : base(IntPtr.Zero, true)
index 2f40d0e..b870528 100644 (file)
     <value>Arguments within the 'args' array passed to Start cannot be null.</value>
   </data>
   <data name="BadMachineName" xml:space="preserve">
-    <value>MachineName value {0} is invalid.</value>
+    <value>MachineName '{0}' is invalid.</value>
   </data>
   <data name="CannotStart" xml:space="preserve">
-    <value>Cannot start service {0} on computer '{1}'.</value>
+    <value>Cannot start service '{0}' on computer '{1}'.</value>
   </data>
   <data name="InvalidEnumArgument" xml:space="preserve">
     <value>The value of argument '{0}' ({1}) is invalid for Enum type '{2}'.</value>
     <value>MachineName was not set.</value>
   </data>
   <data name="NoService" xml:space="preserve">
-    <value>Service {0} was not found on computer '{1}'.</value>
+    <value>Service '{0}' was not found on computer '{1}'.</value>
   </data>
   <data name="OpenSC" xml:space="preserve">
     <value>Cannot open Service Control Manager on computer '{0}'. This operation might require other privileges.</value>
   </data>
   <data name="OpenService" xml:space="preserve">
-    <value>Cannot open {0} service on computer '{1}'.</value>
+    <value>Cannot open '{0}' service on computer '{1}'.</value>
   </data>
   <data name="PauseService" xml:space="preserve">
-    <value>Cannot pause {0} service on computer '{1}'.</value>
+    <value>Cannot pause '{0}' service on computer '{1}'.</value>
   </data>
   <data name="ResumeService" xml:space="preserve">
-    <value>Cannot resume {0} service on computer '{1}'.</value>
+    <value>Cannot resume '{0}' service on computer '{1}'.</value>
   </data>
   <data name="StopService" xml:space="preserve">
-    <value>Cannot stop {0} service on computer '{1}'.</value>
+    <value>Cannot stop '{0}' service on computer '{1}'.</value>
   </data>
   <data name="Timeout" xml:space="preserve">
     <value>Time out has expired and the operation has not been completed.</value>
     <value>Cannot change service name when the service is running.</value>
   </data>
   <data name="ServiceName" xml:space="preserve">
-    <value>Service name {0} contains invalid characters, is empty, or is too long (max length = {1}).</value>
+    <value>Service name '{0}' contains invalid characters, is empty, or is too long (max length = {1}).</value>
   </data>
   <data name="NoServices" xml:space="preserve">
     <value>Service has not been supplied. At least one object derived from ServiceBase is required in order to run.</value>
     <value>Failed in handling the PowerEvent. The error that occurred was: {0}.</value>
   </data>
   <data name="InstallOK" xml:space="preserve">
-    <value>Service {0} has been successfully installed.</value>
+    <value>Service '{0}' has been successfully installed.</value>
   </data>
   <data name="TryToStop" xml:space="preserve">
-    <value>Attempt to stop service {0}.</value>
+    <value>Attempt to stop service '{0}'.</value>
   </data>
   <data name="ServiceRemoving" xml:space="preserve">
-    <value>Service {0} is being removed from the system...</value>
+    <value>Service '{0}' is being removed from the system...</value>
   </data>
   <data name="ServiceRemoved" xml:space="preserve">
-    <value>Service {0} was successfully removed from the system.</value>
+    <value>Service '{0}' was successfully removed from the system.</value>
   </data>
   <data name="ControlService" xml:space="preserve">
-    <value>Cannot control {0} service on computer '{1}'.</value>
+    <value>Cannot control '{0}' service on computer '{1}'.</value>
   </data>
 </root>
index e2d6f56..caf54b7 100644 (file)
@@ -12,6 +12,9 @@
     <Configurations>net461-Windows_NT-Debug;net461-Windows_NT-Release;netcoreapp-Windows_NT-Debug;netcoreapp-Windows_NT-Release;netfx-Windows_NT-Debug;netfx-Windows_NT-Release;netstandard-Debug;netstandard-Release;netstandard-Windows_NT-Debug;netstandard-Windows_NT-Release</Configurations>
   </PropertyGroup>
   <ItemGroup Condition="$(TargetGroup.StartsWith('netcoreapp')) OR ('$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' == 'true')">
+    <Compile Include="$(CommonPath)\CoreLib\System\Text\ValueStringBuilder.cs">
+      <Link>Common\CoreLib\System\Text\ValueStringBuilder.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\Interop.Libraries.cs">
       <Link>Common\Interop\Windows\Interop.Libraries.cs</Link>
     </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.EnumServicesStatusEx.cs">
       <Link>Common\Interop\Windows\Interop.EnumServicesStatusEx.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.GetServiceDisplayName.cs">
+      <Link>Common\Interop\Windows\Interop.GetServiceDisplayName.cs</Link>
+    </Compile>
+    <Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.GetServiceKeyName.cs">
+      <Link>Common\Interop\Windows\Interop.GetServiceKeyName.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.OpenSCManager.cs">
       <Link>Common\Interop\Windows\Interop.OpenSCManager.cs</Link>
     </Compile>
   </ItemGroup>
   <ItemGroup Condition="$(TargetGroup.StartsWith('netcoreapp')) OR ('$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' == 'true')">
     <Reference Include="Microsoft.Win32.Primitives" />
+    <Reference Include="System.Buffers" />
     <Reference Include="System.Collections" />
     <Reference Include="System.Console" />
     <Reference Include="System.ComponentModel.Primitives" />
     <Reference Include="System.Diagnostics.Debug" />
     <Reference Include="System.Diagnostics.Tools" />
+    <Reference Include="System.Memory" />
     <Reference Include="System.Resources.ResourceManager" />
     <Reference Include="System.Runtime" />
+    <Reference Include="System.Runtime.Extensions" />
     <Reference Include="System.Runtime.InteropServices" />
     <Reference Include="System.Threading" />
     <Reference Include="System.Threading.Thread" />
index e66ad3e..0f0b680 100644 (file)
@@ -602,58 +602,65 @@ namespace System.ServiceProcess
             int sizeOfSERVICE_TABLE_ENTRY = Marshal.SizeOf<SERVICE_TABLE_ENTRY>();            
 
             IntPtr entriesPointer = Marshal.AllocHGlobal(checked((services.Length + 1) * sizeOfSERVICE_TABLE_ENTRY));
-            SERVICE_TABLE_ENTRY[] entries = new SERVICE_TABLE_ENTRY[services.Length];
-            bool multipleServices = services.Length > 1;
-            IntPtr structPtr;
-
-            for (int index = 0; index < services.Length; ++index)
+            try
             {
-                services[index].Initialize(multipleServices);
-                entries[index] = services[index].GetEntry();
-                structPtr = entriesPointer + sizeOfSERVICE_TABLE_ENTRY * index;
-                Marshal.StructureToPtr(entries[index], structPtr, fDeleteOld: false);
-            }
+                SERVICE_TABLE_ENTRY[] entries = new SERVICE_TABLE_ENTRY[services.Length];
+                bool multipleServices = services.Length > 1;
+                IntPtr structPtr;
 
-            SERVICE_TABLE_ENTRY lastEntry = new SERVICE_TABLE_ENTRY();
+                for (int index = 0; index < services.Length; ++index)
+                {
+                    services[index].Initialize(multipleServices);
+                    entries[index] = services[index].GetEntry();
+                    structPtr = entriesPointer + sizeOfSERVICE_TABLE_ENTRY * index;
+                    Marshal.StructureToPtr(entries[index], structPtr, fDeleteOld: false);
+                }
 
-            lastEntry.callback = null;
-            lastEntry.name = (IntPtr)0;
-            structPtr = entriesPointer + sizeOfSERVICE_TABLE_ENTRY * services.Length;
-            Marshal.StructureToPtr(lastEntry, structPtr, fDeleteOld: false);
+                SERVICE_TABLE_ENTRY lastEntry = new SERVICE_TABLE_ENTRY();
 
-            // While the service is running, this function will never return. It will return when the service
-            // is stopped.
-            // After it returns, SCM might terminate the process at any time
-            // (so subsequent code is not guaranteed to run).
-            bool res = StartServiceCtrlDispatcher(entriesPointer);
+                lastEntry.callback = null;
+                lastEntry.name = (IntPtr)0;
+                structPtr = entriesPointer + sizeOfSERVICE_TABLE_ENTRY * services.Length;
+                Marshal.StructureToPtr(lastEntry, structPtr, fDeleteOld: false);
 
-            foreach (ServiceBase service in services)
-            {
-                if (service._startFailedException != null)
+                // While the service is running, this function will never return. It will return when the service
+                // is stopped.
+                // After it returns, SCM might terminate the process at any time
+                // (so subsequent code is not guaranteed to run).
+                bool res = StartServiceCtrlDispatcher(entriesPointer);
+
+                foreach (ServiceBase service in services)
                 {
-                    // Propagate exceptions throw during OnStart.
-                    // Note that this same exception is also thrown from ServiceMainCallback
-                    // (so SCM can see it as well).
-                    service._startFailedException.Throw();
+                    if (service._startFailedException != null)
+                    {
+                        // Propagate exceptions throw during OnStart.
+                        // Note that this same exception is also thrown from ServiceMainCallback
+                        // (so SCM can see it as well).
+                        service._startFailedException.Throw();
+                    }
                 }
-            }
 
-            string errorMessage = "";
+                string errorMessage = "";
 
-            if (!res)
-            {
-                errorMessage = new Win32Exception().Message;
-                Console.WriteLine(SR.CantStartFromCommandLine);
-            }
-
-            foreach (ServiceBase service in services)
-            {
-                service.Dispose();
                 if (!res)
                 {
-                    service.WriteLogEntry(SR.Format(SR.StartFailed, errorMessage), true);
+                    errorMessage = new Win32Exception().Message;
+                    Console.WriteLine(SR.CantStartFromCommandLine);
+                }
+
+                foreach (ServiceBase service in services)
+                {
+                    service.Dispose();
+                    if (!res)
+                    {
+                        service.WriteLogEntry(SR.Format(SR.StartFailed, errorMessage), true);
+                    }
                 }
             }
+            finally
+            {
+                Marshal.FreeHGlobal(entriesPointer);
+            }
         }
 
         /// <devdoc>
index 0dabc4e..35123d0 100644 (file)
@@ -19,13 +19,14 @@ namespace System.ServiceProcess
     /// and manipulate it or get information about it.
     public class ServiceController : Component
     {
-        private string _machineName;
+        private string _machineName; // Never null
         private readonly ManualResetEvent _waitForStatusSignal = new ManualResetEvent(false);
         private const string DefaultMachineName = ".";
 
         private string _name;
         private string _eitherName;
         private string _displayName;
+
         private int _commandsAccepted;
         private bool _statusGenerated;
         private bool _startTypeInitialized;
@@ -37,11 +38,9 @@ namespace System.ServiceProcess
         private ServiceController[] _servicesDependedOn;
         private ServiceStartMode _startType;
 
-        private const int SERVICENAMEMAXLENGTH = 80;
-        private const int DISPLAYNAMEBUFFERSIZE = 256;
-
         public ServiceController()
         {
+            _machineName = DefaultMachineName;
             _type = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ALL;
         }
 
@@ -133,7 +132,7 @@ namespace System.ServiceProcess
         {
             get
             {
-                if (_displayName == null)
+                if (String.IsNullOrEmpty(_displayName))
                     GenerateNames();
                 return _displayName;
             }
@@ -163,8 +162,7 @@ namespace System.ServiceProcess
             {
                 if (_dependentServices == null)
                 {
-                    IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_ENUMERATE_DEPENDENTS);
-                    try
+                    using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_ENUMERATE_DEPENDENTS))
                     {
                         // figure out how big a buffer we need to get the info
                         int bytesNeeded = 0;
@@ -207,10 +205,6 @@ namespace System.ServiceProcess
                             Marshal.FreeHGlobal(enumBuffer);
                         }
                     }
-                    finally
-                    {
-                        Interop.Advapi32.CloseServiceHandle(serviceHandle);
-                    }
                 }
 
                 return _dependentServices;
@@ -247,7 +241,7 @@ namespace System.ServiceProcess
         {
             get
             {
-                if (_name == null)
+                if (String.IsNullOrEmpty(_name))
                     GenerateNames();
                 return _name;
             }
@@ -280,8 +274,7 @@ namespace System.ServiceProcess
                 if (_servicesDependedOn != null)
                     return _servicesDependedOn;
 
-                IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_CONFIG);
-                try
+                using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_CONFIG))
                 {
                     int bytesNeeded = 0;
                     bool success = Interop.Advapi32.QueryServiceConfig(serviceHandle, IntPtr.Zero, 0, out bytesNeeded);
@@ -358,10 +351,6 @@ namespace System.ServiceProcess
                         Marshal.FreeHGlobal(bufPtr);
                     }
                 }
-                finally
-                {
-                    Interop.Advapi32.CloseServiceHandle(serviceHandle);
-                }
             }
         }
 
@@ -372,11 +361,8 @@ namespace System.ServiceProcess
                 if (_startTypeInitialized)
                     return _startType;
 
-                IntPtr serviceHandle = IntPtr.Zero;
-                try
+                using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_CONFIG))
                 {
-                    serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_CONFIG);
-
                     int bytesNeeded = 0;
                     bool success = Interop.Advapi32.QueryServiceConfig(serviceHandle, IntPtr.Zero, 0, out bytesNeeded);
 
@@ -385,10 +371,9 @@ namespace System.ServiceProcess
                         throw new Win32Exception(lastError);
 
                     // get the info
-                    IntPtr bufPtr = IntPtr.Zero;
+                    IntPtr bufPtr = Marshal.AllocHGlobal((IntPtr)bytesNeeded);
                     try
                     {
-                        bufPtr = Marshal.AllocHGlobal((IntPtr)bytesNeeded);
                         success = Interop.Advapi32.QueryServiceConfig(serviceHandle, bufPtr, bytesNeeded, out bytesNeeded);
                         if (!success)
                             throw new Win32Exception(Marshal.GetLastWin32Error());
@@ -401,14 +386,8 @@ namespace System.ServiceProcess
                     }
                     finally
                     {
-                        if (bufPtr != IntPtr.Zero)
-                            Marshal.FreeHGlobal(bufPtr);
-                    }
-                }
-                finally
-                {
-                    if (serviceHandle != IntPtr.Zero)
-                        Interop.Advapi32.CloseServiceHandle(serviceHandle);
+                        Marshal.FreeHGlobal(bufPtr);
+                    } 
                 }
 
                 return _startType;
@@ -419,7 +398,7 @@ namespace System.ServiceProcess
         {
             get
             {
-                return new SafeServiceHandle(GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_ALL_ACCESS));
+                return GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_ALL_ACCESS);
             }
         }
 
@@ -448,14 +427,15 @@ namespace System.ServiceProcess
             return !string.IsNullOrWhiteSpace(value) && value.IndexOf('\\') == -1;
         }
 
+        /// <summary>
+        /// Closes the handle to the service manager, but does not
+        /// mark the class as disposed.
+        /// </summary>
+        /// <remarks>
+        /// Violates design guidelines by not matching Dispose() -- matches .NET Framework
+        /// </remarks>
         public void Close()
         {
-            Dispose();
-        }
-
-        /// Disconnects this object from the service and frees any allocated resources.
-        protected override void Dispose(bool disposing)
-        {
             if (_serviceManagerHandle != null)
             {
                 _serviceManagerHandle.Dispose();
@@ -465,15 +445,23 @@ namespace System.ServiceProcess
             _statusGenerated = false;
             _startTypeInitialized = false;
             _type = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ALL;
+        }
+
+        /// <summary>
+        /// Closes the handle to the service manager, and disposes.
+        /// </summary>
+        protected override void Dispose(bool disposing)
+        {
+            Close();
             _disposed = true;
+            base.Dispose(disposing);
         }
 
         private unsafe void GenerateStatus()
         {
             if (!_statusGenerated)
             {
-                IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_STATUS);
-                try
+                using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_QUERY_STATUS))
                 {
                     Interop.Advapi32.SERVICE_STATUS svcStatus = new Interop.Advapi32.SERVICE_STATUS();
                     bool success = Interop.Advapi32.QueryServiceStatus(serviceHandle, &svcStatus);
@@ -485,107 +473,139 @@ namespace System.ServiceProcess
                     _type = svcStatus.serviceType;
                     _statusGenerated = true;
                 }
-                finally
-                {
-                    Interop.Advapi32.CloseServiceHandle(serviceHandle);
-                }
             }
         }
 
         private unsafe void GenerateNames()
         {
-            if (_machineName.Length == 0)
-                throw new ArgumentException(SR.NoMachineName);
-
-            IntPtr databaseHandle = IntPtr.Zero;
-            IntPtr memory = IntPtr.Zero;
-            int bytesNeeded;
-            int servicesReturned;
-            int resumeHandle = 0;
+            GetDataBaseHandleWithConnectAccess();
 
-            try
+            if (String.IsNullOrEmpty(_name))
             {
-                databaseHandle = GetDataBaseHandleWithEnumerateAccess(_machineName);
-                Interop.Advapi32.EnumServicesStatusEx(
-                    databaseHandle,
-                    Interop.Advapi32.ServiceControllerOptions.SC_ENUM_PROCESS_INFO,
-                    Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32 | Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_DRIVER,
-                    Interop.Advapi32.StatusOptions.STATUS_ALL,
-                    IntPtr.Zero,
-                    0,
-                    out bytesNeeded,
-                    out servicesReturned,
-                    ref resumeHandle,
-                    null);
+                // Figure out the _name based on the information we have. 
+                // We must either have _displayName or the constructor parameter _eitherName.
+                string userGivenName = String.IsNullOrEmpty(_eitherName) ? _displayName : _eitherName;
 
-                memory = Marshal.AllocHGlobal(bytesNeeded);
+                if (String.IsNullOrEmpty(userGivenName))
+                    throw new InvalidOperationException(SR.Format(SR.ServiceName, userGivenName, ServiceBase.MaxNameLength.ToString(CultureInfo.CurrentCulture)));
 
-                Interop.Advapi32.EnumServicesStatusEx(
-                    databaseHandle,
-                    Interop.Advapi32.ServiceControllerOptions.SC_ENUM_PROCESS_INFO,
-                    Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32 | Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_DRIVER,
-                    Interop.Advapi32.StatusOptions.STATUS_ALL,
-                    memory,
-                    bytesNeeded,
-                    out bytesNeeded,
-                    out servicesReturned,
-                    ref resumeHandle,
-                    null);
-
-                // Since the service name of one service cannot be equal to the
-                // service or display name of another service, we can safely
-                // loop through all services checking if either the service or
-                // display name matches the user given name. If there is a
-                // match, then we've found the service.
-                for (int i = 0; i < servicesReturned; i++)
+                // Try it as a display name
+                string result = GetServiceKeyName(_serviceManagerHandle, userGivenName);
+
+                if (result != null)
                 {
-                    IntPtr structPtr = (IntPtr)((long)memory + (i * Marshal.SizeOf<Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS>()));
-                    Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS status = new Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS();
-                    Marshal.PtrToStructure(structPtr, status);
+                    // Now we have both
+                    _name = result;
+                    _displayName = userGivenName;
+                    _eitherName = null;
+                    return;
+                }
 
-                    if (string.Equals(_eitherName, status.serviceName, StringComparison.OrdinalIgnoreCase) ||
-                        string.Equals(_eitherName, status.displayName, StringComparison.OrdinalIgnoreCase))
-                    {
-                        if (_name == null)
-                        {
-                            _name = status.serviceName;
-                        }
+                // Try it as a service name
+                result = GetServiceDisplayName(_serviceManagerHandle, userGivenName);
 
-                        if (_displayName == null)
-                        {
-                            _displayName = status.displayName;
-                        }
+                if (result == null)
+                {
+                    throw new InvalidOperationException(SR.Format(SR.NoService, userGivenName, _machineName), new Win32Exception(Interop.Errors.ERROR_SERVICE_DOES_NOT_EXIST));
+                }
 
-                        _eitherName = string.Empty;
-                        return;
-                    }
+                _name = userGivenName;
+                _displayName = result;
+                _eitherName = null;
+            }
+            else if (String.IsNullOrEmpty(_displayName))
+            {
+                // We must have _name
+                string result = GetServiceDisplayName(_serviceManagerHandle, _name);
+
+                if (result == null)
+                {
+                    throw new InvalidOperationException(SR.Format(SR.NoService, _name, _machineName), new Win32Exception(Interop.Errors.ERROR_SERVICE_DOES_NOT_EXIST));
                 }
 
-                throw new InvalidOperationException(SR.Format(SR.NoService, _eitherName, _machineName));
+                _displayName = result;
+                _eitherName = null;
             }
-            finally
+        }
+
+        /// <summary>
+        /// Gets service name (key name) from service display name.
+        /// Returns null if service is not found.
+        /// </summary>
+        private unsafe string GetServiceKeyName(SafeServiceHandle SCMHandle, string serviceDisplayName)
+        {
+            Span<char> initialBuffer = stackalloc char[256];
+            var builder = new ValueStringBuilder(initialBuffer);
+            int bufLen;
+            while (true)
             {
-                Marshal.FreeHGlobal(memory);
-                if (databaseHandle != IntPtr.Zero)
+                bufLen = builder.Capacity;
+                fixed (char* c = builder)
+                {
+                    if (Interop.Advapi32.GetServiceKeyName(SCMHandle, serviceDisplayName, c, ref bufLen))
+                        break;
+                }
+
+                int lastError = Marshal.GetLastWin32Error();
+                if (lastError == Interop.Errors.ERROR_SERVICE_DOES_NOT_EXIST)
+                {
+                    return null;
+                }
+                else if (lastError != Interop.Errors.ERROR_INSUFFICIENT_BUFFER)
                 {
-                    Interop.Advapi32.CloseServiceHandle(databaseHandle);
+                    throw new InvalidOperationException(SR.Format(SR.NoService, serviceDisplayName, _machineName), new Win32Exception(lastError));
                 }
+
+                builder.EnsureCapacity(bufLen + 1); // Does not include null
             }
+
+            builder.Length = bufLen;
+            return builder.ToString();
         }
 
-        private static IntPtr GetDataBaseHandleWithAccess(string machineName, int serviceControlManagerAccess)
+        private unsafe string GetServiceDisplayName(SafeServiceHandle SCMHandle, string serviceName)
         {
-            IntPtr databaseHandle = IntPtr.Zero;
+            var builder = new ValueStringBuilder(4096);
+            int bufLen;
+            while (true)
+            {
+                bufLen = builder.Capacity;
+                fixed (char* c = builder)
+                {
+                    if (Interop.Advapi32.GetServiceDisplayName(SCMHandle, serviceName, c, ref bufLen))
+                        break;
+                }
+
+                int lastError = Marshal.GetLastWin32Error();
+                if (lastError == Interop.Errors.ERROR_SERVICE_DOES_NOT_EXIST)
+                {
+                    return null;
+                }
+                else if (lastError != Interop.Errors.ERROR_INSUFFICIENT_BUFFER)
+                {
+                    throw new InvalidOperationException(SR.Format(SR.NoService, serviceName, _machineName), new Win32Exception(lastError));
+                }
+
+                builder.EnsureCapacity(bufLen + 1); // Does not include null
+            }
+
+            builder.Length = bufLen;
+            return builder.ToString();
+        }
+
+        private static SafeServiceHandle GetDataBaseHandleWithAccess(string machineName, int serviceControlManagerAccess)
+        {
+            SafeServiceHandle databaseHandle = null;
             if (machineName.Equals(DefaultMachineName) || machineName.Length == 0)
             {
-                databaseHandle = Interop.Advapi32.OpenSCManager(null, null, serviceControlManagerAccess);
+                databaseHandle = new SafeServiceHandle(Interop.Advapi32.OpenSCManager(null, null, serviceControlManagerAccess));
             }
             else
             {
-                databaseHandle = Interop.Advapi32.OpenSCManager(machineName, null, serviceControlManagerAccess);
+                databaseHandle = new SafeServiceHandle(Interop.Advapi32.OpenSCManager(machineName, null, serviceControlManagerAccess));
             }
 
-            if (databaseHandle == IntPtr.Zero)
+            if (databaseHandle.IsInvalid)
             {
                 Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
                 throw new InvalidOperationException(SR.Format(SR.OpenSC, machineName), inner);
@@ -604,15 +624,10 @@ namespace System.ServiceProcess
             // get a handle to SCM with connect access and store it in serviceManagerHandle field.
             if (_serviceManagerHandle == null)
             {
-                _serviceManagerHandle = new SafeServiceHandle(GetDataBaseHandleWithAccess(_machineName, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_CONNECT));
+                _serviceManagerHandle = GetDataBaseHandleWithAccess(_machineName, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_CONNECT);
             }
         }
 
-        private static IntPtr GetDataBaseHandleWithEnumerateAccess(string machineName)
-        {
-            return GetDataBaseHandleWithAccess(machineName, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ENUMERATE_SERVICE);
-        }
-
         /// Gets all the device-driver services on the local machine.
         public static ServiceController[] GetDevices()
         {
@@ -625,14 +640,13 @@ namespace System.ServiceProcess
             return GetServicesOfType(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_DRIVER);
         }
 
-        /// Opens a handle for the current service. The handle must be closed with
-        /// a call to Interop.CloseServiceHandle().
-        private IntPtr GetServiceHandle(int desiredAccess)
+        /// Opens a handle for the current service. The handle must be Dispose()'d.
+        private SafeServiceHandle GetServiceHandle(int desiredAccess)
         {
             GetDataBaseHandleWithConnectAccess();
 
-            IntPtr serviceHandle = Interop.Advapi32.OpenService(_serviceManagerHandle.DangerousGetHandle(), ServiceName, desiredAccess);
-            if (serviceHandle == IntPtr.Zero)
+            var serviceHandle = new SafeServiceHandle(Interop.Advapi32.OpenService(_serviceManagerHandle, ServiceName, desiredAccess));
+            if (serviceHandle.IsInvalid)
             {
                 Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
                 throw new InvalidOperationException(SR.Format(SR.OpenService, ServiceName, _machineName), inner);
@@ -671,17 +685,14 @@ namespace System.ServiceProcess
         /// Helper for GetDevices, GetServices, and ServicesDependedOn
         private static T[] GetServices<T>(string machineName, int serviceType, string group, Func<Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS, T> selector)
         {
-            IntPtr databaseHandle = IntPtr.Zero;
-            IntPtr memory = IntPtr.Zero;
             int bytesNeeded;
             int servicesReturned;
             int resumeHandle = 0;
 
             T[] services;
 
-            try
+            using (SafeServiceHandle databaseHandle = GetDataBaseHandleWithAccess(machineName, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ENUMERATE_SERVICE))
             {
-                databaseHandle = GetDataBaseHandleWithEnumerateAccess(machineName);
                 Interop.Advapi32.EnumServicesStatusEx(
                     databaseHandle,
                     Interop.Advapi32.ServiceControllerOptions.SC_ENUM_PROCESS_INFO,
@@ -694,42 +705,39 @@ namespace System.ServiceProcess
                     ref resumeHandle,
                     group);
 
-                memory = Marshal.AllocHGlobal((IntPtr)bytesNeeded);
-
-                //
-                // Get the set of services
-                //
-                Interop.Advapi32.EnumServicesStatusEx(
-                    databaseHandle,
-                    Interop.Advapi32.ServiceControllerOptions.SC_ENUM_PROCESS_INFO,
-                    serviceType,
-                    Interop.Advapi32.StatusOptions.STATUS_ALL,
-                    memory,
-                    bytesNeeded,
-                    out bytesNeeded,
-                    out servicesReturned,
-                    ref resumeHandle,
-                    group);
-
-                //
-                // Go through the block of memory it returned to us and select the results
-                //
-                services = new T[servicesReturned];
-                for (int i = 0; i < servicesReturned; i++)
+                IntPtr memory = Marshal.AllocHGlobal((IntPtr)bytesNeeded);
+                try
                 {
-                    IntPtr structPtr = (IntPtr)((long)memory + (i * Marshal.SizeOf<Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS>()));
-                    Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS status = new Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS();
-                    Marshal.PtrToStructure(structPtr, status);
-                    services[i] = selector(status);
+                    //
+                    // Get the set of services
+                    //
+                    Interop.Advapi32.EnumServicesStatusEx(
+                        databaseHandle,
+                        Interop.Advapi32.ServiceControllerOptions.SC_ENUM_PROCESS_INFO,
+                        serviceType,
+                        Interop.Advapi32.StatusOptions.STATUS_ALL,
+                        memory,
+                        bytesNeeded,
+                        out bytesNeeded,
+                        out servicesReturned,
+                        ref resumeHandle,
+                        group);
+
+                    //
+                    // Go through the block of memory it returned to us and select the results
+                    //
+                    services = new T[servicesReturned];
+                    for (int i = 0; i < servicesReturned; i++)
+                    {
+                        IntPtr structPtr = (IntPtr)((long)memory + (i * Marshal.SizeOf<Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS>()));
+                        Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS status = new Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS();
+                        Marshal.PtrToStructure(structPtr, status);
+                        services[i] = selector(status);
+                    }
                 }
-            }
-            finally
-            {
-                Marshal.FreeHGlobal(memory);
-
-                if (databaseHandle != IntPtr.Zero)
+                finally
                 {
-                    Interop.Advapi32.CloseServiceHandle(databaseHandle);
+                    Marshal.FreeHGlobal(memory);
                 }
             }
 
@@ -739,8 +747,7 @@ namespace System.ServiceProcess
         /// Suspends a service's operation.
         public unsafe void Pause()
         {
-            IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_PAUSE_CONTINUE);
-            try
+            using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_PAUSE_CONTINUE))
             {
                 Interop.Advapi32.SERVICE_STATUS status = new Interop.Advapi32.SERVICE_STATUS();
                 bool result = Interop.Advapi32.ControlService(serviceHandle, Interop.Advapi32.ControlOptions.CONTROL_PAUSE, &status);
@@ -750,17 +757,12 @@ namespace System.ServiceProcess
                     throw new InvalidOperationException(SR.Format(SR.PauseService, ServiceName, _machineName), inner);
                 }
             }
-            finally
-            {
-                Interop.Advapi32.CloseServiceHandle(serviceHandle);
-            }
         }
 
         /// Continues a service after it has been paused.
         public unsafe void Continue()
         {
-            IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_PAUSE_CONTINUE);
-            try
+            using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_PAUSE_CONTINUE))
             {
                 Interop.Advapi32.SERVICE_STATUS status = new Interop.Advapi32.SERVICE_STATUS();
                 bool result = Interop.Advapi32.ControlService(serviceHandle, Interop.Advapi32.ControlOptions.CONTROL_CONTINUE, &status);
@@ -770,16 +772,11 @@ namespace System.ServiceProcess
                     throw new InvalidOperationException(SR.Format(SR.ResumeService, ServiceName, _machineName), inner);
                 }
             }
-            finally
-            {
-                Interop.Advapi32.CloseServiceHandle(serviceHandle);
-            }
         }
 
         public unsafe void ExecuteCommand(int command)
         {
-            IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_USER_DEFINED_CONTROL);
-            try
+            using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_USER_DEFINED_CONTROL))
             {
                 Interop.Advapi32.SERVICE_STATUS status = new Interop.Advapi32.SERVICE_STATUS();
                 bool result = Interop.Advapi32.ControlService(serviceHandle, command, &status);
@@ -789,10 +786,6 @@ namespace System.ServiceProcess
                     throw new InvalidOperationException(SR.Format(SR.ControlService, ServiceName, MachineName), inner);
                 }
             }
-            finally
-            {
-                Interop.Advapi32.CloseServiceHandle(serviceHandle);
-            }
         }
 
         /// Refreshes all property values.
@@ -816,9 +809,7 @@ namespace System.ServiceProcess
             if (args == null)
                 throw new ArgumentNullException(nameof(args));
 
-            IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_START);
-
-            try
+            using (SafeServiceHandle serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_START))
             {
                 IntPtr[] argPtrs = new IntPtr[args.Length];
                 int i = 0;
@@ -858,10 +849,6 @@ namespace System.ServiceProcess
                         argPtrsHandle.Free();
                 }
             }
-            finally
-            {
-                Interop.Advapi32.CloseServiceHandle(serviceHandle);
-            }
         }
 
         /// Stops the service. If any other services depend on this one for operation,
@@ -869,9 +856,7 @@ namespace System.ServiceProcess
         /// of services.
         public unsafe void Stop()
         {
-            IntPtr serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_STOP);
-
-            try
+            using (SafeServiceHandle serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_STOP))
             {
                 // Before stopping this service, stop all the dependent services that are running.
                 // (It's OK not to cache the result of getting the DependentServices property because it caches on its own.)
@@ -894,10 +879,6 @@ namespace System.ServiceProcess
                     throw new InvalidOperationException(SR.Format(SR.StopService, ServiceName, _machineName), inner);
                 }
             }
-            finally
-            {
-                Interop.Advapi32.CloseServiceHandle(serviceHandle);
-            }
         }
 
         /// Waits infinitely until the service has reached the given status.
index 441f55c..cf3a26a 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.ComponentModel;
 using Xunit;
 
 namespace System.ServiceProcess.Tests
@@ -59,6 +60,22 @@ namespace System.ServiceProcess.Tests
             Assert.True(foundOtherSvc, "foundOtherSvc");
         }
 
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        public static void ConstructWithBadServiceName(string value)
+        {
+            Assert.Throws<ArgumentException>(() => new ServiceController(value));
+        }
+
+        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Full framework does not throw")]
+        public static void Initialize_GetNames()
+        {
+            Assert.Throws<InvalidOperationException>(() => new ServiceController().ServiceName);
+            Assert.Throws<InvalidOperationException>(() => new ServiceController().DisplayName);
+        }
+
         [Fact]
         public static void GetDevices()
         {
@@ -88,6 +105,96 @@ namespace System.ServiceProcess.Tests
         }
 
         [Fact]
+        public static void NonExistentService_GetStatus()
+        {
+            var controller = new ServiceController(Guid.NewGuid().ToString("N"));
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.Status);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void NonExistentService_GetDisplayName()
+        {
+            var controller = new ServiceController(Guid.NewGuid().ToString("N"));
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.DisplayName);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void SetNameToNonexistentService_GetStatus()
+        {
+            var controller = new ServiceController();
+            controller.ServiceName = Guid.NewGuid().ToString("N");
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.Status);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void SetNameToNonexistentService_GetDisplayName()
+        {
+            var controller = new ServiceController();
+            controller.ServiceName = Guid.NewGuid().ToString("N");
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.DisplayName);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void SetDisplayNameToNonexistentService_GetStatus()
+        {
+            var controller = new ServiceController();
+            controller.DisplayName = Guid.NewGuid().ToString("N");
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.Status);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void SetDisplayNameToNonexistentService_GetServiceName()
+        {
+            var controller = new ServiceController();
+            controller.DisplayName = Guid.NewGuid().ToString("N");
+            Exception exception = Assert.Throws<InvalidOperationException>(() => controller.ServiceName);
+            Assert.IsType<Win32Exception>(exception.InnerException);
+        }
+
+        [Fact]
+        public static void SetServiceName_GetDisplayName()
+        {
+            var keyIsoDisplayName = new ServiceController(KeyIsoSvcName).DisplayName;
+
+            var controller = new ServiceController();
+            controller.ServiceName = KeyIsoSvcName;
+            Assert.Equal(keyIsoDisplayName, controller.DisplayName);
+        }
+
+        [Fact]
+        public static void SetDisplayName_GetServiceName()
+        {
+            var keyIsoDisplayName = new ServiceController(KeyIsoSvcName).DisplayName;
+
+            var controller = new ServiceController();
+            controller.DisplayName = keyIsoDisplayName;
+            Assert.Equal(KeyIsoSvcName.ToLowerInvariant(), controller.ServiceName.ToLowerInvariant());
+        }
+
+        [Fact]
+        public static void InitializeServiceName_GetDisplayName()
+        {
+            var controller = new ServiceController(KeyIsoSvcName);
+            Assert.Equal(KeyIsoSvcName, controller.ServiceName);
+            Assert.NotEmpty(controller.DisplayName);
+        }
+
+        [Fact]
+        public static void InitializeDisplayName_GetServiceName()
+        {
+            var keyIsoDisplayName = new ServiceController(KeyIsoSvcName).DisplayName;
+
+            var controller = new ServiceController(keyIsoDisplayName);
+            Assert.Equal(KeyIsoSvcName.ToLowerInvariant(), controller.ServiceName.ToLowerInvariant());
+            Assert.Equal(keyIsoDisplayName, controller.DisplayName);
+        }
+
+        [Fact]
         public static void WaitForStatusTimeout()
         {
             var controller = new ServiceController(KeyIsoSvcName);
index c39395c..f56b382 100644 (file)
@@ -25,8 +25,7 @@ namespace System.ServiceProcess.Tests
 
         private void AssertExpectedProperties(ServiceController testServiceController)
         {
-            var comparer = PlatformDetection.IsFullFramework ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal; // Full framework upper cases the name
-            Assert.Equal(_testService.TestServiceName, testServiceController.ServiceName, comparer);
+            Assert.Equal(_testService.TestServiceName, testServiceController.ServiceName, StringComparer.OrdinalIgnoreCase);
             Assert.Equal(_testService.TestServiceDisplayName, testServiceController.DisplayName);
             Assert.Equal(_testService.TestMachineName, testServiceController.MachineName);
             Assert.Equal(ServiceType.Win32OwnProcess, testServiceController.ServiceType);
index 06d5a3c..2b5d1d4 100644 (file)
@@ -12,6 +12,9 @@
     <Compile Include="TestService.cs">
       <SubType>Component</SubType>
     </Compile>
+    <Compile Include="..\..\src\Microsoft\Win32\SafeHandles\SafeServiceHandle.cs">
+      <Link>Microsoft\Win32\SafeHandles\SafeServiceHandle.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\Interop.Libraries.cs">
       <Link>Common\Interop\Windows\Interop.Libraries.cs</Link>
     </Compile>
index db9b3fe..101d1bc 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using Microsoft.Win32.SafeHandles;
 using System.ComponentModel;
 using System.Diagnostics;
 using System.Text;
@@ -52,7 +53,7 @@ namespace System.ServiceProcess.Tests
                 ServiceCommandLine = $"\"{processName}\" {arguments}";
             }
 
-            //Build servicesDependedOn string
+            // Build servicesDependedOn string
             string servicesDependedOn = null;
             if (ServicesDependedOn.Length > 0)
             {
@@ -70,53 +71,45 @@ namespace System.ServiceProcess.Tests
             }
 
             // Open the service manager
-            IntPtr serviceManagerHandle = Interop.Advapi32.OpenSCManager(null, null, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ALL);
-            IntPtr serviceHandle = IntPtr.Zero;
-            if (serviceManagerHandle == IntPtr.Zero)
-                throw new InvalidOperationException("Cannot open Service Control Manager");
-
-            try
+            using (var serviceManagerHandle = new SafeServiceHandle(Interop.Advapi32.OpenSCManager(null, null, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ALL)))
             {
+                if (serviceManagerHandle.IsInvalid)
+                    throw new InvalidOperationException("Cannot open Service Control Manager");
+
                 // Install the service
-                serviceHandle = Interop.Advapi32.CreateService(serviceManagerHandle, ServiceName,
+                using (var serviceHandle = new SafeServiceHandle(Interop.Advapi32.CreateService(serviceManagerHandle, ServiceName,
                     DisplayName, Interop.Advapi32.ServiceAccessOptions.ACCESS_TYPE_ALL, Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32_OWN_PROCESS,
                     (int)StartType, Interop.Advapi32.ServiceStartErrorModes.ERROR_CONTROL_NORMAL,
-                    ServiceCommandLine, null, IntPtr.Zero, servicesDependedOn, username, password);
-
-                if (serviceHandle == IntPtr.Zero)
-                    throw new Win32Exception("Cannot create service");
+                    ServiceCommandLine, null, IntPtr.Zero, servicesDependedOn, username, password)))
+                {
+                    if (serviceHandle.IsInvalid)
+                        throw new Win32Exception("Cannot create service");
 
-                // A local variable in an unsafe method is already fixed -- so we don't need a "fixed { }" blocks to protect
-                // across the p/invoke calls below.
+                    // A local variable in an unsafe method is already fixed -- so we don't need a "fixed { }" blocks to protect
+                    // across the p/invoke calls below.
 
-                if (Description.Length != 0)
-                {
-                    Interop.Advapi32.SERVICE_DESCRIPTION serviceDesc = new Interop.Advapi32.SERVICE_DESCRIPTION();
-                    serviceDesc.description = Marshal.StringToHGlobalUni(Description);
-                    bool success = Interop.Advapi32.ChangeServiceConfig2(serviceHandle, Interop.Advapi32.ServiceConfigOptions.SERVICE_CONFIG_DESCRIPTION, ref serviceDesc);
-                    Marshal.FreeHGlobal(serviceDesc.description);
-                    if (!success)
-                        throw new Win32Exception("Cannot set description");
-                }
+                    if (Description.Length != 0)
+                    {
+                        Interop.Advapi32.SERVICE_DESCRIPTION serviceDesc = new Interop.Advapi32.SERVICE_DESCRIPTION();
+                        serviceDesc.description = Marshal.StringToHGlobalUni(Description);
+                        bool success = Interop.Advapi32.ChangeServiceConfig2(serviceHandle, Interop.Advapi32.ServiceConfigOptions.SERVICE_CONFIG_DESCRIPTION, ref serviceDesc);
+                        Marshal.FreeHGlobal(serviceDesc.description);
+                        if (!success)
+                            throw new Win32Exception("Cannot set description");
+                    }
 
-                // Start the service after creating it
-                using (ServiceController svc = new ServiceController(ServiceName))
-                {
-                    if (svc.Status != ServiceControllerStatus.Running)
+                    // Start the service after creating it
+                    using (ServiceController svc = new ServiceController(ServiceName))
                     {
-                        svc.Start();
-                        if (!ServiceName.StartsWith("PropagateExceptionFromOnStart"))
-                            svc.WaitForStatus(ServiceControllerStatus.Running, TimeSpan.FromSeconds(30));
+                        if (svc.Status != ServiceControllerStatus.Running)
+                        {
+                            svc.Start();
+                            if (!ServiceName.StartsWith("PropagateExceptionFromOnStart"))
+                                svc.WaitForStatus(ServiceControllerStatus.Running, TimeSpan.FromSeconds(30));
+                        }
                     }
                 }
             }
-            finally
-            {
-                if (serviceHandle != IntPtr.Zero)
-                    Interop.Advapi32.CloseServiceHandle(serviceHandle);
-
-                Interop.Advapi32.CloseServiceHandle(serviceManagerHandle);
-            }
         }
 
         public void RemoveService()
@@ -170,28 +163,21 @@ namespace System.ServiceProcess.Tests
 
         private void DeleteService()
         {
-            IntPtr serviceManagerHandle = Interop.Advapi32.OpenSCManager(null, null, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ALL);
-            if (serviceManagerHandle == IntPtr.Zero)
-                throw new Win32Exception("Could not open SCM");
-
-            IntPtr serviceHandle = IntPtr.Zero;
-            try
+            using (var serviceManagerHandle = new SafeServiceHandle(Interop.Advapi32.OpenSCManager(null, null, Interop.Advapi32.ServiceControllerOptions.SC_MANAGER_ALL)))
             {
-                serviceHandle = Interop.Advapi32.OpenService(serviceManagerHandle,
-                    ServiceName, Interop.Advapi32.ServiceOptions.STANDARD_RIGHTS_DELETE);
-
-                if (serviceHandle == IntPtr.Zero)
-                    throw new Win32Exception($"Could not find service '{ServiceName}'");
+                if (serviceManagerHandle.IsInvalid)
+                    throw new Win32Exception("Could not open SCM");
 
-                if (!Interop.Advapi32.DeleteService(serviceHandle))
-                    throw new Win32Exception($"Could not delete service '{ServiceName}'");
-            }
-            finally
-            {
-                if (serviceHandle != IntPtr.Zero)
-                    Interop.Advapi32.CloseServiceHandle(serviceHandle);
+                using (var serviceHandle = new SafeServiceHandle(Interop.Advapi32.OpenService(serviceManagerHandle, ServiceName, Interop.Advapi32.ServiceOptions.STANDARD_RIGHTS_DELETE)))
+                {
+                    if (serviceHandle.IsInvalid)
+                        throw new Win32Exception($"Could not find service '{ServiceName}'");
 
-                Interop.Advapi32.CloseServiceHandle(serviceManagerHandle);
+                    if (!Interop.Advapi32.DeleteService(serviceHandle))
+                    {
+                        throw new Win32Exception($"Could not delete service '{ServiceName}'");
+                    }
+                }
             }
         }
     }