Re: Crash in 'find_and_set_default_shell()'
> 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()'
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()'
> 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()'
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()'
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()'
> 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()'
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.
Crash in 'find_and_set_default_shell()'
Hello list. Some resent (?) change has caused a crash in 'find_and_set_default_shell()' with this minimal Makefile: export PATH := $(PATH):node_modules/.bin SHELL := env "PATH=$(PATH)" /bin/bash all: @echo "Hello" --- Without the 'SHELL ..' line, all is good. Or even a 'SHELL=/bin/bash' works; no ':='. Not sure what Gnumake is trying to do here. The MSVC-2022 built version exits with a "Security check failure or stack buffer overrun - code c409" at 'gnumake!__report_gsfailure(void)+0x18' followed by a bogus call-stack; 185 lines of junk. I discovered this while trying to build this project: https://github.com/Financial-Times/n-fetch But I'm not sure if the above shell syntax should work on Windows or not. And I use 32-bit Cygwin as my POSIX toolbox. -- --gv
