[Emacs-diffs] trunk r113471: Fix array bounds violation when pty allocation fails.

[Paul Eggert]

------------------------------------------------------------
revno: 113471
revision-id: address@hidden
parent: address@hidden
committer: Paul Eggert <address@hidden>
branch nick: trunk
timestamp: Sat 2013-07-20 08:33:00 -0700
message:
  Fix array bounds violation when pty allocation fails.
  
  * configure.ac (PTY_TTY_NAME_SPRINTF): Use PTY_NAME_SIZE,
  not sizeof pty_name, since pty_name is now a pointer to the array.
  * src/process.c (PTY_NAME_SIZE): New constant.
  (pty_name): Remove static variable; it's now auto.
  (allocate_pty): Define even if !HAVE_PTYS; that's simpler.
  Take pty_name as an arg rather than using a static variable.
  All callers changed.
  (create_process): Recover pty_flag from process, not from volatile local.
  (create_pty): Stay inside array even when pty allocation fails.
  (Fmake_serial_process): Omit unnecessary initializaiton of pty_flag.
modified:
  ChangeLog                      changelog-20091113204419-o5vbwnq5f7feedwu-1538
  configure.ac                   
configure.in-20091113204419-o5vbwnq5f7feedwu-783
  src/ChangeLog                  changelog-20091113204419-o5vbwnq5f7feedwu-1438
  src/process.c                  process.c-20091113204419-o5vbwnq5f7feedwu-462

=== modified file 'ChangeLog'
--- a/ChangeLog 2013-07-13 00:01:43 +0000
+++ b/ChangeLog 2013-07-20 15:33:00 +0000
@@ -1,3 +1,9 @@
+2013-07-20  Paul Eggert  <address@hidden>
+
+       Fix array bounds violation when pty allocation fails.
+       * configure.ac (PTY_TTY_NAME_SPRINTF): Use PTY_NAME_SIZE,
+       not sizeof pty_name, since pty_name is now a pointer to the array.
+
 2013-07-13  Paul Eggert  <address@hidden>
 
        * configure.ac: Simplify --with-file-notification handling.

=== modified file 'configure.ac'
--- a/configure.ac      2013-07-13 00:01:43 +0000
+++ b/configure.ac      2013-07-20 15:33:00 +0000
@@ -3938,7 +3938,7 @@
       AC_DEFINE(PTY_ITERATION, [int i; for (i = 0; i < 1; i++)])
       dnl Note that grantpt and unlockpt may fork.  We must block SIGCHLD
       dnl to prevent sigchld_handler from intercepting the child's death.
-      AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptyname = 0; sigset_t blocked; 
sigemptyset (&blocked); sigaddset (&blocked, SIGCHLD); pthread_sigmask 
(SIG_BLOCK, &blocked, 0); if (grantpt (fd) != -1 && unlockpt (fd) != -1) 
ptyname = ptsname(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if 
(!ptyname) { emacs_close (fd); return -1; } snprintf (pty_name, sizeof 
pty_name, "%s", ptyname); }])
+      AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptyname = 0; sigset_t blocked; 
sigemptyset (&blocked); sigaddset (&blocked, SIGCHLD); pthread_sigmask 
(SIG_BLOCK, &blocked, 0); if (grantpt (fd) != -1 && unlockpt (fd) != -1) 
ptyname = ptsname(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if 
(!ptyname) { emacs_close (fd); return -1; } snprintf (pty_name, PTY_NAME_SIZE, 
"%s", ptyname); }])
       dnl if HAVE_POSIX_OPENPT
       if test "x$ac_cv_func_posix_openpt" = xyes; then
         AC_DEFINE(PTY_OPEN, [fd = posix_openpt (O_RDWR | O_CLOEXEC | 
O_NOCTTY)])
@@ -3986,12 +3986,12 @@
     dnl On SysVr4, grantpt(3) forks a subprocess, so keep sigchld_handler()
     dnl from intercepting that death.  If any child but grantpt's should die
     dnl within, it should be caught after sigrelse(2).
-    AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptsname (int), *ptyname; int 
grantpt_result; sigset_t blocked; sigemptyset (&blocked); sigaddset (&blocked, 
SIGCHLD); pthread_sigmask (SIG_BLOCK, &blocked, 0); grantpt_result = grantpt 
(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if (grantpt_result == -1 || 
unlockpt (fd) == -1 || !(ptyname = ptsname (fd))) { emacs_close (fd); return 
-1; } snprintf (pty_name, sizeof pty_name, "%s", ptyname); }])
+    AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptsname (int), *ptyname; int 
grantpt_result; sigset_t blocked; sigemptyset (&blocked); sigaddset (&blocked, 
SIGCHLD); pthread_sigmask (SIG_BLOCK, &blocked, 0); grantpt_result = grantpt 
(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if (grantpt_result == -1 || 
unlockpt (fd) == -1 || !(ptyname = ptsname (fd))) { emacs_close (fd); return 
-1; } snprintf (pty_name, PTY_NAME_SIZE, "%s", ptyname); }])
     ;;
 
   unixware )
     dnl Comments are as per sol2*.
-    AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptsname (int), *ptyname; int 
grantpt_result; sigset_t blocked; sigemptyset (&blocked); sigaddset (&blocked, 
SIGCHLD); pthread_sigmask (SIG_BLOCK, &blocked, 0); grantpt_result = grantpt 
(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if (grantpt_result == -1) 
fatal("could not grant slave pty"); if (unlockpt(fd) == -1) fatal("could not 
unlock slave pty"); if (!(ptyname = ptsname(fd))) fatal ("could not enable 
slave pty"); snprintf (pty_name, sizeof pty_name, "%s", ptyname); }])
+    AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptsname (int), *ptyname; int 
grantpt_result; sigset_t blocked; sigemptyset (&blocked); sigaddset (&blocked, 
SIGCHLD); pthread_sigmask (SIG_BLOCK, &blocked, 0); grantpt_result = grantpt 
(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if (grantpt_result == -1) 
fatal("could not grant slave pty"); if (unlockpt(fd) == -1) fatal("could not 
unlock slave pty"); if (!(ptyname = ptsname(fd))) fatal ("could not enable 
slave pty"); snprintf (pty_name, PTY_NAME_SIZE, "%s", ptyname); }])
     ;;
 esac
 

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog     2013-07-20 14:21:25 +0000
+++ b/src/ChangeLog     2013-07-20 15:33:00 +0000
@@ -1,5 +1,15 @@
 2013-07-20  Paul Eggert  <address@hidden>
 
+       Fix array bounds violation when pty allocation fails.
+       * process.c (PTY_NAME_SIZE): New constant.
+       (pty_name): Remove static variable; it's now auto.
+       (allocate_pty): Define even if !HAVE_PTYS; that's simpler.
+       Take pty_name as an arg rather than using a static variable.
+       All callers changed.
+       (create_process): Recover pty_flag from process, not from volatile 
local.
+       (create_pty): Stay inside array even when pty allocation fails.
+       (Fmake_serial_process): Omit unnecessary initializaiton of pty_flag.
+
        * lread.c (Fload): Avoid initialization only when lint checking.
        Mention that it's needed only for older GCCs.
 

=== modified file 'src/process.c'
--- a/src/process.c     2013-07-19 18:09:23 +0000
+++ b/src/process.c     2013-07-20 15:33:00 +0000
@@ -640,19 +640,16 @@
     return Fcopy_sequence (Fsymbol_name (symbol));
 }
 
-#ifdef HAVE_PTYS
-
-/* The file name of the pty opened by allocate_pty.  */
-static char pty_name[24];
+enum { PTY_NAME_SIZE = 24 };
 
 /* Open an available pty, returning a file descriptor.
-   Return -1 on failure.
-   The file name of the terminal corresponding to the pty
-   is left in the variable pty_name.  */
+   Store into PTY_NAME the file name of the terminal corresponding to the pty.
+   Return -1 on failure.  */
 
 static int
-allocate_pty (void)
+allocate_pty (char pty_name[PTY_NAME_SIZE])
 {
+#ifdef HAVE_PTYS
   int fd;
 
 #ifdef PTY_ITERATION
@@ -697,9 +694,9 @@
            return fd;
          }
       }
+#endif /* HAVE_PTYS */
   return -1;
 }
-#endif /* HAVE_PTYS */
 
 static Lisp_Object
 make_process (Lisp_Object name)
@@ -1621,14 +1618,14 @@
 #endif
   int forkin, forkout;
   bool pty_flag = 0;
+  char pty_name[PTY_NAME_SIZE];
   Lisp_Object lisp_pty_name = Qnil;
   Lisp_Object encoded_current_dir;
 
   inchannel = outchannel = -1;
 
-#ifdef HAVE_PTYS
   if (!NILP (Vprocess_connection_type))
-    outchannel = inchannel = allocate_pty ();
+    outchannel = inchannel = allocate_pty (pty_name);
 
   if (inchannel >= 0)
     {
@@ -1647,7 +1644,6 @@
       lisp_pty_name = build_string (pty_name);
     }
   else
-#endif /* HAVE_PTYS */
     {
       if (emacs_pipe (sv) != 0)
        report_file_error ("Creating pipe", Qnil);
@@ -1704,7 +1700,6 @@
     Lisp_Object volatile encoded_current_dir_volatile = encoded_current_dir;
     Lisp_Object volatile lisp_pty_name_volatile = lisp_pty_name;
     Lisp_Object volatile process_volatile = process;
-    bool volatile pty_flag_volatile = pty_flag;
     char **volatile new_argv_volatile = new_argv;
     int volatile forkin_volatile = forkin;
     int volatile forkout_volatile = forkout;
@@ -1716,12 +1711,13 @@
     encoded_current_dir = encoded_current_dir_volatile;
     lisp_pty_name = lisp_pty_name_volatile;
     process = process_volatile;
-    pty_flag = pty_flag_volatile;
     new_argv = new_argv_volatile;
     forkin = forkin_volatile;
     forkout = forkout_volatile;
     wait_child_setup[0] = wait_child_setup_0_volatile;
     wait_child_setup[1] = wait_child_setup_1_volatile;
+
+    pty_flag = XPROCESS (process)->pty_flag;
   }
 
   if (pid == 0)
@@ -1791,15 +1787,15 @@
       if (pty_flag)
        {
 
-         /* I wonder if emacs_close (emacs_open (pty_name, ...))
+         /* I wonder if emacs_close (emacs_open (SSDATA (lisp_pty_name), ...))
             would work?  */
          if (xforkin >= 0)
            emacs_close (xforkin);
-         xforkout = xforkin = emacs_open (pty_name, O_RDWR, 0);
+         xforkout = xforkin = emacs_open (SSDATA (lisp_pty_name), O_RDWR, 0);
 
          if (xforkin < 0)
            {
-             emacs_perror (pty_name);
+             emacs_perror (SSDATA (lisp_pty_name));
              _exit (EXIT_CANCELED);
            }
 
@@ -1899,17 +1895,16 @@
     }
 }
 
-void
+static void
 create_pty (Lisp_Object process)
 {
+  char pty_name[PTY_NAME_SIZE];
   int inchannel, outchannel;
-  bool pty_flag = 0;
 
   inchannel = outchannel = -1;
 
-#ifdef HAVE_PTYS
   if (!NILP (Vprocess_connection_type))
-    outchannel = inchannel = allocate_pty ();
+    outchannel = inchannel = allocate_pty (pty_name);
 
   if (inchannel >= 0)
     {
@@ -1928,40 +1923,34 @@
       child_setup_tty (forkout);
 #endif /* DONT_REOPEN_PTY */
 #endif /* not USG, or USG_SUBTTY_WORKS */
-      pty_flag = 1;
+
+      fcntl (inchannel, F_SETFL, O_NONBLOCK);
+      fcntl (outchannel, F_SETFL, O_NONBLOCK);
+
+      /* Record this as an active process, with its channels.
+        As a result, child_setup will close Emacs's side of the pipes.  */
+      chan_process[inchannel] = process;
+      XPROCESS (process)->infd = inchannel;
+      XPROCESS (process)->outfd = outchannel;
+
+      /* Previously we recorded the tty descriptor used in the subprocess.
+        It was only used for getting the foreground tty process, so now
+        we just reopen the device (see emacs_get_tty_pgrp) as this is
+        more portable (see USG_SUBTTY_WORKS above).  */
+
+      XPROCESS (process)->pty_flag = 1;
+      pset_status (XPROCESS (process), Qrun);
+      setup_process_coding_systems (process);
+
+      FD_SET (inchannel, &input_wait_mask);
+      FD_SET (inchannel, &non_keyboard_wait_mask);
+      if (inchannel > max_process_desc)
+       max_process_desc = inchannel;
+
+      pset_tty_name (XPROCESS (process), build_string (pty_name));
     }
-#endif /* HAVE_PTYS */
-
-  fcntl (inchannel, F_SETFL, O_NONBLOCK);
-  fcntl (outchannel, F_SETFL, O_NONBLOCK);
-
-  /* Record this as an active process, with its channels.
-     As a result, child_setup will close Emacs's side of the pipes.  */
-  chan_process[inchannel] = process;
-  XPROCESS (process)->infd = inchannel;
-  XPROCESS (process)->outfd = outchannel;
-
-  /* Previously we recorded the tty descriptor used in the subprocess.
-     It was only used for getting the foreground tty process, so now
-     we just reopen the device (see emacs_get_tty_pgrp) as this is
-     more portable (see USG_SUBTTY_WORKS above).  */
-
-  XPROCESS (process)->pty_flag = pty_flag;
-  pset_status (XPROCESS (process), Qrun);
-  setup_process_coding_systems (process);
-
-  FD_SET (inchannel, &input_wait_mask);
-  FD_SET (inchannel, &non_keyboard_wait_mask);
-  if (inchannel > max_process_desc)
-    max_process_desc = inchannel;
 
   XPROCESS (process)->pid = -2;
-#ifdef HAVE_PTYS
-  if (pty_flag)
-    pset_tty_name (XPROCESS (process), build_string (pty_name));
-  else
-#endif
-    pset_tty_name (XPROCESS (process), Qnil);
 }
 
 
@@ -2589,7 +2578,7 @@
     p->kill_without_query = 1;
   if (tem = Fplist_get (contact, QCstop), !NILP (tem))
     pset_command (p, Qt);
-  p->pty_flag = 0;
+  eassert (! p->pty_flag);
 
   if (!EQ (p->command, Qt))
     {