Added C++ wrapper script.

[piannucci]

No description provided.

[kmcallister]

This has a few build errors on my Linux system. I'm working on fixes.

[kmcallister]

I have a few fixes here. I also rebased out the erroneous binary file add.

It still fails to build because glibc doesn't have fgetln. We could get it from libbsd but I'd rather not add another dep just for this function. It should be easy enough to replace it with getline (which is supported by glibc and not OS X, sigh).

[dimikot]

Here are pure implementations of fgetln() and getline().
http://codepad.org/swe7NRct

[dimikot]

I have also tested it (with fgetln replacements of course) an confirm that this C++ version of the wrapper works correctly under cygwin (even key mappings are OK in Midnight Commander, wow!), so C's cygwin PTYs work without bugs (not the same as Perl's PTYs which seems to be quite buggy). So it may be freely merged into the mainstream (possibly with some corrections, because I suppose perl's wrapper is ahead by functionality).

[keithw]

The Perl script seems to have crystallized enough that I think we don't lose anything by moving to C++. Peter, this looks really good.

One thing I would like is to execute the subsidiary SSH "properly" with forkpty() (similarly to how mosh-server works) and then have the wrapper proxy all the user's keystrokes to SSH and then handle displaying the SSH output itself.

Right now ssh is reading directly from /dev/tty and printing the password prompt to /dev/tty (which goes straight to the user's terminal, bypassing the Perl script), and then the later output to stdout (which the Perl script parses line-by-line).

We can fix issue #52 by having all keystrokes go through the wrapper and all output be parsed by the wrapper before display. This does make the logic a little more complicated because we want to display the line to the user on a byte-by-byte basis unless it seems to be starting with \nMOSH IP...

[kmcallister]

There are a few more issues we need to address before this is merged.

• We just got rid of Boost in Mosh mainline (Remove Boost dependency #158).
• The code formatting is inconsistent with the rest of the project.
• Doesn't support --ssh, added in 86113b5.
• We should do a careful security review. In particular, it's bad if a malicious server can exploit the connecting client.

[piannucci]

I have pushed an updated version that addresses the boost, formatting, and --ssh issues.

For compatibility with 86113b5 I am invoking sh -c with the fully-composed exec string. According to perl.org/functions/exec, this behavior is system-dependent.

[dimikot]

Possibly you could add fgetln() emulation (e.g. from the code at http://codepad.org/swe7NRct I've copied from its original sources) and create a new (final) pull request? I suppose it could speed up the process.

[piannucci]

@DmitryKoterov, it might help if you made clear what the provenance and license of that code is.

[dimikot]

This code is from http://www.gnu-darwin.org/www001/ports-1.5a-CURRENT/ftp/bsdftpd-ssl/work/bsdftpd-ssl-1.1.0/contrib/libbsdport/libc/stdio/fgetln.c and http://ar.runcode.us/q/are-there-alternate-implementations-of-gnu-getline-interface - I don't know what licences are used (suppose the same as libc). The pieces are so short that they could be rewritten from scratch. :-)

[piannucci]

My cxx-wrapper branch now avoids fgetln, using getline instead. Builds on OS X and whatever flavor of Debian is running on linerva.mit.edu.

[dimikot]

People say that getline() is not supported in Mac OS X Snow Leopard: http://stackoverflow.com/questions/1117108/compiling-c-code-using-gnu-c-getline-on-mac-osx

Why don't you get rid of it totally? You may use fgets() with a fixed length buffer - fgets() is supported by any platform. In such case you should also replace

printf( "%s\n", line.c_str() ); 

by

puts( buffer_of_fgets );

And - after that - could you please update this pull request?

[kmcallister]

The cxx-wrapper branch is looking pretty good, and it builds cleanly for me on Debian.

I wonder if this should be part of the mosh-client binary. Then we could end (in the common case) the sketchy practice of passing the AES key through an environment variable. See also #156.

I think the most important thing with this code is the correctness of the string and buffer manipulation, which is pretty hard to verify. I did notice a few self-contained things though.

cat doesn't handle the case where read returns an error other than EINTR. This could result in passing a huge size to write.

ioctl( 0, TIOCGWINSZ, &ws );

You don't check for an error. If ioctl fails we pass stack garbage to openpty.

static const char *const MOSH_VERSION = "1.2";

Autoconf already defines PACKAGE_VERSION and VERSION in config.h.

inline string shell_quote_string( string x ) ...
inline string shell_quote( SequenceT &sequence ) ...
void predict_check( string predict, bool env_set ) ...

The string arguments should probably be const string &. The SequenceT & can be const and its iterator a const_iterator.

void die( const char *format, ... ) ...

I think this function would be simpler as a variadic macro.

static char **argv;

It looks like this global is only used for argv[0]. In that case, we can globalize just that string, and keep the conventional name for main's argument.

signal( SIGHUP, SIG_IGN );

The Linux signal(2) manpage says

The behavior of signal() varies across UNIX versions, and has also varied historically across different versions of Linux. Avoid its use: use sigaction(2) instead.

But later on the manpage suggests it's portable for SIG_IGN, so this is probably OK.

Instead of

string explanation;
if ( env_set ) {
  explanation = " (MOSH_PREDICTION_DISPLAY in environment)";               
}                                                                          
fprintf( stderr, "%s: Unknown mode \"%s\"%s.\n", argv[0], predict.c_str(), explanation.c_str() );

how about

fprintf( stderr, "%s: Unknown mode \"%s\"%s.\n", argv[0], predict.c_str(),
         env_set ? " (MOSH_PREDICTION_DISPLAY in environment)" : "" );

Did we decide anything about using iostream for getline?

This file has a 125 character line, which I think is too long. I know Mosh has some longer lines but I'd like to keep them below 100 where possible.

[piannucci]

Errors from read and ioctl trapped. Replaced MOSH_VERSION with PACKAGE_VERSION. Used const. Replaced argv global with argv0. Made other cleanups including line wrapping.

I don't trust myself to write a portable variadic macro.

You can't open an iostream from a file descriptor or a FILE* in a portable way, so I don't see an obvious alternative to getline.

[dimikot]

Why don't you use fgets() with a static buffer? I've written above that it may be used safely: #96 (comment)

[rurban]

I needed to fix patch-3,
#include <unistd.h> missing for read et al in frontent/mosh.cc

See piannucci#1 which merges patch-3 into master.

Tested ok on linux and cygwin, the cygwin ssh prompt issue was tested ok.

[rurban]

I've uploaded now an experimental cygwin package 1.2.2-2 with this branch.
So cygwin users with perl IO::Pty problems can try it out and give feedback.

http://cygwin.com/ml/cygwin-announce/2012-07/msg00021.html

I've also added this commit rurban@f53009c

• rm scripts/mosh (perl gone)

[davidben]

The newest version doesn't parse the mosh-client -c output quite right. It gets confused by the trailing newline that getline leaves in there. This patch should fix it:

diff --git a/src/frontend/mosh.cc b/src/frontend/mosh.cc
index 1da606f..274c487 100644
--- a/src/frontend/mosh.cc
+++ b/src/frontend/mosh.cc
@@ -376,6 +376,9 @@ int main( int argc, char *argv[] )
   if ( ( n = getline( &buf, &buf_sz, color_file ) ) < 0 ) {
     die( "%s: Can't count colors: %d", argv[0], errno );
   }
+  // chomp the trailing newline.
+  if ( n > 0 && buf[n - 1] == '\n' )
+    n--;
   string colors = string( buf, n );
   pclose( color_file );

It also leaves the dist_bin_SCRIPTS line in scripts/Makefile.am so the C++ version is actually clobbered by the perl version on make install for me. I don't know if that's intentional.

[jalcine]

This isn't going to be picked up? I'd like to help with this but it hasn't been touched in nearly a year.

[acornejo]

are there any technical difficulties holding this back, or is just more testing?