Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.

2019-09-04 Thread Corinna Vinschen
Hi Takashi,

On Sep  4 21:39, Takashi Yano wrote:
> Hi Corinna,
> 
> On Wed, 4 Sep 2019 12:03:51 +0200
> Corinna Vinschen wrote:
> > I'll push the other 3 patches from this series.  For this patch,
> > I wonder why you create set_ishybrid_and_switch_to_pcon while
> > at the same time define a macro CHK_CONSOLE_ACCESS with identical
> > functionality.
> 
> Yah, indeed!
> 
> > Suggestion: Only define set_ishybrid_and_switch_to_pcon() as
> > inline function (probably in winsup.h) and use only this througout.
> 
> This function uses static variable isHybrid (sorry camelback again)
> and static function set_switch_to_pcon() defined in fhandler_tty.cc.
> 
> To make it inline, a lot of changes will be necessary. How about
> non-inline function?

That will add extra function calls, but, yeah, sure.  We can streamline
this later.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer


signature.asc
Description: PGP signature


Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.

2019-09-04 Thread Takashi Yano
Hi Corinna,

On Wed, 4 Sep 2019 12:03:51 +0200
Corinna Vinschen wrote:
> I'll push the other 3 patches from this series.  For this patch,
> I wonder why you create set_ishybrid_and_switch_to_pcon while
> at the same time define a macro CHK_CONSOLE_ACCESS with identical
> functionality.

Yah, indeed!

> Suggestion: Only define set_ishybrid_and_switch_to_pcon() as
> inline function (probably in winsup.h) and use only this througout.

This function uses static variable isHybrid (sorry camelback again)
and static function set_switch_to_pcon() defined in fhandler_tty.cc.

To make it inline, a lot of changes will be necessary. How about
non-inline function?

-- 
Takashi Yano 


Re: [PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.

2019-09-04 Thread Corinna Vinschen
Hi Takashi,

On Sep  4 10:44, Takashi Yano wrote:
> - API hook used for pseudo console support causes slow down.
>   This patch limits API hook to only program which is linked
>   with the corresponding APIs. Normal cygwin program is not
>   linked with such APIs (such as WriteFile, etc...) directly,
>   therefore, no slow down occurs. However, console access by
>   cygwin.dll itself cannot switch the r/w pipe to pseudo console
>   side. Therefore, the code to switch it forcely to pseudo
>   console side is added to smallprint.cc and strace.cc.

I'll push the other 3 patches from this series.  For this patch,
I wonder why you create set_ishybrid_and_switch_to_pcon while
at the same time define a macro CHK_CONSOLE_ACCESS with identical
functionality.

Suggestion: Only define set_ishybrid_and_switch_to_pcon() as
inline function (probably in winsup.h) and use only this througout.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer


signature.asc
Description: PGP signature


[PATCH 4/4] Cygwin: pty: Limit API hook to the program linked with the APIs.

2019-09-03 Thread Takashi Yano
- API hook used for pseudo console support causes slow down.
  This patch limits API hook to only program which is linked
  with the corresponding APIs. Normal cygwin program is not
  linked with such APIs (such as WriteFile, etc...) directly,
  therefore, no slow down occurs. However, console access by
  cygwin.dll itself cannot switch the r/w pipe to pseudo console
  side. Therefore, the code to switch it forcely to pseudo
  console side is added to smallprint.cc and strace.cc.
---
 winsup/cygwin/fhandler_tty.cc | 60 ---
 winsup/cygwin/smallprint.cc   |  5 +++
 winsup/cygwin/strace.cc   | 29 +++--
 3 files changed, 51 insertions(+), 43 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index f76f7b262..3a23083de 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -89,6 +89,18 @@ set_switch_to_pcon (void)
   }
 }
 
+void set_ishybrid_and_switch_to_pcon (HANDLE h)
+{
+  DWORD dummy;
+  if (!isHybrid
+  && GetFileType (h) == FILE_TYPE_CHAR
+  && GetConsoleMode (h, ))
+{
+  isHybrid = true;
+  set_switch_to_pcon ();
+}
+}
+
 #define DEF_HOOK(name) static __typeof__ (name) *name##_Orig
 DEF_HOOK (WriteFile);
 DEF_HOOK (WriteConsoleA);
@@ -101,6 +113,7 @@ DEF_HOOK (WriteConsoleOutputW);
 DEF_HOOK (WriteConsoleOutputCharacterA);
 DEF_HOOK (WriteConsoleOutputCharacterW);
 DEF_HOOK (WriteConsoleOutputAttribute);
+DEF_HOOK (SetConsoleTextAttribute);
 DEF_HOOK (WriteConsoleInputA);
 DEF_HOOK (WriteConsoleInputW);
 DEF_HOOK (ReadConsoleInputA);
@@ -197,6 +210,13 @@ WriteConsoleOutputAttribute_Hooked
   return WriteConsoleOutputAttribute_Orig (h, a, l, c, n);
 }
 static BOOL WINAPI
+SetConsoleTextAttribute_Hooked
+ (HANDLE h, WORD a)
+{
+  CHK_CONSOLE_ACCESS (h);
+  return SetConsoleTextAttribute_Orig (h, a);
+}
+static BOOL WINAPI
 WriteConsoleInputA_Hooked
  (HANDLE h, CONST INPUT_RECORD *r, DWORD l, LPDWORD n)
 {
@@ -242,6 +262,7 @@ PeekConsoleInputW_Hooked
 #define WriteFile_Orig 0
 #define ReadFile_Orig 0
 #define PeekConsoleInputA_Orig 0
+void set_ishybrid_and_switch_to_pcon (void) {}
 #endif /* USE_API_HOOK */
 
 bool
@@ -2855,25 +2876,26 @@ fhandler_pty_slave::fixup_after_exec ()
{ \
  void *api = hook_api (module, #name, (void *) name##_Hooked); \
  name##_Orig = (__typeof__ (name) *) api; \
- if (!api) system_printf("Hooking " #name " failed."); \
-   }
-  DO_HOOK ("kernel32.dll", WriteFile);
-  DO_HOOK ("kernel32.dll", WriteConsoleA);
-  DO_HOOK ("kernel32.dll", WriteConsoleW);
-  DO_HOOK ("kernel32.dll", ReadFile);
-  DO_HOOK ("kernel32.dll", ReadConsoleA);
-  DO_HOOK ("kernel32.dll", ReadConsoleW);
-  DO_HOOK ("kernel32.dll", WriteConsoleOutputA);
-  DO_HOOK ("kernel32.dll", WriteConsoleOutputW);
-  DO_HOOK ("kernel32.dll", WriteConsoleOutputCharacterA);
-  DO_HOOK ("kernel32.dll", WriteConsoleOutputCharacterW);
-  DO_HOOK ("kernel32.dll", WriteConsoleOutputAttribute);
-  DO_HOOK ("kernel32.dll", WriteConsoleInputA);
-  DO_HOOK ("kernel32.dll", WriteConsoleInputW);
-  DO_HOOK ("kernel32.dll", ReadConsoleInputA);
-  DO_HOOK ("kernel32.dll", ReadConsoleInputW);
-  DO_HOOK ("kernel32.dll", PeekConsoleInputA);
-  DO_HOOK ("kernel32.dll", PeekConsoleInputW);
+ /*if (api) system_printf(#name " hooked.");*/ \
+   }
+  DO_HOOK (NULL, WriteFile);
+  DO_HOOK (NULL, WriteConsoleA);
+  DO_HOOK (NULL, WriteConsoleW);
+  DO_HOOK (NULL, ReadFile);
+  DO_HOOK (NULL, ReadConsoleA);
+  DO_HOOK (NULL, ReadConsoleW);
+  DO_HOOK (NULL, WriteConsoleOutputA);
+  DO_HOOK (NULL, WriteConsoleOutputW);
+  DO_HOOK (NULL, WriteConsoleOutputCharacterA);
+  DO_HOOK (NULL, WriteConsoleOutputCharacterW);
+  DO_HOOK (NULL, WriteConsoleOutputAttribute);
+  DO_HOOK (NULL, SetConsoleTextAttribute);
+  DO_HOOK (NULL, WriteConsoleInputA);
+  DO_HOOK (NULL, WriteConsoleInputW);
+  DO_HOOK (NULL, ReadConsoleInputA);
+  DO_HOOK (NULL, ReadConsoleInputW);
+  DO_HOOK (NULL, PeekConsoleInputA);
+  DO_HOOK (NULL, PeekConsoleInputW);
 }
 #endif /* USE_API_HOOK */
 }
diff --git a/winsup/cygwin/smallprint.cc b/winsup/cygwin/smallprint.cc
index a7a19132b..bd19c1f5f 100644
--- a/winsup/cygwin/smallprint.cc
+++ b/winsup/cygwin/smallprint.cc
@@ -384,6 +384,9 @@ __small_sprintf (char *dst, const char *fmt, ...)
   return r;
 }
 
+/* Defined in fhandler_tty.cc */
+extern void set_ishybrid_and_switch_to_pcon (HANDLE);
+
 void
 small_printf (const char *fmt, ...)
 {
@@ -405,6 +408,7 @@ small_printf (const char *fmt, ...)
   count = __small_vsprintf (buf, fmt, ap);
   va_end (ap);
 
+  set_ishybrid_and_switch_to_pcon (GetStdHandle (STD_ERROR_HANDLE));
   WriteFile (GetStdHandle (STD_ERROR_HANDLE), buf, count, , NULL);
   FlushFileBuffers (GetStdHandle (STD_ERROR_HANDLE));
 }
@@ -431,6 +435,7 @@ console_printf (const char