Fix exception and enable event logging when service has no name (#67827)

* Increase timeouts that are sporadically being hit

* Add logging to TestServiceInstaller

* Update ServiceTypeOptions to match header

* Rename SERVICE_ constants to match header

* Remove defunct comment

* Add failing test

* Remove event log noise

* Make test pass

* more apos

* spacing

* feedback

* Avoid overwriting last error

* feedback

Author: danmoseley

src/libraries/Common/src/Interop/Windows/Advapi32/Interop.ServiceProcessOptions.cs

@@ -65,25 +65,24 @@ internal static partial class ServiceOptions
 
         internal static partial class ServiceTypeOptions
         {
-            internal const int SERVICE_TYPE_ADAPTER = 0x00000004;
-            internal const int SERVICE_TYPE_FILE_SYSTEM_DRIVER = 0x00000002;
-            internal const int SERVICE_TYPE_INTERACTIVE_PROCESS = 0x00000100;
-            internal const int SERVICE_TYPE_KERNEL_DRIVER = 0x00000001;
-            internal const int SERVICE_TYPE_RECOGNIZER_DRIVER = 0x00000008;
-            internal const int SERVICE_TYPE_WIN32_OWN_PROCESS = 0x00000010;
-            internal const int SERVICE_TYPE_WIN32_SHARE_PROCESS = 0x00000020;
-            internal const int SERVICE_TYPE_WIN32 =
-                SERVICE_TYPE_WIN32_OWN_PROCESS |
-                SERVICE_TYPE_WIN32_SHARE_PROCESS;
-            internal const int SERVICE_TYPE_DRIVER =
-                SERVICE_TYPE_KERNEL_DRIVER |
-                SERVICE_TYPE_FILE_SYSTEM_DRIVER |
-                SERVICE_TYPE_RECOGNIZER_DRIVER;
-            internal const int SERVICE_TYPE_ALL =
-                SERVICE_TYPE_WIN32 |
-                SERVICE_TYPE_ADAPTER |
-                SERVICE_TYPE_DRIVER |
-                SERVICE_TYPE_INTERACTIVE_PROCESS;
+            internal const int SERVICE_KERNEL_DRIVER = 0x00000001;
+            internal const int SERVICE_FILE_SYSTEM_DRIVER = 0x00000002;
+            internal const int SERVICE_ADAPTER = 0x00000004;
+            internal const int SERVICE_RECOGNIZER_DRIVER = 0x00000008;
+
+            internal const int SERVICE_DRIVER =
+                SERVICE_KERNEL_DRIVER |
+                SERVICE_FILE_SYSTEM_DRIVER |
+                SERVICE_RECOGNIZER_DRIVER;
+
+            internal const int SERVICE_WIN32_OWN_PROCESS = 0x00000010;
+            internal const int SERVICE_WIN32_SHARE_PROCESS = 0x00000020;
+
+            internal const int SERVICE_WIN32 =
+                SERVICE_WIN32_OWN_PROCESS |
+                SERVICE_WIN32_SHARE_PROCESS;
+
+            internal const int SERVICE_INTERACTIVE_PROCESS = 0x00000100;
         }
 
         internal static partial class ServiceAccessOptions

src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs

@@ -577,22 +577,8 @@ private unsafe void DeferredShutdown()
 
         /// <summary>
         /// <para>When implemented in a derived class, <see cref='System.ServiceProcess.ServiceBase.OnCustomCommand'/>
-        /// executes when a custom command is passed to
-        /// the service. Specifies the actions to take when
+        /// executes when a custom command is passed to the service. Specifies the actions to take when
         /// a command with the specified parameter value occurs.</para>
-        /// <note type="rnotes">
-        ///    Previously had "Passed to the
-        ///    service by
-        ///    the SCM", but the SCM doesn't pass custom commands. Do we want to indicate an
-        ///    agent here? Would it be the ServiceController, or is there another way to pass
-        ///    the int into the service? I thought that the SCM did pass it in, but
-        ///    otherwise ignored it since it was an int it doesn't recognize. I was under the
-        ///    impression that the difference was that the SCM didn't have default processing, so
-        ///    it transmitted it without examining it or trying to performs its own
-        ///    default behavior on it. Please correct where my understanding is wrong in the
-        ///    second paragraph below--what, if any, contact does the SCM have with a
-        ///    custom command?
-        /// </note>
         /// </summary>
         protected virtual void OnCustomCommand(int command)
         {
@@ -698,11 +684,11 @@ private void Initialize(bool multipleServices)
 
                 if (!multipleServices)
                 {
-                    _status.serviceType = ServiceTypeOptions.SERVICE_TYPE_WIN32_OWN_PROCESS;
+                    _status.serviceType = ServiceTypeOptions.SERVICE_WIN32_OWN_PROCESS;
                 }
                 else
                 {
-                    _status.serviceType = ServiceTypeOptions.SERVICE_TYPE_WIN32_SHARE_PROCESS;
+                    _status.serviceType = ServiceTypeOptions.SERVICE_WIN32_SHARE_PROCESS;
                 }
 
                 _status.currentState = ServiceControlStatus.STATE_START_PENDING;
@@ -885,10 +871,15 @@ public unsafe void ServiceMainCallback(int argCount, IntPtr argPointer)
 
                 if (argCount > 0)
                 {
-                    char** argsAsPtr = (char**)argPointer.ToPointer();
+                    char** argsAsPtr = (char**)argPointer;
+
+                    // The first arg is always the service name. We don't want to pass that in,
+                    // but we can use it to set the service name on ourselves if we don't already know it.
+                    if (string.IsNullOrEmpty(_serviceName))
+                    {
+                         _serviceName = Marshal.PtrToStringUni((IntPtr)(*argsAsPtr))!;
+                    }
 
-                    //Lets read the arguments
-                    // the first arg is always the service name. We don't want to pass that in.
                     args = new string[argCount - 1];
 
                     for (int index = 0; index < args.Length; ++index)
@@ -954,7 +945,8 @@ public unsafe void ServiceMainCallback(int argCount, IntPtr argPointer)
                 statusOK = SetServiceStatus(_statusHandle, pStatus);
                 if (!statusOK)
                 {
-                    WriteLogEntry(SR.Format(SR.StartFailed, new Win32Exception().Message), EventLogEntryType.Error);
+                    string errorMessage = new Win32Exception().Message;
+                    WriteLogEntry(SR.Format(SR.StartFailed, errorMessage), EventLogEntryType.Error);
                     _status.currentState = ServiceControlStatus.STATE_STOPPED;
                     SetServiceStatus(_statusHandle, pStatus);
                 }

src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

@@ -19,6 +19,11 @@ public class ServiceController : Component
         private string _machineName; // Never null
         private const string DefaultMachineName = ".";
 
+        // Note that ServiceType currently does not include all of SERVICE_TYPE_ALL; see see Interop.Advapi32.ServiceTypeOptions
+        private const int AllServiceTypes = (int)(ServiceType.Adapter | ServiceType.FileSystemDriver | ServiceType.InteractiveProcess |
+                                                  ServiceType.KernelDriver | ServiceType.RecognizerDriver | ServiceType.Win32OwnProcess |
+                                                  ServiceType.Win32ShareProcess);
+
         private string? _name;
         private string? _eitherName;
         private string? _displayName;
@@ -37,7 +42,7 @@ public class ServiceController : Component
         public ServiceController()
         {
             _machineName = DefaultMachineName;
-            _type = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ALL;
+            _type = AllServiceTypes;
         }
 
         /// <summary>
@@ -65,7 +70,7 @@ public ServiceController(string name, string machineName)
 
             _machineName = machineName;
             _eitherName = name;
-            _type = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ALL;
+            _type = AllServiceTypes;
         }
 
         private ServiceController(string machineName, Interop.Advapi32.ENUM_SERVICE_STATUS status)
@@ -310,7 +315,7 @@ public unsafe ServiceController[] ServicesDependedOn
                 {
                     success = Interop.Advapi32.QueryServiceConfig(serviceHandle, bufPtr, bytesNeeded, out bytesNeeded);
                     if (!success)
-                        throw new Win32Exception(Marshal.GetLastWin32Error());
+                        throw new Win32Exception();
 
                     Interop.Advapi32.QUERY_SERVICE_CONFIG config = new Interop.Advapi32.QUERY_SERVICE_CONFIG();
                     Marshal.PtrToStructure(bufPtr, config);
@@ -389,7 +394,7 @@ public ServiceStartMode StartType
                 {
                     success = Interop.Advapi32.QueryServiceConfig(serviceHandle, bufPtr, bytesNeeded, out bytesNeeded);
                     if (!success)
-                        throw new Win32Exception(Marshal.GetLastWin32Error());
+                        throw new Win32Exception();
 
                     Interop.Advapi32.QUERY_SERVICE_CONFIG config = new Interop.Advapi32.QUERY_SERVICE_CONFIG();
                     Marshal.PtrToStructure(bufPtr, config);
@@ -467,7 +472,7 @@ public void Close()
 
             _statusGenerated = false;
             _startTypeInitialized = false;
-            _type = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ALL;
+            _type = AllServiceTypes;
         }
 
         /// <summary>
@@ -491,7 +496,7 @@ private unsafe void GenerateStatus()
             Interop.Advapi32.SERVICE_STATUS svcStatus = default;
             bool success = Interop.Advapi32.QueryServiceStatus(serviceHandle, &svcStatus);
             if (!success)
-                throw new Win32Exception(Marshal.GetLastWin32Error());
+                throw new Win32Exception();
 
             _commandsAccepted = svcStatus.controlsAccepted;
             _status = (ServiceControllerStatus)svcStatus.currentState;
@@ -632,7 +637,7 @@ private static SafeServiceHandle GetDataBaseHandleWithAccess(string machineName,
 
             if (databaseHandle.IsInvalid)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.OpenSC, machineName), inner);
             }
 
@@ -669,7 +674,7 @@ public static ServiceController[] GetDevices()
         /// <returns>Set of service controllers</returns>
         public static ServiceController[] GetDevices(string machineName)
         {
-            return GetServicesOfType(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_DRIVER);
+            return GetServicesOfType(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_DRIVER);
         }
 
         /// <summary>
@@ -684,7 +689,7 @@ private SafeServiceHandle GetServiceHandle(int desiredAccess)
             var serviceHandle = new SafeServiceHandle(Interop.Advapi32.OpenService(_serviceManagerHandle, ServiceName, desiredAccess));
             if (serviceHandle.IsInvalid)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.OpenService, ServiceName, _machineName), inner);
             }
 
@@ -707,7 +712,7 @@ public static ServiceController[] GetServices()
         /// <returns></returns>
         public static ServiceController[] GetServices(string machineName)
         {
-            return GetServicesOfType(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32);
+            return GetServicesOfType(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_WIN32);
         }
 
         /// <summary>
@@ -718,7 +723,7 @@ public static ServiceController[] GetServices(string machineName)
         /// <returns></returns>
         private static Interop.Advapi32.ENUM_SERVICE_STATUS_PROCESS[] GetServicesInGroup(string machineName, string group)
         {
-            return GetServices(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32, group, status => status);
+            return GetServices(machineName, Interop.Advapi32.ServiceTypeOptions.SERVICE_WIN32, group, status => status);
         }
 
         /// <summary>
@@ -804,7 +809,7 @@ public unsafe void Pause()
 
             if (!result)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.PauseService, ServiceName, _machineName), inner);
             }
         }
@@ -819,7 +824,7 @@ public unsafe void Continue()
             bool result = Interop.Advapi32.ControlService(serviceHandle, Interop.Advapi32.ControlOptions.CONTROL_CONTINUE, &status);
             if (!result)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.ResumeService, ServiceName, _machineName), inner);
             }
         }
@@ -835,7 +840,7 @@ public unsafe void ExecuteCommand(int command)
             bool result = Interop.Advapi32.ControlService(serviceHandle, command, &status);
             if (!result)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.ControlService, ServiceName, MachineName), inner);
             }
         }
@@ -891,7 +896,7 @@ public void Start(string[] args!!)
                 bool result = Interop.Advapi32.StartService(serviceHandle, args.Length, argPtrsHandle.AddrOfPinnedObject());
                 if (!result)
                 {
-                    Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                    Exception inner = new Win32Exception();
                     throw new InvalidOperationException(SR.Format(SR.CannotStart, ServiceName, _machineName), inner);
                 }
             }
@@ -952,7 +957,7 @@ unsafe void Stop(bool stopDependentServices)
             bool result = Interop.Advapi32.ControlService(serviceHandle, Interop.Advapi32.ControlOptions.CONTROL_STOP, &status);
             if (!result)
             {
-                Exception inner = new Win32Exception(Marshal.GetLastWin32Error());
+                Exception inner = new Win32Exception();
                 throw new InvalidOperationException(SR.Format(SR.StopService, ServiceName, _machineName), inner);
             }
         }

src/libraries/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceType.cs

@@ -6,12 +6,13 @@ namespace System.ServiceProcess
     [Flags]
     public enum ServiceType
     {
-        Adapter = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_ADAPTER,
-        FileSystemDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_FILE_SYSTEM_DRIVER,
-        InteractiveProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_INTERACTIVE_PROCESS,
-        KernelDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_KERNEL_DRIVER,
-        RecognizerDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_RECOGNIZER_DRIVER,
-        Win32OwnProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32_OWN_PROCESS,
-        Win32ShareProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_TYPE_WIN32_SHARE_PROCESS
+        // Does not currently represent all of the Win32 values; see Interop.Advapi32.ServiceTypeOptions
+        Adapter = Interop.Advapi32.ServiceTypeOptions.SERVICE_ADAPTER,
+        FileSystemDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_FILE_SYSTEM_DRIVER,
+        InteractiveProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_INTERACTIVE_PROCESS,
+        KernelDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_KERNEL_DRIVER,
+        RecognizerDriver = Interop.Advapi32.ServiceTypeOptions.SERVICE_RECOGNIZER_DRIVER,
+        Win32OwnProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_WIN32_OWN_PROCESS,
+        Win32ShareProcess = Interop.Advapi32.ServiceTypeOptions.SERVICE_WIN32_SHARE_PROCESS
     }
 }

src/libraries/System.ServiceProcess.ServiceController/tests/SafeServiceControllerTests.cs

@@ -81,12 +81,12 @@ public static void GetDevices()
             ServiceController[] devices = ServiceController.GetDevices();
             Assert.True(devices.Length != 0);
 
-            const ServiceType SERVICE_TYPE_DRIVER =
+            const ServiceType SERVICE_DRIVER =
                 ServiceType.FileSystemDriver |
                 ServiceType.KernelDriver |
                 ServiceType.RecognizerDriver;
 
-            Assert.All(devices, device => Assert.NotEqual(0, (int)(device.ServiceType & SERVICE_TYPE_DRIVER)));
+            Assert.All(devices, device => Assert.NotEqual(0, (int)(device.ServiceType & SERVICE_DRIVER)));
         }
 
         [Fact]

src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs

@@ -223,6 +223,21 @@ public void PropagateExceptionFromOnStart()
             testService.DeleteTestServices();
         }
 
+        [ConditionalFact(nameof(IsElevatedAndSupportsEventLogs))]
+        public void NoServiceNameOnServiceBase()
+        {
+            // When installing a service, you must supply a non empty name.
+            // When a service starts itself (using StartServiceCtrlDispatcher) it's legal to pass an empty string for the name.
+            string serviceName = "NoServiceNameOnServiceBase";
+            var testService = new TestServiceProvider(serviceName);
+
+            // Ensure it has successfully written to the event log,
+            // indicating it figured out its own name.
+            Assert.True(EventLog.SourceExists(serviceName));
+
+            testService.DeleteTestServices();
+        }
+
         private ServiceController ConnectToServer()
         {
             TestServiceProvider.DebugTrace("ServiceBaseTests.ConnectToServer: connecting");

src/libraries/System.ServiceProcess.ServiceController/tests/System.ServiceProcess.ServiceController.TestService/Program.cs

@@ -65,7 +65,7 @@ static int Main(string[] args)
                 }
                 else
                 {
-                    Console.WriteLine("EROOR: Invalid Service verb. Only suppot create or delete.");
+                    Console.WriteLine("EROOR: Invalid Service verb. Only support create or delete.");
                     return 2;
                 }
             }

src/libraries/System.ServiceProcess.ServiceController/tests/System.ServiceProcess.ServiceController.TestService/TestService.cs

@@ -22,7 +22,11 @@ public class TestService : ServiceBase
         public TestService(string serviceName, Exception throwException = null)
         {
             DebugTrace("TestService " + ServiceName + ": Ctor");
-            this.ServiceName = serviceName;
+
+            if (serviceName != "NoServiceNameOnServiceBase")
+            {
+                this.ServiceName = serviceName;
+            }
 
             // Enable all the events
             this.CanPauseAndContinue = true;
@@ -101,7 +105,15 @@ protected override void OnStop()
             base.OnStop();
             // We may be  stopping because the test has completed and we're cleaning up the test service so there's no client at all, so don't waitForConnect.
             // Tests that verify "Stop" itself should ensure the client connection has completed before calling stop, by waiting on some other message from the pipe first.
-            WriteStreamAsync(PipeMessageByteCode.Stop, waitForConnect:false).Wait();
+            try
+            {
+                WriteStreamAsync(PipeMessageByteCode.Stop, waitForConnect:false).Wait();
+            }
+            catch (AggregateException ae) when (ae.InnerException.GetType() == typeof(InvalidOperationException))
+            {
+                // Some tests don't bother to connect to the pipe, and just stop the service to clean up.
+                // Don't log this exception into the event log.
+            }
         }
 
         public async Task WriteStreamAsync(PipeMessageByteCode code, int command = 0, bool waitForConnect = true)