[PATCH] programs/sc: Allow using separate arguments for parameter name and value.

[Torge Matthies]

In addition to using one argument for both parameter name and value.

This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.

Signed-off-by: Torge Matthies 
---
 programs/sc/sc.c | 101 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 32 deletions(-)

diff --git a/programs/sc/sc.c b/programs/sc/sc.c
index 43148ab99d60..5caffdbe7fe4 100644
--- a/programs/sc/sc.c
+++ b/programs/sc/sc.c
@@ -27,6 +27,26 @@

WINE_DEFAULT_DEBUG_CHANNEL(sc);

+static BOOL parse_string_param( int argc, const WCHAR *argv[], unsigned int *index,
+                                const WCHAR *param_name, size_t name_len, const WCHAR **out )
+{
+    if (!wcsnicmp( argv[*index], param_name, name_len ))
+    {
+        if (argv[*index][name_len])
+        {
+            *out = &argv[*index][name_len];
+            return TRUE;
+        }
+        else if (*index < argc - 1)
+        {
+            *index += 1;
+            *out = argv[*index];
+            return TRUE;
+        }
+    }
+    return FALSE;
+}
+
 struct create_params
 {
     const WCHAR *displayname;
@@ -56,48 +76,56 @@ static BOOL parse_create_params( int argc, const WCHAR *argv[], struct create_pa
     cp->obj         = NULL;
     cp->password    = NULL;

+#define PARSE(x, y) parse_string_param( (argc), (argv), &(i), (x), ARRAY_SIZE(x) - 1, (y) )
     for (i = 0; i < argc; i++)
     {
-        if (!wcsnicmp( argv[i], L"displayname=", 12 )) cp->displayname = argv[i] + 12;
-        if (!wcsnicmp( argv[i], L"binpath=", 8 )) cp->binpath = argv[i] + 8;
-        if (!wcsnicmp( argv[i], L"group=", 6 )) cp->group = argv[i] + 6;
-        if (!wcsnicmp( argv[i], L"depend=", 7 )) cp->depend = argv[i] + 7;
-        if (!wcsnicmp( argv[i], L"obj=", 4 )) cp->obj = argv[i] + 4;
-        if (!wcsnicmp( argv[i], L"password=", 9 )) cp->password = argv[i] + 9;
-
-        if (!wcsnicmp( argv[i], L"tag=", 4 ))
+        const WCHAR *tag, *type, *start, *error;
+
+        if (PARSE( L"displayname=", &cp->displayname )) continue;
+        if (PARSE( L"binpath=", &cp->binpath )) continue;
+        if (PARSE( L"group=", &cp->group )) continue;
+        if (PARSE( L"depend=", &cp->depend )) continue;
+        if (PARSE( L"obj=", &cp->obj )) continue;
+        if (PARSE( L"password=", &cp->password )) continue;
+
+        if (PARSE( L"tag=", &tag ))
         {
-            if (!wcsicmp( argv[i] + 4, L"yes" ))
+            if (!wcsicmp( tag, L"yes" ))
             {
                 WINE_FIXME("tag argument not supported\n");
                 cp->tag = TRUE;
             }
+            continue;
         }
-        if (!wcsnicmp( argv[i], L"type=", 5 ))
+        if (PARSE( L"type=", &type ))
         {
-            if (!wcsicmp( argv[i] + 5, L"own" )) cp->type = SERVICE_WIN32_OWN_PROCESS;
-            if (!wcsicmp( argv[i] + 5, L"share" )) cp->type = SERVICE_WIN32_SHARE_PROCESS;
-            if (!wcsicmp( argv[i] + 5, L"kernel" )) cp->type = SERVICE_KERNEL_DRIVER;
-            if (!wcsicmp( argv[i] + 5, L"filesys" )) cp->type = SERVICE_FILE_SYSTEM_DRIVER;
-            if (!wcsicmp( argv[i] + 5, L"rec" )) cp->type = SERVICE_RECOGNIZER_DRIVER;
-            if (!wcsicmp( argv[i] + 5, L"interact" )) cp->type |= SERVICE_INTERACTIVE_PROCESS;
+            if (!wcsicmp( type, L"own" )) cp->type = SERVICE_WIN32_OWN_PROCESS;
+            else if (!wcsicmp( type, L"share" )) cp->type = SERVICE_WIN32_SHARE_PROCESS;
+            else if (!wcsicmp( type, L"kernel" )) cp->type = SERVICE_KERNEL_DRIVER;
+            else if (!wcsicmp( type, L"filesys" )) cp->type = SERVICE_FILE_SYSTEM_DRIVER;
+            else if (!wcsicmp( type, L"rec" )) cp->type = SERVICE_RECOGNIZER_DRIVER;
+            else if (!wcsicmp( type, L"interact" )) cp->type |= SERVICE_INTERACTIVE_PROCESS;
+            continue;
         }
-        if (!wcsnicmp( argv[i], L"start=", 6 ))
+        if (PARSE( L"start=", &start ))
         {
-            if (!wcsicmp( argv[i] + 6, L"boot" )) cp->start = SERVICE_BOOT_START;
-            if (!wcsicmp( argv[i] + 6, L"system" )) cp->start = SERVICE_SYSTEM_START;
-            if (!wcsicmp( argv[i] + 6, L"auto" )) cp->start = SERVICE_AUTO_START;
-            if (!wcsicmp( argv[i] + 6, L"demand" )) cp->start = SERVICE_DEMAND_START;
-            if (!wcsicmp( argv[i] + 6, L"disabled" )) cp->start = SERVICE_DISABLED;
+            if (!wcsicmp( start, L"boot" )) cp->start = SERVICE_BOOT_START;
+            else if (!wcsicmp( start, L"system" )) cp->start = SERVICE_SYSTEM_START;
+            else if (!wcsicmp( start, L"auto" )) cp->start = SERVICE_AUTO_START;
+            else if (!wcsicmp( start, L"demand" )) cp->start = SERVICE_DEMAND_START;
+            else if (!wcsicmp( start, L"disabled" )) cp->start = SERVICE_DISABLED;
+            continue;
         }
-        if (!wcsnicmp( argv[i], L"error=", 6 ))
+        if (PARSE( L"error=", &error ))
         {
-            if (!wcsicmp( argv[i] + 6, L"normal" )) cp->error = SERVICE_ERROR_NORMAL;
-            if (!wcsicmp( argv[i] + 6, L"severe" )) cp->error = SERVICE_ERROR_SEVERE;
-            if (!wcsicmp( argv[i] + 6, L"critical" )) cp->error = SERVICE_ERROR_CRITICAL;
-            if (!wcsicmp( argv[i] + 6, L"ignore" )) cp->error = SERVICE_ERROR_IGNORE;
+            if (!wcsicmp( error, L"normal" )) cp->error = SERVICE_ERROR_NORMAL;
+            else if (!wcsicmp( error, L"severe" )) cp->error = SERVICE_ERROR_SEVERE;
+            else if (!wcsicmp( error, L"critical" )) cp->error = SERVICE_ERROR_CRITICAL;
+            else if (!wcsicmp( error, L"ignore" )) cp->error = SERVICE_ERROR_IGNORE;
+            continue;
         }
     }
+#undef PARSE
     if (!cp->binpath) return FALSE;
     return TRUE;
 }
@@ -156,16 +184,25 @@ static BOOL parse_failure_params( int argc, const WCHAR *argv[], SERVICE_FAILURE
     fa->cActions      = 0;
     fa->lpsaActions   = NULL;

+#define PARSE(x, y) parse_string_param( (argc), (argv), &(i), (x), ARRAY_SIZE(x) - 1, (y) )
     for (i = 0; i < argc; i++)
     {
-        if (!wcsnicmp( argv[i], L"reset=", 6 )) fa->dwResetPeriod = wcstol( argv[i] + 6, NULL, 10 );
-        if (!wcsnicmp( argv[i], L"reboot=", 7 )) fa->lpRebootMsg = (WCHAR *)argv[i] + 7;
-        if (!wcsnicmp( argv[i], L"command=", 8 )) fa->lpCommand = (WCHAR *)argv[i] + 8;
-        if (!wcsnicmp( argv[i], L"actions=", 8 ))
+        const WCHAR *reset, *actions;
+
+        if (PARSE( L"reset=", &reset ))
+        {
+            fa->dwResetPeriod = wcstol( reset, NULL, 10 );
+            continue;
+        }
+        if (PARSE( L"reboot=", &fa->lpRebootMsg )) continue;
+        if (PARSE( L"command=", &fa->lpCommand )) continue;
+        if (PARSE( L"actions=", &actions ))
         {
-            if (!parse_failure_actions( argv[i] + 8, fa )) return FALSE;
+            if (!parse_failure_actions( actions, fa )) return FALSE;
+            continue;
         }
     }
+#undef PARSE
     return TRUE;
 }

[Dmitry Timoshkov]

Torge Matthies  wrote:
> In addition to using one argument for both parameter name and value.
>
> This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.

Could you please clarify what precisely is the regression caused by the
mentioned above commit, and provide some examples which command lines
were broken and started to work with your patch.

[Torge Matthies]

On 4/25/22 20:17, Dmitry Timoshkov wrote:
> Torge Matthies  wrote:
> > In addition to using one argument for both parameter name and value.
> >
> > This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.
>
> Could you please clarify what precisely is the regression caused by the
> mentioned above commit, and provide some examples which command lines
> were broken and started to work with your patch.

The mentioned commit broke commands like:

sc.exe create test binpath= C:\windows\system32\cmd.exe

and:

sc.exe create test binpath= C:\windows\system32\cmd.exe 
displayname= "Test Service" start= auto

All sc create or sc failure commands where the parameter names and
values are separated by a space are affected.

Should I add the commands above to the commit message?

[Alexandre Julliard]

Torge Matthies  writes:
> On 4/25/22 20:17, Dmitry Timoshkov wrote:
> > Torge Matthies  wrote:
> > > In addition to using one argument for both parameter name and value.
> > >
> > > This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.
> >
> > Could you please clarify what precisely is the regression caused by
> > the
> > mentioned above commit, and provide some examples which command lines
> > were broken and started to work with your patch.
>
> The mentioned commit broke commands like:
> sc.exe create test binpath= C:\windows\system32\cmd.exe
>
> and:
> sc.exe create test binpath= C:\windows\system32\cmd.exe
> displayname= "Test Service" start= auto
>
> All sc create or sc failure commands where the parameter names and
> values are separated by a space are affected.
>
> Should I add the commands above to the commit message?

If possible, adding some test cases would be preferable.

[Dmitry Timoshkov]

Torge Matthies  wrote:
> On 4/25/22 20:17, Dmitry Timoshkov wrote:
> > Torge Matthies  wrote:
> > > In addition to using one argument for both parameter name and value.
> > >
> > > This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.
> >
> > Could you please clarify what precisely is the regression caused by the
> > mentioned above commit, and provide some examples which command lines
> > were broken and started to work with your patch.
>
> The mentioned commit broke commands like:
>  sc.exe create test binpath= C:\windows\system32\cmd.exe
>
> and:
>  sc.exe create test binpath= C:\windows\system32\cmd.exe 
>
> displayname= "Test Service" start= auto
>
> All sc create or sc failure commands where the parameter names and
> values are separated by a space are affected.

Thanks, it looks like sc.exe from Windows 10 does support providing
an argument as a separate parameter, and it's even reflected in the help
message (parameter name includes equal sign (=)) when 'binpath = path' is
passed on the command line.

Just curious, is your patch supposed to fix some real use case?

[Torge Matthies]

On 4/25/22 20:52, Dmitry Timoshkov wrote:
> Torge Matthies  wrote:
> > On 4/25/22 20:17, Dmitry Timoshkov wrote:
> > > Torge Matthies  wrote:
> > > > In addition to using one argument for both parameter name and value.
> > > >
> > > > This fixes a regression from commit 8b38c91d83844dea882922055ced7cdeb79c1693.
> > >
> > > Could you please clarify what precisely is the regression caused by the
> > > mentioned above commit, and provide some examples which command lines
> > > were broken and started to work with your patch.
> >
> > The mentioned commit broke commands like:
> >   sc.exe create test binpath= C:\windows\system32\cmd.exe
> >
> > and:
> >   sc.exe create test binpath= C:\windows\system32\cmd.exe
> >
> > displayname= "Test Service" start= auto
> >
> > All sc create or sc failure commands where the parameter names and
> > values are separated by a space are affected.
>
> Thanks, it looks like sc.exe from Windows 10 does support providing
> an argument as a separate parameter, and it's even reflected in the help
> message (parameter name includes equal sign (=)) when 'binpath = path' is
> passed on the command line.
>
> Just curious, is your patch supposed to fix some real use case?

It's supposed to fix service creation with sc.exe in general. The syntax
with the spaces has been the default forever and I didn't even know that
the other syntax was accepted.

I have a script that installs an exe as a service and that got broken.
But I'm sure there are many more things that depend on sc.exe accepting
the syntax with spaces between parameter name and value.