winpty injection

[Corinna Vinschen]

On Apr  4 19:07, Thomas Wolff wrote:
> The interworking problems of pty-based terminals with native Windows
> programs have long been discussed and raise recurring user reports and
> complaints. Mintty issues https://github.com/mintty/mintty/issues/56
> (Improve support for native console programs) and
> https://github.com/mintty/mintty/issues/376 (Use different encodings for
> native Windows commands output and remote output) have dozens of duplicates.
> There is the fairly well-known wrapper winpty which solves most of the
> interworking problems. However, it needs to be involved explicitly and
> selectively for Windows command-line applications.
> 
> So I had the idea that cygwin could apply winpty wrapping automatically when
> it detects that a non-cygwin command-line program is being called. The
> attached patch demonstrates how this would work. It is a proof-of-concept,
> not ready for integration but provided for discussion of the approach (thus
> not sent to cygwin-patches).

The idea is not bad, in general.  A few points, though.

> If it is appreciated, there are a number of open issues:
> * The patch offers two methods. The activated one (#ifdef
> inject_winpty_recursively) works but leaves a memory leak in the argv1 array
> needed to expand the argv with the injected wrapper. The other one, which
> tries to inject winpty into argv and then continue the function linearly,
> would solve the memory leak but does not work (yet)...

The problem is that you're trying to do the stuff in
child_info_spawn::worker, while the interesting action for this case
occurs in av::setup.  You should be able to do your stuff there without
too much problems.

Additionally this function opens the executable anyway to check if it's
a Cygwin executable.  Check out the __try/__except block starting at
spawn.cc line 1090.  The hook_or_detect_cygwin function conveniently
returns a value in subsys.  It's not used at the moment, but this is
the PE/COFF header info returning the subsystem.  I.e.
IMAGE_SUBSYSTEM_WINDOWS_GUI = 2, IMAGE_SUBSYSTEM_WINDOWS_CUI = 3, ...

> * The winpty tool is not even a cygwin package yet. If the injection
> approach is approved, it should be packaged as a base package, or even its
> code could be migrated into cygwin.

Sure for the Cygwin package.  Not so sure if the agent part could be
included into Cygwin.  This may be better discussed with the original
author of the code.

> * As some people may object such a magic (and it might in fact raise
> unexpected problems), it could also be made dependent on a setting in the
> CYGWIN environment variable.

Nah.  If it improves the situation, nobody will complain.  However, it
may slow down calling native Windows apps considerably when used a lot
from scripts.

> Looking forward to feedback
> Thomas

> From 2233529613af5a3b569a078d4e33334bbf8bd7e2 Mon Sep 17 00:00:00 2001
> From: Thomas Wolff <towo@towo.net>
> Date: Fri, 23 Mar 2018 22:34:11 +0100
> Subject: [PATCH] winpty injection (proof of concept)
> 
> If a non-cygwin program is run from a pty-based environment,
> the winpty wrapper is injected to adapt:
> * pty handling, especially raw input
> * character set conversion
> * limited signal handling interworking
> ---
>  winsup/cygwin/spawn.cc | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 37db526..3cd442e 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -292,6 +292,7 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>    av newargv;
>    linebuf cmd;
>    PWCHAR envblock = NULL;
> +  char * * argv1 = 0;

Style.  Please make sure you stick to the style of the surrounding
code.  Here:   char **argv1 = NULL;

>    path_conv real_path;
>    bool reset_sendsig = false;
>  
> @@ -332,10 +333,53 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>  	}
>  
>        res = newargv.setup (prog_arg, real_path, ext, ac, argv, p_type_exec);
> -
>        if (res)
>  	__leave;
>  
> +      /* winpty injection */
> +
> +      // provide a check for proper installation of winpty;
> +      // this slows us down a bit (thus called last below),
> +      // maybe it could check winpty.exe only or nothing...

Please use /* ... */ except for very short oneliner comments at the end
of an expression (and preferredly not even then).

> +#define winpty_available()	\
> +		   !access ("/bin/winpty.exe", R_OK | X_OK) \
> +		&& !access ("/bin/winpty.dll", R_OK | X_OK) \
> +		&& !access ("/bin/winpty-agent.exe", R_OK | X_OK)

Ideally this check should only actually check for the /bin/winpty.exe
executable and ideally this information is stored in shared storage
so it's only called once or, at least, very seldom.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-developers/attachments/20180405/21752bca/attachment.sig>