Bug in Ruby's PTY extension

I’m new to the ML so please forgive me if this has been discussed
previously.

Ruby version: 1.8.5
OS: Solaris 10 x64 (11/06)
CPU: Dual core Opteron

When using expect or RExpect I was getting an intermittent premature
EOFError on the first call to expect. Catching this exception and then
going on was an ugly work around.

pty.c:

I noticed that Solaris 10 does not support TIOCSCTTY so in function
establishShell() the child process closes the slave device and reopens
it to
force it to be the controlling tty. The parent process also closes the
slave device because it does not need it. I added a call to nanosleep
for
20 milliseconds just before the parent process closes the slave fd at
the
end of establishShell() and I now both expect and RExpect work reliably.
I
suspect that just keeping a reference to the open file object in the
parent
process while the child process closes and reopens the slave device
prevents
the EOF from occurring – but I am far from an expert on Solaris
internals.
This may not be the best way to solve the problem and just adding the
small
delay does not ensure that the problem will never happen again but so
far,
so good.

Ron

Hi,

At Mon, 6 Aug 2007 02:28:38 +0900,
ronald braswell wrote in [ruby-talk:263410]:

delay does not ensure that the problem will never happen again but so far,
so good.

Does this patch work?

Index: ext/pty/expect_sample.rb

— ext/pty/expect_sample.rb (revision 12878)
+++ ext/pty/expect_sample.rb (working copy)
@@ -16,8 +16,4 @@ PTY.spawn(“ftp ftp.ruby-lang.org”) do |r
$expect_verbose = false

  • r_f.expect(/^Name.*: /) do

  • w_f.print “ftp\n”

  • end

  • if !ENV[‘USER’].nil?
    username = ENV[‘USER’]
    @@ -28,9 +24,6 @@ PTY.spawn(“ftp ftp.ruby-lang.org”) do |r
    end

  • r_f.expect(‘word:’) do

  • w_f.print username+“@\n”

  • end

  • r_f.expect("> ") do

  • w_f.print “cd pub/ruby\n”

  • r_f.expect(/^(Name).*: |(word):|> /) do
  • w_f.puts($1 ? “ftp” : $2 ? “#{username}@” : “cd pub/ruby”)
    end
    r_f.expect("> ") do
    Index: ext/pty/pty.c
    ===================================================================
    — ext/pty/pty.c (revision 12878)
    +++ ext/pty/pty.c (working copy)
    @@ -40,8 +40,8 @@
    #if !defined(HAVE_OPENPTY)
    #if defined(__hpux)
    -static
    -char *MasterDevice = “/dev/ptym/pty%s”,
  • *SlaveDevice = “/dev/pty/tty%s”,
  • *deviceNo[] = {
    +static const
    +char MasterDevice[] = “/dev/ptym/pty%s”,
  • SlaveDevice[] = “/dev/pty/tty%s”,
  • *const deviceNo[] = {
    “p0”,“p1”,“p2”,“p3”,“p4”,“p5”,“p6”,“p7”,
    “p8”,“p9”,“pa”,“pb”,“pc”,“pd”,“pe”,“pf”,
    @@ -63,8 +63,8 @@ char MasterDevice = “/dev/ptym/pty%s”,
    };
    #elif defined(_IBMESA) /
    AIX/ESA */
    -static
    -char *MasterDevice = “/dev/ptyp%s”,
  • *SlaveDevice = “/dev/ttyp%s”,
  • *deviceNo[] = {
    +static const
    +char MasterDevice[] = “/dev/ptyp%s”,
  • SlaveDevice[] = “/dev/ttyp%s”,
  • *const deviceNo[] = {
    “00”,“01”,“02”,“03”,“04”,“05”,“06”,“07”,“08”,“09”,“0a”,“0b”,“0c”,“0d”,“0e”,“0f”,
    “10”,“11”,“12”,“13”,“14”,“15”,“16”,“17”,“18”,“19”,“1a”,“1b”,“1c”,“1d”,“1e”,“1f”,
    @@ -85,8 +85,8 @@ char *MasterDevice = “/dev/ptyp%s”,
    };
    #elif !defined(HAVE_PTSNAME)
    -static
    -char *MasterDevice = “/dev/pty%s”,
  • *SlaveDevice = “/dev/tty%s”,
  • *deviceNo[] = {
    +static const
    +char MasterDevice[] = “/dev/pty%s”,
  • SlaveDevice[] = “/dev/tty%s”,
  • *const deviceNo[] = {
    “p0”,“p1”,“p2”,“p3”,“p4”,“p5”,“p6”,“p7”,
    “p8”,“p9”,“pa”,“pb”,“pc”,“pd”,“pe”,“pf”,
    @@ -102,6 +102,4 @@ char MasterDevice = “/dev/pty%s”,
    #endif /
    !defined(HAVE_OPENPTY) */

-static char SlaveName[DEVICELEN];

#ifndef HAVE_SETEUID

ifdef HAVE_SETREUID

@@ -156,15 +154,13 @@ pty_syswait(info)
if (cpid == -1) return Qnil;

-#if defined(IF_STOPPED)

  • if (IF_STOPPED(status)) { /* suspend */
  •  raise_from_wait("stopped", info);
    
  • }
    -#elif defined(WIFSTOPPED)
  • if (WIFSTOPPED(status)) { /* suspend */
  •  raise_from_wait("stopped", info);
    
  • }
    +#if defined(WIFSTOPPED)
    +#elif defined(IF_STOPPED)
    +#define WIFSTOPPED(status) IF_STOPPED(status)
    #else
    ---->> Either IF_STOPPED or WIFSTOPPED is needed <<----
    #endif /* WIFSTOPPED | IF_STOPPED */
  • if (WIFSTOPPED(status)) { /* suspend */
  •  raise_from_wait("stopped", info);
    
  • }
    else if (kill(info->child_pid, 0) == 0) {
    raise_from_wait(“changed”, info);
    @@ -177,5 +173,5 @@ pty_syswait(info)
    }

-static void getDevice _((int*, int*));
+static void getDevice _((int*, int*, char [DEVICELEN]));

struct exec_info {
@@ -195,11 +191,12 @@ pty_exec(v)

static void
-establishShell(argc, argv, info)
+establishShell(argc, argv, info, SlaveName)
int argc;
VALUE *argv;
struct pty_info *info;

  • char SlaveName[DEVICELEN];
    {
    int i,master,slave;
  • char *p,*getenv();
  • char *p, tmp, *getenv();
    struct passwd *pwent;
    VALUE v;
    @@ -224,5 +221,5 @@ establishShell(argc, argv, info)
    argv = &v;
    }
  • getDevice(&master,&slave);
  • getDevice(&master, &slave, SlaveName);

    info->thread = rb_thread_current();
    @@ -274,4 +271,5 @@ establishShell(argc, argv, info)
    close(master);
    #endif

  • write(slave, “”, 1);
    dup2(slave,0);
    dup2(slave,1);
    @@ -289,4 +287,5 @@ establishShell(argc, argv, info)
    }

  • read(master, &tmp, 1);
    close(slave);

@@ -306,6 +305,7 @@ pty_finalize_syswait(info)

static int
-get_device_once(master, slave, fail)
+get_device_once(master, slave, SlaveName, fail)
int *master, *slave, fail;

  • char SlaveName[DEVICELEN];
    {
    #if defined HAVE_OPENPTY
    @@ -354,4 +354,5 @@ get_device_once(master, slave, fail)
    if(ioctl(j, I_PUSH, “ptem”) != -1) {
    if(ioctl(j, I_PUSH, “ldterm”) != -1) {
  •    ioctl(j, I_PUSH, "ttcompat");
    

#endif
*master = i;
@@ -396,10 +397,11 @@ get_device_once(master, slave, fail)

static void
-getDevice(master, slave)
+getDevice(master, slave, SlaveName)
int *master, *slave;

  • char SlaveName[DEVICELEN];
    {
  • if (get_device_once(master, slave, 0)) {
  • if (get_device_once(master, slave, SlaveName, 0)) {
    rb_gc();
  • get_device_once(master, slave, 1);
  • get_device_once(master, slave, SlaveName, 1);
    }
    }
    @@ -418,9 +420,10 @@ pty_getpty(argc, argv, self)
    VALUE rport = rb_obj_alloc(rb_cFile);
    VALUE wport = rb_obj_alloc(rb_cFile);

  • char SlaveName[DEVICELEN];

    MakeOpenFile(rport, rfptr);
    MakeOpenFile(wport, wfptr);

  • establishShell(argc, argv, &info);
  • establishShell(argc, argv, &info, SlaveName);

    rfptr->mode = rb_io_mode_flags(“r”);

I noticed that Solaris 10 does not support TIOCSCTTY so in function
parent
Does this patch work?

  • w_f.print “ftp\n”
  • r_f.expect("> ") do
    #if !defined(HAVE_OPENPTY)
    “p8”,“p9”,“pa”,“pb”,“pc”,“pd”,“pe”,“pf”,
  • *const deviceNo[] = {
  • *deviceNo[] = {
  • if (WIFSTOPPED(status)) { /* suspend */
  • }

  • char *p, tmp, *getenv();
    close(master);
    @@ -306,6 +305,7 @@ pty_finalize_syswait(info)
    if(ioctl(j, I_PUSH, “ldterm”) != -1) {
    {

  • char SlaveName[DEVICELEN];

    Nobu Nakada

Hi Nobu,

Thanks for your patch. It is much better than the quick hack that I had
done. I have tested it and it works fine! Just one thing, I had to
comment
out freeDevice() in my version of pty.c since it referenced the
previously
file scoped SlaveName. freeDevice() was not externally linked and there
were no references to it in pty.c. Maybe I overlooked the deletion of
it
in you diff -u output.

Ron

Hi,

In message “Re: Bug in Ruby’s PTY extension”
on Mon, 6 Aug 2007 17:38:17 +0900, Nobuyoshi N.
[email protected] writes:

|Does this patch work?

|— ext/pty/expect_sample.rb (revision 12878)
|+++ ext/pty/expect_sample.rb (working copy)

Can you commit please?

          matz.

Hi,

At Tue, 7 Aug 2007 01:28:08 +0900,
ronald braswell wrote in [ruby-talk:263528]:

Thanks for your patch. It is much better than the quick hack that I had
done. I have tested it and it works fine! Just one thing, I had to comment
out freeDevice() in my version of pty.c since it referenced the previously
file scoped SlaveName. freeDevice() was not externally linked and there
were no references to it in pty.c. Maybe I overlooked the deletion of it
in you diff -u output.

It has been deleted in 1.8.6 already, since it wasn’t used at
all.

On Aug 5, 11:28 am, “ronald braswell” [email protected] wrote:

the EOF from occurring – but I am far from an expert on Solaris internals.
This may not be the best way to solve the problem and just adding the small
delay does not ensure that the problem will never happen again but so far,
so good.

This quote from comp.programming may be useful:

"In System V, the controlling terminal is the first tty that’s opened
that is not already associated with a session. If you don’t want that
tty to be the controlling terminal, use the O_NOCTTY flag on open().

You don’t need the TIOCSCTTY ioctl in SVR4 systems."

http://tinyurl.com/2af8ob

Regards,

Dan