On Windows, using the -o or -O options or the s command causes less to immediately terminate.

[thegavaguy]

Log command causes crash for some Windows builds of less

---

TL;DR

Using the -o or -O options or the s command with some Windows builds of
less results in less immediately terminating back to the command line.

The affected builds are those which link with Microsoft's Universal C Runtime
library (UCRT). It makes no difference which C compiler is used. It results
from the UCRT performing stringent argument validation and treating all
runtime assertion failures as fatal errors by default. The invalid argument
behind the failure is the pmode value used in the creat() call on line 980
of edit.c.

If you are interested, please read the rest of this issue for the details about
what I observed and how I determined the underlying cause.

---

[ Before I get into details, I want to say that none of the difficulties
underlying this issue are the result of the procedure used to produce the
prebuilt binaries. GitHub user jftuga is generous enough to set up
automatic builds using Visual Studio and the makefile supplied by the less
project. Anyone with the necessary buildtime tools would obtain the same
results. So, many thanks to jftuga for bringing less to the many Windows
users who do not have program development tools installed and perhaps do not
have a background in software development. ]

I was using less to debug intermediate stages of a complex pipeline. Having
located a possible culprit I wanted to save the result for additional analysis.
I entered s (save input to a file), entered a logfile path, and less just
terminated back to the command prompt. I was using the prebuilt Windows binary of
version 633,
obtained from the link provided on the download page of the Greenwood Software
website.

Since there was no error message, pop-up message, or any other indication of
what had happened I tried it again. The same thing happened. Then I checked
the Windows Event Viewer and found this:

Faulting application name: less.exe, version: 0.0.0.0, time stamp: 0x645308fd
Faulting module name: less.exe, version: 0.0.0.0, time stamp: 0x645308fd
Exception code: 0xc0000409
Fault offset: 0x000000000002f994
Faulting process id: 0x0x1BE8
Faulting application start time: 0x0x1D9B807CD4A2551
Faulting application path: C:\usr\local\bin\less.exe
Faulting module path: C:\usr\local\bin\less.exe
Report Id: d74918ae-0f3d-4e8d-ba43-c33918fca457

The exception code of 0xC0000409 equates to STATUS_STACK_BUFFER_OVERRUN in
winnt.h but is also known as STATUS_FAIL_FAST_EXCEPTION according to some
Microsoft web pages. A Fail Fast Exception bypasses all exception handlers,
terminates the application and invokes Windows Error Reporting, thus explaining
the entry in the Event Log. Apparently, it was originally intended for Guard
Segment violations, indicating a stack overwrite corruption which potentially
leaves an application in an unstable state and thus not to be trusted to carry
out normal exception handling. The application is terminated as fast as
possible. More recently, the various argument validation macros invoke
_invalid_parameter_* or _invoke_watson functions which in turn invoke
the __fastfail() intrinsic with an argument of FAST_FAIL_INVALID_ARG
when an assertion failure is detected.

To investigate further I used Visual Studio 2022 17.6.5 and Makefile.win-vc64 to
build a 64-bit debug version of less. Rerunning my test case I get the following
pop-up message saying:

(Note: The file above shows the path on the systems used by Microsoft to build
their Universal C Runtime (UCRT) DLLs, not a path on my system.)

Looking at the call stack in the debugger, the problem started at edit.c:980.

976     case 'O': case 'o':
977         /*
978          * Overwrite: create the file.
979          */
980         logfile = creat(filename, 0644);
981         break;

According to Microsoft's documentation for
creat(),
as implemented in their Universal C Runtime (UCRT) library, it will only accept
values of S_IREAD and/or S_IWRITE for the pmode parameter. When other bits are set
it produces the above runtime assertion failure. Depending on the build type,
release or debug, such an assertion failure logs an event and terminates the
application or produces the above pop-up message, giving the user a choice of
actions. In short, the UCRT uses VALIDATE macros to more thoroughly validate
arguments than earlier Microsoft C runtime libraries and it treats VALIDATE failures
as fatal errors by default.

In the less source file edit.c, line 980, creat() is called with a pmode value
of 0644: read and write for owner, just read for group and others. This is a
common value for Unix-like systems but it is now causing problems for some Windows
builds of less. It turns out that 0600 corresponds to S_IREAD | S_IWRITE
on Windows (the constants are declared in <sys\stat.h>) but the other READ bits
cause the current problem.

Given that the content of edit.c:980 goes back to October 15, 1994:

$ git log -S'logfile = creat' -- edit.c
commit fe6f798541833aac6d69e6568debc4bcf474312c (tag: v243)
Author: Mark Nudelman <markn@greenwoodsoftware.com>
Date:   Sat Oct 15 07:10:53 1994 +0000

    Initial revision

and initial less support for Windows was added in 1996 I was curious about the
status of the logging feature over the years. I did have some older prebuilt
Windows binaries of less on hand but it seemed to be a better idea to check the
currently available ones.

If anyone cares to play along at home, open a Command Prompt session, grab one of
the available prebuilt binaries (the newer ones are in .ZIP files) and issue a
command which pipes into less such as:

C:\Users\user name> dir | less

then enter s. You will be prompted log file: . Enter a name and press
Enter. If less stays open displaying the directory listing then it worked.
If it terminates back to the command prompt it failed.

The oldest currently available, prebuilt binary is v560 of less. It works.
The next oldest is v561.1. It fails. All the newer ones also fail.

Since the call to creat() has not changed, the cause had to be elsewhere.
I used dumpbin /imports less.exe to check the runtime dependencies. Version
560 is built with a MinGW64 compiler dynamically linked to msvcrt.dll,
Microsoft's C runtime library prior to the UCRT. Since it worked it must be the
case that it is not so picky about validating arguments. Version 561.1 is built
with whatever release of Visual Studio was current at the time and is
dynamically linked to the UCRT.

To be certain that the C runtime was the source of the problem I created a test
program to call creat() and built it with a variety of toolchains. Besides
Visual Studio 2022 17.6.5, I have also installed MSYS2 with full C toolchains
for its MSYS, MINGW64, UCRT64, and CLANG64 environments. Note
that the MSYS environment produces binaries dependent on msys-2.0.dll,
a lightly modified fork of the Cygwin POSIX emulation layer. It has its own C
runtime library and requires programs to adhere to POSIX conventions so I will
leave it out of these comparisons. I did write a version of my test program for
it, however, and it works just fine with a pmode value of 0644.

Again, if anyone wishes to play along at home, here is my test program.

#include <stdio.h>
#include <stdlib.h>
#include <io.h>
#include <sys\stat.h>
#include <string.h>

#define DIM(x)  (sizeof((x)) / sizeof((x)[0]))

int main(int argc, char *argv[])
{
    char *filename = "myTestFile.log";
    char *envvar = "TEMP";
    char pathname[_MAX_PATH];
    char *envval;
    char *sepstr;
    int fd;
    int ret;
    int len;

    if ((envval = getenv(envvar)) == NULL) {
        printf("Environment variable %s is not set.  Exiting.\n", envvar);
        return EXIT_FAILURE;
    }
    sepstr = (envval[strlen(envval) - 1] == '\\') ? "" : "\\";

    len = snprintf(pathname, DIM(pathname), "%s%s%s", envval, sepstr, filename);
    if (len >= DIM(pathname)) {
        printf("Temp file path is too long:\n"
               "%%%s%% = %s\n"
               "filename = %s\n"
               "Exiting.\n", envvar, envval, filename);
        return EXIT_FAILURE;
    }

    fd = _creat(pathname, 0644);
    if (fd != -1) {
        printf("Successfully created %s, fd = %d\n", pathname, fd);
        ret = _close(fd);
        if (ret != -1) {
            printf("Successfully closed %s\n", pathname);
        } else {
            printf("Unable to close %s.  errno = %d: %s\n", pathname, errno, strerror(errno) );
        }
        ret = _unlink(pathname);
        if (ret != -1) {
            printf("Successfully deleted %s\n", pathname);
        } else {
            printf("Unable to delete %s.  errno = %d: %s\n", pathname, errno, strerror(errno) );
        }
    } else {
        printf("Unable to create %s.  errno = %d: %s\n", pathname, errno, strerror(errno) );
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

To be generic it uses the TEMP environment variable to locate a suitable
place to create, and remove, a test file.

To build and run it with Visual Studio, the simplest way is to open an x64 Native Tools Command Prompt for VS2022 session, change to a scratch directory of your
choice, for example C:\tmp\scratch, copy the above code to the file creat.c
and issue the following commands:

C:\tmp\scratch> cl /Zi /MDd /Fecreat_ms_ucrtd.exe creat.c
C:\tmp\scratch> creat_ms_ucrtd

This produces a debug version dynamically linked to the debug UCRT DLL. I name
the executable file creat_ms_ucrtd.exe to signify use of the Microsoft C
compiler and a debug version of the UCRT. When run it will produce the pop-up
debug assertion failure dialog, as illustrated above.

Next, open a MINGW64 MSYS2 session. Use commands such as the following to build
the test program:

$ cd /c/tmp/scratch
$ gcc -g -o creat_gcc_msvcrtd creat.c
$ ./creat_gcc_msvcrtd

This version works as expected. Going back to the x64 Native Tools Command Prompt for VS2022 session, run dumpbin /imports creat_gcc_msvcrtd.exe | less to see that it imports msvcrt.dll. Use of the
-g option to gcc does not automatically select a debug version of the C
runtime library. On my Windows 11 system I cannot even find a debug version
of msvcrt.dll, although there are various versions in the WinSxS (side-by-side)
directory.

Similarly, open a UCRT64 MSYS2 session:

$cd /c/tmp/scratch
$ gcc -g -o creat_gcc_ucrtd creat.c
$ ./creat_gcc_ucrtd

Again, you can use dumpbin /imports which will show that it is dynamically linked to a
group of DLLs:

api-ms-win-crt-environment-l1-1-0.dll
api-ms-win-crt-filesystem-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-private-l1-1-0.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-string-l1-1-0.dll
api-ms-win-crt-time-l1-1-0.dll

These, in turn, forward CRT calls to the actual UCRT DLL, again not the debug
version. This version fails. It will display errno = 22: Invalid argument.

Finally, open a CLANG64 MSYS2 session:

$ cd /c/tmp/scratch
$ gcc -g -o creat_clang_ucrtd creat.c
$ ./creat_clang_ucrtd

Using dumpbin /imports will show the same DLLs, although in a different order.
This version also fails with the same output.

One question I have not yet resolved is why the version built with Microsoft C
either produces the pop-up assertion failure dialog or invokes __fast_fail()
to record an event and terminate the program while versions built with GCC or
CLANG allow the function to return a value indicating failure with an error
code left in errno.

So, it looks like only versions of less linked against the older C runtime
MSVCRT.DLL permit the call to creat() to have extra bits set in its pmode
argument. Any compiler linking to the current UCRT C runtime will produce a
program that fails if pmode has any extra bits set. I note that it makes no
difference whether you statically link to the UCRT or dynamically link. It
ultimately has the same runtime assertions and results.

So, what is the fix for this? Well, the simplest solution is to set up a
conditional compilation block to #define pmode to be (S_IREAD | S_IWRITE)
for Windows builds and 0644 for Unix-like builds. Before I make these edits
and submit them as a PR, I hope others with more experience dealing with
security and portability issues will be kind enough to comment on my proposed
approach.

But wait, there's more! I created a test build simply changing the value of
pmode as indicated above. I thought that would fix the problem, and so it
did, until I accidently entered an invalid path for the log file. There is an
error message from less on its status line:

Cannot write to "C:\build\less\myTestFile.log"  (press RETURN)

(It should be C:\builds\less\myTestFile.log.)

I press Enter and immediately get another pop-up message with a different
Debug Assertion Failed! dialog.

The function use_logfile() calls creat() in edit.c, line 980 which sets the
global variable logfile to a file descriptor value. If creat() fails it
returns -1. The end of use_logfile() checks for a negative value and
displays the error message when that is detected. The call to use_logfile() is
in optfunc.c, line 134:

132         namelogfile = shell_unquote(filename);
133         free(filename);
134         use_logfile(namelogfile);
135         sync_logfile();
136         break;

The function sync_logfile() begins in ch.c, line 401. The relevant section
is:

413         FOR_BUFS(bn)
414         {
415             bp = bufnode_buf(bn);
416             if (bp->block == block)
417             {
418                 write(logfile, (char *) bp->data, bp->datasize);
419                 wrote = TRUE;
420                 break;
421             }
422         }

Nowhere is logfile checked to be sure it is in a valid range. The call to
write() has an argument validation assertion failure as seen in the dialog
above. Instead of silently failing, note that the code above does not check the
return value of write(), the UCRT will terminate the program by default.
So, to correct this problem, the value of logfile needs to be checked
somewhere before calling write(). Whether or not the return value of
write() should be checked for other types of failures is a judgement call.

To wrap this up, changes to the strictness of argument validation in Microsoft's
UCRT mean that code which previously ran using MSVCRT (although there might be
unexpected or missing functionality for some C runtime calls) now will be
terminated by default, with an entry in the Event Log. The calls to creat()
and write() identified here will need to be updated to handle these changes.

I welcome any comments or criticisms along with suggestions.

[gwsw]

Thanks very much for this astonishingly complete and well-documented report. Can you confirm whether ef04926 fixes these issues for you?

[thegavaguy]

I can confirm that ef04926 does indeed fix both issues. The changes look pretty much exactly what I had in mind. Thanks for the incredibly quick turn-around. I will close out this issue.