Skip to content

[Code Quality] ServiceManager.cs — Win32 access-right and service-type constants are re-declared shadowing the same names in NativeMethods.cs #867

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Info

File: src/Servy.Core/Services/ServiceManager.cs, lines 40-80

The file imports using static Servy.Core.Native.NativeMethods; at line 16, then re-declares several Win32 constants that already live in NativeMethods.cs:

// In NativeMethods.cs:
public const int SERVICE_QUERY_CONFIG = 0x0001;
public const int SERVICE_QUERY_STATUS = 0x0004;
public const int SERVICE_WIN32_OWN_PROCESS = 0x00000010;

// In ServiceManager.cs (shadowing — note also int → uint type drift):
public const uint SC_MANAGER_CONNECT          = 0x0001;
public const uint SC_MANAGER_CREATE_SERVICE   = 0x0002;
public const uint SERVICE_CHANGE_CONFIG       = 0x0002;
public const uint SERVICE_QUERY_STATUS        = 0x0004;   // duplicate of NativeMethods (int)
public const uint SERVICE_START               = 0x0010;
public const uint SERVICE_STOP                = 0x0020;
public const uint SERVICE_DELETE              = 0x00010000;
public const uint SERVICE_WIN32_OWN_PROCESS   = 0x00000010;   // duplicate of NativeMethods (int)
public const uint SERVICE_ERROR_NORMAL        = 0x00000001;
public const int  SERVICE_CONFIG_PRESHUTDOWN_INFO = 7;

Two effects:

  1. Shadowing — inside ServiceManager, references like SERVICE_QUERY_STATUS resolve to the local uint constant, not the int from NativeMethods. The compiler picks the closer scope. Anyone reading this and assuming "these all come from NativeMethods" can be surprised when an overload picks the uint overload of an API that doesn't exist in NativeMethods's int-based form.

  2. Type drift — most P/Invoke signatures in this codebase use int for these masks (see NativeMethods.cs). The uint clones in ServiceManager mean every callsite is doing an implicit int ↔ uint conversion. For values < 0x80000000 it's harmless, but SERVICE_DELETE = 0x00010000 is still positive in int so today nothing breaks. The risk is the next access-right (GENERIC_ALL = 0x10000000) or write-DAC (0x40000) being added with the same mismatch and a future flag bit going negative. This kind of drift is exactly what "single source of truth" prevents.

Suggested fix:

Move the missing constants (SC_MANAGER_*, SERVICE_CHANGE_CONFIG, SERVICE_START, SERVICE_STOP, SERVICE_DELETE, SERVICE_ERROR_NORMAL, SERVICE_CONFIG_PRESHUTDOWN_INFO) into NativeMethods.cs next to their siblings, in a single canonical type (typically int to match existing conventions). Delete the local copies in ServiceManager.cs. The using static Servy.Core.Native.NativeMethods; at line 16 already brings them into scope without qualification.

If access-rights need to be uint because they're passed to OpenService(... uint), change them all to uint once in NativeMethods.cs — picking one type is more important than which type.

Issue #865 covers the same pattern at the struct level (duplicate ServiceStatus/SERVICE_STATUS); this is the constants flavor of the same problem.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions