Re: cygrunsrv CWE-428

Hello Corinna,

You are probably right. I'm not a very experienced Windows developer. I believe MAX_PATH is a constant provided by Windows, right? So, I guess I was thinking that was a hard limit that shouldn't be exceeded. But since this is being written to a registry value, maybe it is okay to exceed that.

Is it worth trying to improve that patch and submit it somewhere? I was assuming something like this should really be handled by someone who knows what they are doing. But I'm happy to help if I can. The patch should probably also try to avoid buffer overflows.

Thanks,
James
________________________________________
From: Corinna Vinschen <[email protected]>
Sent: Tuesday, April 21, 2026 11:55 AM
To: James Warnock <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: cygrunsrv CWE-428
 
Hi James,

thanks for the patch.  One point, though:

On Apr 21 08:57, James Warnock via Cygwin wrote:
> diff --git a/cygrunsrv.cc b/cygrunsrv.cc
> index dab8790..c3b04ee 100644
> --- a/cygrunsrv.cc
> +++ b/cygrunsrv.cc
> @@ -810,6 +810,7 @@ install_service (const char *name, const char *crspath, const char *disp,
>                 int interactive)
>  {
>    char mypath[MAX_PATH];
> +  char* mypath_p = mypath;
>    SC_HANDLE sm = (SC_HANDLE) 0;
>    SC_HANDLE sh = (SC_HANDLE) 0;
>    char userbuf[INTERNET_MAX_HOST_NAME_LENGTH + UNLEN + 2];
> @@ -824,28 +825,31 @@ install_service (const char *name, const char *crspath, const char *disp,
>    if (!san.server ())
>      check_system_mounts ();
> 
> +  mypath[0] = '"';
> +  mypath_p++;
>    if (crspath)                 /* Got path, nothing to do. */
>      {
> -      cygwin_conv_path (CCP_POSIX_TO_WIN_A, crspath, mypath, MAX_PATH);
> +      cygwin_conv_path (CCP_POSIX_TO_WIN_A, crspath, mypath_p, MAX_PATH-2);
                                                                  ^^^^^^^^^^

>        if (strcasecmp (mypath + strlen (mypath) - 4, ".exe") != 0)
>          strcat (mypath, ".exe");
>      }
>    else if (san.server ()) /* Figure out cygrunsrv path on remote server. */
>      {
> -      DWORD ret, type, size = MAX_PATH - 20;
> +      DWORD ret, type, size = MAX_PATH - 22;
                                 ^^^^^^^^^^^^^

Rather than further restricting the maximum path length, wouldn't it
make sense to raise the size of mypath to MAX_PATH + 2?


Thanks,
Corinna

-- 
Problem reports:      https://cygwin.com/problems.html
FAQ:                  https://cygwin.com/faq/
Documentation:        https://cygwin.com/docs.html
Unsubscribe info:     https://cygwin.com/ml/#unsubscribe-simple