Re: Crash in 'find_and_set_default_shell()'

2022-06-19 Thread Eli Zaretskii
> From: Paul Smith 
> Cc: gva...@online.no, bug-make@gnu.org
> Date: Sun, 19 Jun 2022 09:23:41 -0400
> 
> As best as I recall, the non-standard part of the old snprintf() was
> that it returned -1 if the buffer wasn't large enough, rather than the
> number of chars that would be needed.
> 
> The change made here doesn't rely on that behaviour.
> 
> However I realize now that I need to forcibly add a nul terminator
> because the old snprintf() on Windows didn't nul-terminate the string
> if the buffer wasn't large enough.
> 
> Maybe I'll just punt on that and simply allocate a large-enough buffer.
> 
> Were there other differences in old snprintf()?

The above two, plus the fact that it doesn't support the newer format
specifiers, like %z etc.



Re: Crash in 'find_and_set_default_shell()'

2022-06-19 Thread Paul Smith
On Sun, 2022-06-19 at 08:47 +0300, Eli Zaretskii wrote:
> > I do have a memory that some versions of Visual Studio, up until
> > relatively recently, have non-standard implementations of
> > snprintf()
> > but hopefully it's standard enough to manage this.
> 
> If we now rely on ANSI-standard behavior of snprintf in the Windows
> port, we need the MinGW build to use -D__USE_MINGW_ANSI_STDIO=1, to
> force replacement of the MS version of snprintf with MinGW's own
> reimplementation, which is ANSI-standard.  That's because linking
> Make against MSVCRT.DLL will use a non-compliant snprintf in that DLL
> (MinGW cannot link against proprietary C runtime libraries that come
> with later versions of Visual Studio).

As best as I recall, the non-standard part of the old snprintf() was
that it returned -1 if the buffer wasn't large enough, rather than the
number of chars that would be needed.

The change made here doesn't rely on that behaviour.

However I realize now that I need to forcibly add a nul terminator
because the old snprintf() on Windows didn't nul-terminate the string
if the buffer wasn't large enough.

Maybe I'll just punt on that and simply allocate a large-enough buffer.

Were there other differences in old snprintf()?



Re: Crash in 'find_and_set_default_shell()'

2022-06-18 Thread Eli Zaretskii
> From: Paul Smith 
> Date: Sat, 18 Jun 2022 17:05:44 -0400
> 
> On Wed, 2022-05-11 at 08:00 +0200, Gisle Vanem wrote:
> > The crash and wild call-stack seems to be caused
> > by an overflow to 'sprintf(sh_path, ..)'. But replacing
> > with 'snprintf()' works w/o any crash:
> 
> I applied changes based on this: some didn't need sprintf() at all; the
> rest I used snprintf().
> 
> I do have a memory that some versions of Visual Studio, up until
> relatively recently, have non-standard implementations of snprintf()
> but hopefully it's standard enough to manage this.

If we now rely on ANSI-standard behavior of snprintf in the Windows
port, we need the MinGW build to use -D__USE_MINGW_ANSI_STDIO=1, to
force replacement of the MS version of snprintf with MinGW's own
reimplementation, which is ANSI-standard.  That's because linking Make
against MSVCRT.DLL will use a non-compliant snprintf in that DLL
(MinGW cannot link against proprietary C runtime libraries that come
with later versions of Visual Studio).



Re: Crash in 'find_and_set_default_shell()'

2022-06-18 Thread Paul Smith
On Wed, 2022-05-11 at 08:00 +0200, Gisle Vanem wrote:
> The crash and wild call-stack seems to be caused
> by an overflow to 'sprintf(sh_path, ..)'. But replacing
> with 'snprintf()' works w/o any crash:

I applied changes based on this: some didn't need sprintf() at all; the
rest I used snprintf().

I do have a memory that some versions of Visual Studio, up until
relatively recently, have non-standard implementations of snprintf()
but hopefully it's standard enough to manage this.



Re: Crash in 'find_and_set_default_shell()'

2022-05-11 Thread Gisle Vanem

Paul Smith wrote:


... else (b) make has to parse this string
and break it up into words that it can use to call exec() without going
through a shell


The crash and wild call-stack seems to be caused
by an overflow to 'sprintf(sh_path, ..)'. But replacing
with 'snprintf()' works w/o any crash:

--- a/src/main.c 2022-05-10 13:37:02
+++ b/src/main.c 2022-05-11 07:47:07
@@ -956,7 +956,7 @@
 {
   batch_mode_shell = 1;
   unixy_shell = 0;
-  sprintf (sh_path, "%s", search_token);
+  snprintf (sh_path, GET_PATH_MAX, "%s", search_token);
   default_shell = xstrdup (w32ify (sh_path, 0));
   DB (DB_VERBOSE, (_("find_and_set_shell() setting default_shell = %s\n"),
default_shell));
@@ -971,7 +971,7 @@
   else if (_access (search_token, 0) == 0)
 {
   /* search token path was found */
-  sprintf (sh_path, "%s", search_token);
+  snprintf (sh_path, GET_PATH_MAX, "%s", search_token);
   default_shell = xstrdup (w32ify (sh_path, 0));
   DB (DB_VERBOSE, (_("find_and_set_shell() setting default_shell = %s\n"),
default_shell));
@@ -994,7 +994,7 @@
 {
   *ep = '\0';

-  sprintf (sh_path, "%s/%s", p, search_token);
+  snprintf (sh_path, GET_PATH_MAX, "%s/%s", p, search_token);
   if (_access (sh_path, 0) == 0)
 {
   default_shell = xstrdup (w32ify (sh_path, 0));
@@ -1016,7 +1016,7 @@
   /* be sure to check last element of Path */
   if (p && *p)
 {
-  sprintf (sh_path, "%s/%s", p, search_token);
+  snprintf (sh_path, GET_PATH_MAX, "%s/%s", p, search_token);
   if (_access (sh_path, 0) == 0)
 {
   default_shell = xstrdup (w32ify (sh_path, 0));

--

And testing it with:
  set GNUMAKEFLAGS=--debug=verbose,jobs
  gnumake -f Minimal.make

seems to work okay:
  ..
  find_and_set_shell() path search set default_shell = f:/CygWin32/bin/sh.exe
  ..
  CreateProcess(f:\CygWin32\bin\sh.exe,f:/CygWin32/bin/sh.exe -c "echo 
\"Hello\"",...)
  Putting child 012E9F10 (all) PID 20053024 on the chain.
  Live child 012E9F10 (all) PID 20053024
  Main thread handle = 0118
  Hello

--
--gv



Re: Crash in 'find_and_set_default_shell()'

2022-05-10 Thread Eli Zaretskii
> From: Paul Smith 
> Date: Tue, 10 May 2022 11:05:40 -0400
> 
> I will say that this does work as expected and doesn't throw an error
> with the latest GNU make Git version, on GNU/Linux.

On MS-Windows, we can barely _emulate_ Posix, so what might, by sheer
luck, work on GNU/Linux, regardless of its being invalid, can
legitimately crash on Windows, which actually assumes the value is
valid.



Re: Crash in 'find_and_set_default_shell()'

2022-05-10 Thread Paul Smith
On Tue, 2022-05-10 at 15:03 +0200, Gisle Vanem wrote:
> SHELL := env "PATH=$(PATH)" /bin/bash

Well, I dunno.  The problem is that at some point you have to choose
which command to use to invoke something.  The SHELL variable is
intended to contain a shell program that make will exec().  Here you're
saying that either (a) make has to invoke a shell which will invoke the
SHELL command (and how does it choose which shell?  It clearly can't
use the SHELL variable...), or else (b) make has to parse this string
and break it up into words that it can use to call exec() without going
through a shell: normally make leaves this sort of command parsing up
to the shell.

I will say that this does work as expected and doesn't throw an error
with the latest GNU make Git version, on GNU/Linux.