[PATCH 0/1] MR6605: cmd: Call ReadConsoleW() only with standard input handles.

[PATCH 1/1] cmd: Call ReadConsoleW() only with standard input handles.

[Hans Leidekker]

From: Hans Leidekker

---
 programs/cmd/batch.c    | 20 ++++++++++----------
 programs/cmd/wcmdmain.c |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/programs/cmd/batch.c b/programs/cmd/batch.c
index 25281df15f0..288f96a2eff 100644
--- a/programs/cmd/batch.c
+++ b/programs/cmd/batch.c
@@ -222,7 +222,16 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h)
   /* We can't use the native f* functions because of the filename syntax differences
      between DOS and Unix. Also need to lose the LF (or CRLF) from the line. */

-  if (!ReadConsoleW(h, buf, noChars, &charsRead, NULL)) {
+  if (h == GetStdHandle(STD_INPUT_HANDLE) && ReadConsoleW(h, buf, noChars, &charsRead, NULL) && charsRead) {
+      if (!charsRead) return NULL;
+
+      /* Find first EOL */
+      for (i = 0; i < charsRead; i++) {
+          if (buf[i] == '\n' || buf[i] == '\r')
+              break;
+      }
+  }
+  else {
       LARGE_INTEGER filepos;
       char *bufA;
       UINT cp;
@@ -254,15 +263,6 @@ WCHAR *WCMD_fgets(WCHAR *buf, DWORD noChars, HANDLE h)
       i = MultiByteToWideChar(cp, 0, bufA, p - bufA, buf, noChars);
       free(bufA);
   }
-  else {
-      if (!charsRead) return NULL;
-
-      /* Find first EOL */
-      for (i = 0; i < charsRead; i++) {
-          if (buf[i] == '\n' || buf[i] == '\r')
-              break;
-      }
-  }

/* Truncate at EOL (or end of buffer) */
   if (i == noChars)
diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c
index 27c7def7900..4975c237dad 100644
--- a/programs/cmd/wcmdmain.c
+++ b/programs/cmd/wcmdmain.c
@@ -222,7 +222,7 @@ BOOL WCMD_ReadFile(const HANDLE hIn, WCHAR *intoBuf, const DWORD maxChars, LPDWO
     char *buffer;

/* Try to read from console as Unicode */
-    if (ReadConsoleW(hIn, intoBuf, maxChars, charsRead, NULL)) return TRUE;
+    if (hIn == GetStdHandle(STD_INPUT_HANDLE) && ReadConsoleW(hIn, intoBuf, maxChars, charsRead, NULL)) return TRUE;

/* We assume it's a file handle and read then convert from assumed OEM codepage */
     if (!(buffer = get_file_buffer()))

[eric pouech (＠epo)]

why do need this? (and since we don't control what's the standard input handle is, I'm not sure the test against std handle is what you may want)

[Hans Leidekker (＠hans)]

I have an application that hooks kernel functions and the ReadConsoleW() hook is not expected to be called (it's a stub). cmd.exe is passed a batch file on the command line (/C) so we're reading a regular file, not standard input.

[eric pouech (＠epo)]

In that case, likely GetFileType(h) == FILE_TYPE_CHAR would be a clearer discrimination to call into console APIs

[Hans Leidekker (＠hans)]

WCMD_fgets()/WCMD_ReadFile() are called with either a handle returned from CreateFileW() or GetStdHandle(STD_INPUT_HANDLE). So using h == GetStdHandle(STD_INPUT_HANDLE) seems clear and consistent to me.

[Piotr Caban (＠piotr)]

On Wed Oct  2 12:22:02 2024 +0000, eric pouech wrote:
> In that case, likely GetFileType(h) == FILE_TYPE_CHAR would be a clearer
> discrimination to call into console APIs

I don't know the details here (so ignore my comment if it doesn't make sense) but we had to switch to `VerifyConsoleIoHandle()` to properly detect console in msvcrt.

[Hans Leidekker (＠hans)]

On Wed Oct  2 12:22:02 2024 +0000, Piotr Caban wrote:
> I don't know the details here (so ignore my comment if it doesn't make
> sense) but we had to switch to `VerifyConsoleIoHandle()` to properly
> detect console in msvcrt.

I don't know the details of that fix but note that with this patch we still fall back to ReadFile() if ReadConsoleW() fails, e.g. because stdin was redirected.

[eric pouech (＠epo)]

the std input handle can be anything:
- it's by default the input handle from which cmd.exe has been created (someone could invoke cmd.exe with "cmd /c foo.bat < COM1" and you get a serial line handle)
- and it's changed with any redirection (so < COM1, < NUL, < CON would set a CHAR file type); and perhaps tomorrow | (piping) commands will be implemented with (named) pipes

Piotr's suggestion is even cleaner (as it will not accept other CHAR devices)

Note: we need to keep ReadConsole (instead of ReadFile) when we implement completion of commands