]> Zhao Yanbai Git Server - minix.git/commitdiff
VFS: unbreak select on /dev/tty 87/2787/1
authorDavid van Moolenbroek <david@minix3.org>
Tue, 26 Aug 2014 22:28:58 +0000 (22:28 +0000)
committerDavid van Moolenbroek <david@minix3.org>
Thu, 28 Aug 2014 16:30:48 +0000 (16:30 +0000)
The remapping from /dev/tty to the real controlling terminal in the
device code was confusing the select code.  The latter is now aware
of this case and should handle it properly, at the cost of one extra
field in the filp structure.

There is a nasty, hopefully sufficiently rare case of /dev/tty being
kept open while controlling terminals are changing, that we are still
not handling.  Doing so would require more than just a few changes,
but the code should at least detect and cleanly fail on this case.

Test77 now has a basic test set for selecting on /dev/tty.

Change-Id: Iaedea449cdb728d0e66a9de8faacdfd9638dfe92

minix/servers/vfs/device.c
minix/servers/vfs/file.h
minix/servers/vfs/filedes.c
minix/servers/vfs/misc.c
minix/servers/vfs/proto.h
minix/servers/vfs/select.c
minix/tests/test77.c

index c76951471866166dec70fcbb959b859f9c82b285..7d34c0479d2d5e40645e1c940683f53885561592 100644 (file)
@@ -200,39 +200,57 @@ static cp_grant_id_t make_grant(endpoint_t driver_e, endpoint_t user_e, int op,
 }
 
 /*===========================================================================*
- *                             cdev_get                                     *
+ *                             cdev_map                                     *
  *===========================================================================*/
-static struct dmap *cdev_get(dev_t dev, devminor_t *minor_dev)
+dev_t cdev_map(dev_t dev, struct fproc *rfp)
 {
-/* Obtain the dmap structure for the given device, if a valid driver exists for
- * the major device. Perform redirection for CTTY_MAJOR.
+/* Map the given device number to a real device number, remapping /dev/tty to
+ * the given process's controlling terminal if it has one. Perform a bounds
+ * check on the resulting device's major number, and return NO_DEV on failure.
+ * This function is idempotent but not used that way.
  */
-  devmajor_t major_dev;
-  struct dmap *dp;
-  int slot;
+  devmajor_t major;
 
   /* First cover one special case: /dev/tty, the magic device that translates
    * to the controlling tty.
    */
-  if (major(dev) == CTTY_MAJOR) {
+  if ((major = major(dev)) == CTTY_MAJOR) {
        /* No controlling terminal? Fail the request. */
-       if (fp->fp_tty == 0) return(NULL);
+       if (rfp->fp_tty == NO_DEV) return NO_DEV;
 
        /* Substitute the controlling terminal device. */
-       dev = fp->fp_tty;
+       dev = rfp->fp_tty;
+       major = major(dev);
   }
 
-  /* Determine task dmap. */
-  major_dev = major(dev);
-  if (major_dev < 0 || major_dev >= NR_DEVICES) return(NULL);
+  if (major < 0 || major >= NR_DEVICES) return NO_DEV;
 
-  dp = &dmap[major_dev];
+  return dev;
+}
+
+/*===========================================================================*
+ *                             cdev_get                                     *
+ *===========================================================================*/
+static struct dmap *cdev_get(dev_t dev, devminor_t *minor_dev)
+{
+/* Obtain the dmap structure for the given device, if a valid driver exists for
+ * the major device. Perform redirection for CTTY_MAJOR.
+ */
+  struct dmap *dp;
+  int slot;
+
+  /* Remap /dev/tty as needed. Perform a bounds check on the major number. */
+  if ((dev = cdev_map(dev, fp)) == NO_DEV)
+       return(NULL);
+
+  /* Determine task dmap. */
+  dp = &dmap[major(dev)];
 
   /* See if driver is roughly valid. */
   if (dp->dmap_driver == NONE) return(NULL);
 
   if (isokendpt(dp->dmap_driver, &slot) != OK) {
-       printf("VFS: cdev_get: old driver for major %x (%d)\n", major_dev,
+       printf("VFS: cdev_get: old driver for major %x (%d)\n", major(dev),
                dp->dmap_driver);
        return(NULL);
   }
@@ -531,21 +549,27 @@ int do_ioctl(void)
  *===========================================================================*/
 int cdev_select(dev_t dev, int ops)
 {
-/* Initiate a select call on a device. Return OK iff the request was sent. */
-  devminor_t minor_dev;
+/* Initiate a select call on a device. Return OK iff the request was sent.
+ * This function explicitly bypasses cdev_get() since it must not do CTTY
+ * mapping, because a) the caller already has done that, b) "fp" may be wrong.
+ */
+  devmajor_t major;
   message dev_mess;
   struct dmap *dp;
   int r;
 
-  /* Determine task dmap. */
-  if ((dp = cdev_get(dev, &minor_dev)) == NULL)
-       return(EIO);
+  /* Determine task dmap, without CTTY mapping. */
+  assert(dev != NO_DEV);
+  major = major(dev);
+  assert(major >= 0 && major < NR_DEVICES);
+  assert(major != CTTY_MAJOR);
+  dp = &dmap[major];
 
   /* Prepare the request message. */
   memset(&dev_mess, 0, sizeof(dev_mess));
 
   dev_mess.m_type = CDEV_SELECT;
-  dev_mess.m_vfs_lchardriver_select.minor = minor_dev;
+  dev_mess.m_vfs_lchardriver_select.minor = minor(dev);
   dev_mess.m_vfs_lchardriver_select.ops = ops;
 
   /* Send the request to the driver. */
index efbe80d97584ba3f3dde7c48e883862fb24b8f11..a1a4a51e0a7778a6ed78a52c74a65150af189b88 100644 (file)
@@ -29,6 +29,7 @@ EXTERN struct filp {
 
   /* following are for fd-type-specific select() */
   int filp_pipe_select_ops;
+  dev_t filp_char_select_dev;
 } filp[NR_FILPS];
 
 #define FILP_CLOSED    0       /* filp_mode: associated device closed/gone */
index a810feb2f47354ad4def624c92450723cba90707..6adc70386d5839793e7fe65278c2c2601eebab46 100644 (file)
@@ -119,6 +119,7 @@ int get_fd(struct fproc *rfp, int start, mode_t bits, int *k, struct filp **fpt)
                f->filp_selectors = 0;
                f->filp_select_ops = 0;
                f->filp_pipe_select_ops = 0;
+               f->filp_char_select_dev = NO_DEV;
                f->filp_flags = 0;
                f->filp_select_flags = 0;
                f->filp_softlock = NULL;
index 65617d2a378d27ef50e4c19aaea3114d71585c7d..65d4b8630b7603e64b2ff783cd1a3a014cdf65f9 100644 (file)
@@ -668,7 +668,7 @@ static void free_proc(int flags)
                if (vp->v_sdev != dev) continue;
                lock_filp(rfilp, VNODE_READ);
                (void) cdev_close(dev); /* Ignore any errors. */
-
+               /* FIXME: missing select check */
                rfilp->filp_mode = FILP_CLOSED;
                unlock_filp(rfilp);
           }
index ad0449dc9d233a89e99bb045d152e1e7025f34a9..051a807b6b31d50fe9302c82de7355cc3ae2b28c 100644 (file)
@@ -34,6 +34,7 @@ int cdev_open(dev_t dev, int flags);
 int cdev_close(dev_t dev);
 int cdev_io(int op, dev_t dev, endpoint_t proc_e, vir_bytes buf, off_t pos,
        unsigned long bytes, int flags);
+dev_t cdev_map(dev_t dev, struct fproc *rfp);
 int cdev_select(dev_t dev, int ops);
 int cdev_cancel(dev_t dev);
 void cdev_reply(void);
index 66ff4ffadd3050ce2a692ceed421bc6617c990a8..5bf2a1171c553b3d01f9cb67c15f99eb08db007a 100644 (file)
@@ -57,9 +57,12 @@ static int is_regular_file(struct filp *f);
 static int is_pipe(struct filp *f);
 static int is_char_device(struct filp *f);
 static void select_lock_filp(struct filp *f, int ops);
-static int select_request_file(struct filp *f, int *ops, int block);
-static int select_request_char(struct filp *f, int *ops, int block);
-static int select_request_pipe(struct filp *f, int *ops, int block);
+static int select_request_file(struct filp *f, int *ops, int block,
+       struct fproc *rfp);
+static int select_request_char(struct filp *f, int *ops, int block,
+       struct fproc *rfp);
+static int select_request_pipe(struct filp *f, int *ops, int block,
+       struct fproc *rfp);
 static void select_cancel_all(struct selectentry *e);
 static void select_cancel_filp(struct filp *f);
 static void select_return(struct selectentry *);
@@ -68,7 +71,8 @@ static int tab2ops(int fd, struct selectentry *e);
 static void wipe_select(struct selectentry *s);
 
 static struct fdtype {
-       int (*select_request)(struct filp *, int *ops, int block);
+       int (*select_request)(struct filp *, int *ops, int block,
+               struct fproc *rfp);
        int (*type_match)(struct filp *f);
 } fdtypes[] = {
        { select_request_char, is_char_device },
@@ -237,7 +241,7 @@ int do_select(void)
                wantops = (f->filp_select_ops |= ops);
                type = se->type[fd];
                select_lock_filp(f, wantops);
-               r = fdtypes[type].select_request(f, &wantops, se->block);
+               r = fdtypes[type].select_request(f, &wantops, se->block, fp);
                unlock_filp(f);
                if (r != OK && r != SUSPEND) {
                        se->error = r;
@@ -355,19 +359,48 @@ static int is_char_device(struct filp *f)
 /*===========================================================================*
  *                             select_request_char                          *
  *===========================================================================*/
-static int select_request_char(struct filp *f, int *ops, int block)
+static int select_request_char(struct filp *f, int *ops, int block,
+       struct fproc *rfp)
 {
 /* Check readiness status on a character device. Unless suitable results are
  * available right now, this will only initiate the polling process, causing
  * result processing to be deferred. This function MUST NOT block its calling
  * thread. The given filp may or may not be locked.
  */
-  devmajor_t major;
+  dev_t dev;
   int r, rops;
   struct dmap *dp;
 
-  major = major(f->filp_vno->v_sdev);
-  if (major < 0 || major >= NR_DEVICES) return(ENXIO);
+  /* Start by remapping the device node number to a "real" device number. Those
+   * two are different only for CTTY_MAJOR aka /dev/tty, but that one single
+   * exception requires quite some extra effort here: the select code matches
+   * character driver replies to their requests based on the device number, so
+   * it needs to be aware that device numbers may be mapped. The idea is to
+   * perform the mapping once and store the result in the filp object, so that
+   * at least we don't run into problems when a process loses its controlling
+   * terminal while doing a select (see also free_proc). It should be noted
+   * that it is possible that multiple processes share the same /dev/tty filp,
+   * and they may not all have a controlling terminal. The ctty-less processes
+   * should never pass the mapping; a more problematic case is checked below.
+   *
+   * The cdev_map call also checks the major number for rough validity, so that
+   * we can use it to index the dmap array safely a bit later.
+   */
+  if ((dev = cdev_map(f->filp_vno->v_sdev, rfp)) == NO_DEV)
+       return(ENXIO);
+
+  if (f->filp_char_select_dev != NO_DEV && f->filp_char_select_dev != dev) {
+       /* Currently, this case can occur as follows: a process with a
+        * controlling terminal opens /dev/tty and forks, the new child starts
+        * a new session, opens a new controlling terminal, and both parent and
+        * child call select on the /dev/tty file descriptor. If this case ever
+        * becomes real, a better solution may be to force-close a filp for
+        * /dev/tty when a new controlling terminal is opened.
+        */
+       printf("VFS: file pointer has multiple controlling TTYs!\n");
+       return(EIO);
+  }
+  f->filp_char_select_dev = dev; /* set before possibly suspending */
 
   rops = *ops;
 
@@ -401,12 +434,12 @@ static int select_request_char(struct filp *f, int *ops, int block)
   if (f->filp_select_flags & FSF_BUSY)
        return(SUSPEND);
 
-  dp = &dmap[major];
+  dp = &dmap[major(dev)];
   if (dp->dmap_sel_busy)
        return(SUSPEND);
 
   f->filp_select_flags &= ~FSF_UPDATE;
-  r = cdev_select(f->filp_vno->v_sdev, rops);
+  r = cdev_select(dev, rops);
   if (r != OK)
        return(r);
 
@@ -421,7 +454,7 @@ static int select_request_char(struct filp *f, int *ops, int block)
  *                             select_request_file                          *
  *===========================================================================*/
 static int select_request_file(struct filp *UNUSED(f), int *UNUSED(ops),
-  int UNUSED(block))
+  int UNUSED(block), struct fproc *UNUSED(rfp))
 {
   /* Files are always ready, so output *ops is input *ops */
   return(OK);
@@ -430,7 +463,8 @@ static int select_request_file(struct filp *UNUSED(f), int *UNUSED(ops),
 /*===========================================================================*
  *                             select_request_pipe                          *
  *===========================================================================*/
-static int select_request_pipe(struct filp *f, int *ops, int block)
+static int select_request_pipe(struct filp *f, int *ops, int block,
+       struct fproc *UNUSED(rfp))
 {
 /* Check readiness status on a pipe. The given filp is locked. This function
  * may block its calling thread if necessary.
@@ -612,13 +646,15 @@ static void select_cancel_filp(struct filp *f)
 
        /* If this filp is the subject of an ongoing select query to a
         * character device, mark the query as stale, so that this filp will
-        * not be checked when the result arrives.
+        * not be checked when the result arrives. The filp select device may
+        * still be NO_DEV if do_select fails on the initial fd check.
         */
-       if (is_char_device(f)) {
-               major = major(f->filp_vno->v_sdev);
+       if (is_char_device(f) && f->filp_char_select_dev != NO_DEV) {
+               major = major(f->filp_char_select_dev);
                if (dmap[major].dmap_sel_busy &&
                        dmap[major].dmap_sel_filp == f)
                        dmap[major].dmap_sel_filp = NULL; /* leave _busy set */
+               f->filp_char_select_dev = NO_DEV;
        }
   }
 }
@@ -746,10 +782,11 @@ void select_unsuspend_by_endpt(endpoint_t proc_e)
        }
 
        for (fd = 0; fd < se->nfds; fd++) {
-               if ((f = se->filps[fd]) == NULL || f->filp_vno == NULL)
+               if ((f = se->filps[fd]) == NULL || !is_char_device(f))
                        continue;
 
-               major = major(f->filp_vno->v_sdev);
+               assert(f->filp_char_select_dev != NO_DEV);
+               major = major(f->filp_char_select_dev);
                if (dmap_driver_match(proc_e, major)) {
                        se->filps[fd] = NULL;
                        se->error = EIO;
@@ -775,7 +812,6 @@ void select_reply1(endpoint_t driver_e, devminor_t minor, int status)
   dev_t dev;
   struct filp *f;
   struct dmap *dp;
-  struct vnode *vp;
 
   /* Figure out which device is replying */
   if ((dp = get_dmap(driver_e)) == NULL) return;
@@ -796,15 +832,14 @@ void select_reply1(endpoint_t driver_e, devminor_t minor, int status)
    */
   if ((f = dp->dmap_sel_filp) != NULL) {
        /* Find vnode and check we got a reply from the device we expected */
-       vp = f->filp_vno;
-       assert(vp != NULL);
-       assert(S_ISCHR(vp->v_mode));
-       if (vp->v_sdev != dev) {
+       assert(is_char_device(f));
+       assert(f->filp_char_select_dev != NO_DEV);
+       if (f->filp_char_select_dev != dev) {
                /* This should never happen. The driver may be misbehaving.
                 * For now we assume that the reply we want will arrive later..
                 */
                printf("VFS (%s:%d): expected reply from dev %llx not %llx\n",
-                       __FILE__, __LINE__, vp->v_sdev, dev);
+                       __FILE__, __LINE__, f->filp_char_select_dev, dev);
                return;
        }
   }
@@ -872,7 +907,6 @@ void select_reply2(endpoint_t driver_e, devminor_t minor, int status)
   dev_t dev;
   struct filp *f;
   struct dmap *dp;
-  struct vnode *vp;
   struct selectentry *se;
 
   if (status == 0) {
@@ -898,9 +932,9 @@ void select_reply2(endpoint_t driver_e, devminor_t minor, int status)
        found = FALSE;
        for (fd = 0; fd < se->nfds; fd++) {
                if ((f = se->filps[fd]) == NULL) continue;
-               if ((vp = f->filp_vno) == NULL) continue;
-               if (!S_ISCHR(vp->v_mode)) continue;
-               if (vp->v_sdev != dev) continue;
+               if (!is_char_device(f)) continue;
+               assert(f->filp_char_select_dev != NO_DEV);
+               if (f->filp_char_select_dev != dev) continue;
 
                if (status > 0) {       /* Operations ready */
                        /* Clear the replied bits from the request
@@ -974,7 +1008,7 @@ static void select_restart_filps(void)
                assert(is_char_device(f));
 
                wantops = ops = f->filp_select_ops;
-               r = select_request_char(f, &wantops, se->block);
+               r = select_request_char(f, &wantops, se->block, se->requestor);
                if (r != OK && r != SUSPEND) {
                        se->error = r;
                        restart_proc(se);
index fe0afcbc08504694bab148d7ab43e049d0b357db..e1e6c72906088f69d50b0803b6b1b75dddba2339 100644 (file)
@@ -482,6 +482,157 @@ test77e(void)
        if (sigaction(SIGUSR1, &usr_oact, NULL) < 0) e(16);
 }
 
+/*
+ * Test basic select functionality on /dev/tty.  While this test should not be
+ * part of this test set, we already have all the infrastructure we need here.
+ */
+static void
+test77f(void)
+{
+       struct sigaction act, oact;
+       char c, pname[PATH_MAX], tname[PATH_MAX];
+       struct timeval tv;
+       fd_set fd_set;
+       int fd, maxfd, masterfd, slavefd;
+
+       subtest = 6;
+
+       /* We do not want to get SIGHUP signals in this test. */
+       memset(&act, 0, sizeof(act));
+       act.sa_handler = SIG_IGN;
+       if (sigaction(SIGHUP, &act, &oact) < 0) e(1);
+
+       /* Get master and slave device names for a free pseudo terminal. */
+       get_names(pname, tname);
+
+       if ((masterfd = open(pname, O_RDWR | O_NOCTTY)) < 0) e(2);
+
+       switch (fork()) {
+       case 0:
+               if (setsid() < 0) e(3);
+
+               close(masterfd);
+
+               if ((slavefd = open(tname, O_RDWR)) < 0) e(4);
+
+               if ((fd = open("/dev/tty", O_RDWR)) < 0) e(5);
+
+               make_raw(fd);
+
+               /* Without slave input, /dev/tty is not ready for reading. */
+               FD_ZERO(&fd_set);
+               FD_SET(fd, &fd_set);
+               tv.tv_sec = 0;
+               tv.tv_usec = 0;
+
+               if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(6);
+               if (FD_ISSET(fd, &fd_set)) e(7);
+
+               FD_SET(fd, &fd_set);
+               tv.tv_sec = 0;
+               tv.tv_usec = 10000;
+
+               if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(8);
+               if (FD_ISSET(fd, &fd_set)) e(9);
+
+               /* It will be ready for writing, though. */
+               FD_SET(fd, &fd_set);
+
+               if (select(fd + 1, NULL, &fd_set, NULL, NULL) != 1) e(10);
+               if (!FD_ISSET(fd, &fd_set)) e(11);
+
+               /* Test mixing file descriptors to the same terminal. */
+               FD_ZERO(&fd_set);
+               FD_SET(fd, &fd_set);
+               FD_SET(slavefd, &fd_set);
+               tv.tv_sec = 0;
+               tv.tv_usec = 10000;
+
+               maxfd = fd > slavefd ? fd : slavefd;
+               if (select(maxfd + 1, &fd_set, NULL, NULL, &tv) != 0) e(12);
+               if (FD_ISSET(fd, &fd_set)) e(13);
+               if (FD_ISSET(slavefd, &fd_set)) e(14);
+
+               /* The delayed echo on the master must wake up our select. */
+               c = 'A';
+               if (write(slavefd, &c, sizeof(c)) != sizeof(c)) e(15);
+
+               FD_ZERO(&fd_set);
+               FD_SET(fd, &fd_set);
+
+               if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(16);
+               if (!FD_ISSET(fd, &fd_set)) e(17);
+
+               /* Select must now still flag readiness for reading. */
+               tv.tv_sec = 0;
+               tv.tv_usec = 0;
+
+               if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 1) e(18);
+               if (!FD_ISSET(fd, &fd_set)) e(19);
+
+               /* That is, until we read the byte. */
+               if (read(slavefd, &c, sizeof(c)) != sizeof(c)) e(20);
+               if (c != 'B') e(21);
+
+               if (select(fd + 1, &fd_set, NULL, NULL, &tv) != 0) e(22);
+               if (FD_ISSET(fd, &fd_set)) e(23);
+
+               /* Ask the parent to close the master. */
+               c = 'C';
+               if (write(slavefd, &c, sizeof(c)) != sizeof(c)) e(24);
+
+               FD_SET(fd, &fd_set);
+
+               /* The closure must cause an EOF condition on the slave. */
+               if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(25);
+               if (!FD_ISSET(fd, &fd_set)) e(26);
+
+               if (select(fd + 1, &fd_set, NULL, NULL, NULL) != 1) e(27);
+               if (!FD_ISSET(fd, &fd_set)) e(28);
+
+               if (read(slavefd, &c, sizeof(c)) != 0) e(29);
+
+               exit(errct);
+       case -1:
+               e(30);
+       default:
+               /* Wait for the child to write something to the slave. */
+               FD_ZERO(&fd_set);
+               FD_SET(masterfd, &fd_set);
+
+               if (select(masterfd + 1, &fd_set, NULL, NULL, NULL) != 1)
+                       e(31);
+               if (!FD_ISSET(masterfd, &fd_set)) e(32);
+
+               if (read(masterfd, &c, sizeof(c)) != sizeof(c)) e(33);
+               if (c != 'A') e(34);
+
+               /* Write a reply once the child is blocked in its select. */
+               tv.tv_sec = 1;
+               tv.tv_usec = 0;
+               if (select(masterfd + 1, &fd_set, NULL, NULL, &tv) != 0)
+                       e(35);
+
+               c = 'B';
+               if (write(masterfd, &c, sizeof(c)) != sizeof(c)) e(36);
+
+               /* Wait for the child to request closing the master. */
+               if (read(masterfd, &c, sizeof(c)) != sizeof(c)) e(37);
+               if (c != 'C') e(38);
+
+               /* Close the master once the child is blocked in its select. */
+               sleep(1);
+
+               close(masterfd);
+
+               break;
+       }
+
+       if (waitchild() < 0) e(39);
+
+       if (sigaction(SIGHUP, &oact, NULL) < 0) e(28);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -500,6 +651,7 @@ main(int argc, char **argv)
                if (m & 0x04) test77c();
                if (m & 0x08) test77d();
                if (m & 0x10) test77e();
+               if (m & 0x20) test77f();
        }
 
        quit();