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.