isatty: make it return true in Cygwin consoles on native Windows

isatty: make it return true in Cygwin consoles on native Windows

[Bruno Haible]

When a native Windows program call isatty(STDOUT_FILENO) or
isatty(STDIN_FILENO), it returns true for a cmd.exe console
but false for a Cygwin console. This makes no sense, because the
Cygwin console is just as interactive as the cmd.exe console; only
the input/output encoding is different (UTF-8 vs. GetOEMCP()).

This patch fixes it.


2019-03-14  Bruno Haible  <bruno@clisp.org>

	isatty: Make it return true in Cygwin consoles on native Windows.
	* lib/isatty.c: Include <string.h>.
	(GetProcAddress): New macro.
	(GetNamedPipeClientProcessIdFuncType): New type.
	(GetNamedPipeClientProcessIdFunc): New variable.
	(QueryFullProcessImageNameFuncType): New type.
	(QueryFullProcessImageNameFunc): New variable.
	(initialized): New variable.
	(initialize): New function.
	(IsCygwinConsoleHandle): New function.
	(isatty): Invoke it.
	* doc/posix-functions/isatty.texi: Mention the issue.

diff --git a/doc/posix-functions/isatty.texi b/doc/posix-functions/isatty.texi
index 61c55da..b5e196b 100644
--- a/doc/posix-functions/isatty.texi
+++ b/doc/posix-functions/isatty.texi
@@ -12,6 +12,8 @@ Portability problems fixed by Gnulib:
 On native Windows, this function also returns true for character devices such
 as @file{NUL}.
 @item
+On native Windows, this function returns false for Cygwin consoles.
+@item
 This function crashes when invoked with invalid arguments on some platforms:
 MSVC 14.
 @end itemize
diff --git a/lib/isatty.c b/lib/isatty.c
index 71b32dd..085cd48 100644
--- a/lib/isatty.c
+++ b/lib/isatty.c
@@ -22,6 +22,7 @@
 /* This replacement is enabled on native Windows.  */
 
 #include <errno.h>
+#include <string.h>
 
 /* Get declarations of the Win32 API functions.  */
 #define WIN32_LEAN_AND_MEAN
@@ -38,12 +39,90 @@
 # include <io.h>
 #endif
 
+/* Avoid warnings from gcc -Wcast-function-type.  */
+#define GetProcAddress \
+  (void *) GetProcAddress
+
+/* GetNamedPipeClientProcessId was introduced only in Windows Vista.  */
+typedef BOOL (WINAPI * GetNamedPipeClientProcessIdFuncType) (HANDLE hPipe,
+                                                             PULONG pClientProcessId);
+static GetNamedPipeClientProcessIdFuncType GetNamedPipeClientProcessIdFunc = NULL;
+/* QueryFullProcessImageName was introduced only in Windows Vista.  */
+typedef BOOL (WINAPI * QueryFullProcessImageNameFuncType) (HANDLE hProcess,
+                                                           DWORD dwFlags,
+                                                           LPSTR lpExeName,
+                                                           PDWORD pdwSize);
+static QueryFullProcessImageNameFuncType QueryFullProcessImageNameFunc = NULL;
+static BOOL initialized = FALSE;
+
+static void
+initialize (void)
+{
+  HMODULE kernel32 = LoadLibrary ("kernel32.dll");
+  if (kernel32 != NULL)
+    {
+      GetNamedPipeClientProcessIdFunc =
+        (GetNamedPipeClientProcessIdFuncType) GetProcAddress (kernel32, "GetNamedPipeClientProcessId");
+      QueryFullProcessImageNameFunc =
+        (QueryFullProcessImageNameFuncType) GetProcAddress (kernel32, "QueryFullProcessImageNameA");
+    }
+  initialized = TRUE;
+}
+
 static BOOL IsConsoleHandle (HANDLE h)
 {
   DWORD mode;
+  /* GetConsoleMode
+     <https://docs.microsoft.com/en-us/windows/console/getconsolemode> */
   return GetConsoleMode (h, &mode) != 0;
 }
 
+static BOOL IsCygwinConsoleHandle (HANDLE h)
+{
+  /* A handle to a Cygwin console is in fact a named pipe whose client process
+     and server process is <CYGWIN_INSTALL_DIR>\bin\mintty.exe.  */
+  BOOL result = FALSE;
+  ULONG processId;
+
+  if (!initialized)
+    initialize ();
+
+  /* GetNamedPipeClientProcessId
+     <https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getnamedpipeclientprocessid>
+     It requires -D_WIN32_WINNT=_WIN32_WINNT_VISTA or higher.  */
+  if (GetNamedPipeClientProcessIdFunc && QueryFullProcessImageNameFunc
+      && GetNamedPipeClientProcessIdFunc (h, &processId))
+    {
+      /* OpenProcess
+         <https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-openprocess> */
+      HANDLE processHandle =
+        OpenProcess (PROCESS_QUERY_LIMITED_INFORMATION, FALSE, processId);
+      if (processHandle != NULL)
+        {
+          char buf[1024];
+          DWORD bufsize = sizeof (buf);
+          /* The file name can be determined through
+             GetProcessImageFileName
+             <https://docs.microsoft.com/en-us/windows/desktop/api/psapi/nf-psapi-getprocessimagefilenamea>
+             or
+             QueryFullProcessImageName
+             <https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-queryfullprocessimagenamea>
+             The former returns a file name in non-standard notation (it starts
+             with '\Device\') and may require linking with psapi.dll.
+             The latter is better, but requires -D_WIN32_WINNT=_WIN32_WINNT_VISTA
+             or higher.  */
+          if (QueryFullProcessImageNameFunc (processHandle, 0, buf, &bufsize))
+            {
+              if (strlen (buf) >= 11
+                  && strcmp (buf + strlen (buf) - 11, "\\mintty.exe") == 0)
+                result = TRUE;
+            }
+          CloseHandle (processHandle);
+        }
+    }
+  return result;
+}
+
 #if HAVE_MSVC_INVALID_PARAMETER_HANDLER
 static int
 _isatty_nothrow (int fd)
@@ -84,6 +163,8 @@ isatty (int fd)
       if (IsConsoleHandle (h))
         return 1;
     }
+  if (IsCygwinConsoleHandle (h))
+    return 1;
   errno = ENOTTY;
   return 0;
 }

Re: isatty: make it return true in Cygwin consoles on native Windows

[Gisle Vanem]

Bruno Haible wrote:

> +          if (QueryFullProcessImageNameFunc (processHandle, 0, buf, &bufsize))
> +            {
> +              if (strlen (buf) >= 11
> +                  && strcmp (buf + strlen (buf) - 11, "\\mintty.exe") == 0)

What if the .exe has another name?

The purpose of your function looks similar to what Mihail Konev
did for the MinGW-w64 project:
   https://mingw-w64-public.narkive.com/s7kYRrtG/patch-mingw-w64-add-include-iscygtty-c

Instead he looked for pipes matching:
"\Device\NamedPipe\(cygwin|msys)-[a-fA-F0-9]{16}-pty[0-9]{1,4}-(from-master|to-master|to-master-cyg)"

BTW.
I created my own version of in his work in my Envtool
program. The 'is_cygwin_tty()' function:
   https://github.com/gvanem/EnvTool/blob/master/src/misc.c#L3918


-- 
--gv

Re: isatty: make it return true in Cygwin consoles on native Windows

[Bruno Haible]

Hi Gisle,

> > +                  && strcmp (buf + strlen (buf) - 11, "\\mintty.exe") == 0)
> 
> What if the .exe has another name?

What other names are in common use for this program?

> The purpose of your function looks similar to what Mihail Konev
> did for the MinGW-w64 project:
>    https://mingw-w64-public.narkive.com/s7kYRrtG/patch-mingw-w64-add-include-iscygtty-c
> 
> Instead he looked for pipes matching:
> "\Device\NamedPipe\(cygwin|msys)-[a-fA-F0-9]{16}-pty[0-9]{1,4}-(from-master|to-master|to-master-cyg)"

I prefer to avoid the ntdll.dll API when possible.

Bruno

Re: isatty: make it return true in Cygwin consoles on native Windows

[Gisle Vanem]

Bruno Haible wrote:

>> What if the .exe has another name?
> 
> What other names are in common use for this program?

No sure. I'll need to check the MinGW-w64 list and motive.
Some custom variation of it perhaps?

>> The purpose of your function looks similar to what Mihail Konev
>> did for the MinGW-w64 project:
>>     https://mingw-w64-public.narkive.com/s7kYRrtG/patch-mingw-w64-add-include-iscygtty-c
>>
>> Instead he looked for pipes matching:
>> "\Device\NamedPipe\(cygwin|msys)-[a-fA-F0-9]{16}-pty[0-9]{1,4}-(from-master|to-master|to-master-cyg)"
> 
> I prefer to avoid the ntdll.dll API when possible.

Okay, what's wrong with that?

-- 
--gv

Re: isatty: make it return true in Cygwin consoles on native Windows

[Bruno Haible]

Gisle Vanem asked:
> > I prefer to avoid the ntdll.dll API when possible.
> 
> Okay, what's wrong with that?

1) It's a violation of abstraction. Diagram:

         Programs
             |
    Windows API (kernel32.dll etc.)
             |
          NT API (ntdll.dll)
             |
        NT kernel

   Programs should use only topmost API layer.
   When you use a mix between two or more layers, i.e. when you circumvent
   the topmost API layer, in my experience this causes big problems in the
   long run.

2) The code you pointed to uses the function NtQueryObject. However, the
   Microsoft documentation
   <https://docs.microsoft.com/en-us/windows/desktop/api/winternl/nf-winternl-ntqueryobject>
   states "This function may be changed or removed from Windows without
   further notice."

3) Probably code will run better on ReactOS or WINE if they don't use the
   lower layers.

Bruno

Re: isatty: make it return true in Cygwin consoles on native Windows

[LRN]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]

On 15.03.2019 22:42, Bruno Haible wrote:
> Gisle Vanem asked:
>>> I prefer to avoid the ntdll.dll API when possible.
>>
>> Okay, what's wrong with that?
> 
> 1) It's a violation of abstraction.
> 
> 2) The code you pointed to uses the function NtQueryObject. However, the
>    Microsoft documentation
>    <https://docs.microsoft.com/en-us/windows/desktop/api/winternl/nf-winternl-ntqueryobject>
>    states "This function may be changed or removed from Windows without
>    further notice."
> 
> 3) Probably code will run better on ReactOS or WINE if they don't use the
>    lower layers.
> 
Advanced functionality sometimes requires the use of kernel API in cases where
Microsoft decided not to expose some NT kernel functions to applications in
Win32 API. These situations happen from time to time when dealing with portability.

You can lessen the impact by configure-time-testing the kernel APIs to ensure
that they are available and behave as expected. In some sense this is kind of
like using very Linux-specific functions. If the functions vanish, you'll
notice. Also ReactOS and WINE won't use that code, if they have no functions
and thus fail the configure-tests.

Either way, some things cannot be implemented in a clear and/or performant way
without these.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: isatty: make it return true in Cygwin consoles on native Windows

[Bruno Haible]

LRN wrote:
> Either way, some things cannot be implemented in a clear and/or performant way
> without these. [ntdll]

Sure, I don't dispute this.

But when it's possible - like in this case with isatty() - I prefer to
rely only on the Windows API.

Bruno