[Bug driver/107007] New: libiberty's win32_spawn error handling is poor

[Bug driver/107007] New: libiberty's win32_spawn error handling is poor

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107007

            Bug ID: 107007
           Summary: libiberty's win32_spawn error handling is poor
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: driver
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
  Target Milestone: ---
              Host: *-*-mingw*

As described in
https://gcc.gnu.org/pipermail/gcc-help/2022-September/141906.html the
pex_win32_exec_child function just assumes that all errors are ENOENT:

  /* Create the child process.  */  
  pid = win32_spawn (executable, (flags & PEX_SEARCH) != 0,
                     argv, env, dwCreationFlags, &si, &pi);
  if (pid == (pid_t) -1)
    pid = spawn_script (executable, argv, env, dwCreationFlags,
                        &si, &pi);
  if (pid == (pid_t) -1)
    {
      *err = ENOENT;
      *errmsg = "CreateProcess";
    }


This gives users incorrect information when something goes wrong for a reason
that isn't "No such file or directory".

Maybe win32_spawn should use the Win32 GetLastError() function to get a Windows
Error Code and then translate that to an errno value, and store it in errno. Or
ensure that GetLastError will still be valid for the caller of win32_spawn and
spawn_script to retrieve.

libstdc++-v3/src/c++11/system_error.cc already has code to do that translation
(so maybe it should move to libiberty, and then reuse that in libstdc++).

[Bug driver/107007] libiberty's win32_spawn error handling is poor

[costas.argyris at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107007

Costas Argyris <costas.argyris at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |costas.argyris at gmail dot com

--- Comment #1 from Costas Argyris <costas.argyris at gmail dot com> ---
A potential improvement in the win32_spawn function could be to check the
length of cmdline

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L625

before passing it to CreateProcess

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L630

because we know from

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa#parameters

that the second argument cannot be more than 32,767 characters.

So if we can check that before even making the call to CreateProcess, why
bother doing a call that we know will fail?

Perhaps the check could be done inside the argv_to_cmdline function itself.   
This function mentions the 32k limit in its comments

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L350

but doesn't actually consider it in the code.    Looks like the length has been
decided by the time we reach

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L385

in the cmdline_len variable, so it should be just a simple numerical check that
this value is less than or equal to 32,767 before doing the memory allocation. 
  If greater, it could just return NULL without allocating any memory and the
(single) caller at

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L625

already deals with NULL.