diff mbox

PPC sim: Don't close file descriptors 0, 1, or 2

Message ID 20151207132910.71bf8289@pinnacle.lan
State Superseded
Delegated to: Mike Frysinger
Headers show

Commit Message

Kevin Buettner Dec. 7, 2015, 8:29 p.m. UTC
On Tue, 17 Nov 2015 16:05:40 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On 17 Nov 2015 13:31, Kevin Buettner wrote:
> > On Tue, 17 Nov 2015 00:41:33 -0500 Mike Frysinger wrote:
> > > > So sim/common is doing the same thing as my proposed patch for ppc;
> > > > sim/common is just using a more elegant mechanism to avoid calling
> > > > close() on these three file descriptors.
> > > 
> > > the difference is that this code sequence misbehaves after your change:
> > > 	close(1);
> > > 	write(1, "foo", 3);
> > > under the common sim, the write will return EBADF.
> > > 
> > > considering how much of common/ came from ppc/ i'm a little surprised
> > > virtualization of the fd table didn't.
> > > 
> > > it would be nice if we could at least hide these three fds (a static
> > > array of 3 bools maybe?), but i won't push hard for you to do that.
> > 
> > Do you mean an array which indicates the open / closed status of each
> > of stdin, stdout, and stderr?  This status would then be used to
> > return EBADF in the right places when the descriptor is "closed".
> 
> correct.  maybe add a helper func like the common code does, and then
> have every wrapper (read, write, etc...) check that before making the
> actual call.  i don't think we have to get fancy and preserve exact
> behavior since the standard does not require it; i.e. this code:
> 	close(2)
> 	int fd = open(...)
> fd would normally be 2, but since we haven't really closed it, you'd
> get back a higher fd.

Below is a patch which adds and uses the helper function, fdbad, which
mimics (as much as makes sense) what is done in sim/common.

This adds on to my earlier patch:

https://sourceware.org/ml/gdb-patches/2015-11/msg00354.html

I.e. my earlier patch must be applied first before applying the patch
below.

    ppc sim: Track closed state of file descriptors 0, 1, and 2.
    
    This change tracks the "closed" state of file descriptors 0, 1, and 2,
    introducing the function fdbad() to emul_netbsd.c and emul_unix.c.
    Note that a function of the same name and purpose exists in
    sim/common/callback.c.
    
    sim/ppc/ChangeLog:
    
    	* emul_netbsd.c (fd_closed): New static array.
    	(fdbad): New function.
    	(do_read, do_write, do_close, do_dup, do_ioctl, do_dup2, do_fcntl)
    	(do_fstatfs, do_fstat, do_lseek): Call `fdbad'.
    	(emul_netbsd_init): Initialize `fd_closed'.
    	* emul_unix.c (fd_closed): New static array.
    	(fdbad): New function.
    	(do_unix_read, do_unix_write, do_unix_close, do_unix_dup)
    	(do_unix_dup2, do_unix_lseek, do_solaris_fstat, do_solaris_ioctl)
    	(do_linux_fstat, do_linux_ioctl): Call `fdbad'.
    	(emul_solaris_init, emul_linux_init): Initialize `fd_closed'.
---
 sim/ppc/emul_netbsd.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
 sim/ppc/emul_unix.c   | 79 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 131 insertions(+), 29 deletions(-)
diff mbox

Patch

diff --git a/sim/ppc/emul_netbsd.c b/sim/ppc/emul_netbsd.c
index 5e39975..c50fea0 100644
--- a/sim/ppc/emul_netbsd.c
+++ b/sim/ppc/emul_netbsd.c
@@ -292,6 +292,31 @@  write_rusage(unsigned_word addr,
 }
 #endif
 
+
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+   
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
 static void
 do_exit(os_emul_data *emul,
 	unsigned call,
@@ -339,7 +364,9 @@  do_read(os_emul_data *emul,
       status = -1;
   }
 #endif
-  status = read (d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -374,7 +401,10 @@  do_write(os_emul_data *emul,
 		   processor, cia);
 
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
+
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -440,12 +470,19 @@  do_close(os_emul_data *emul,
 
   SYS(close);
 
-  /* Do not close stdin, stdout, or stderr. GDB may still need access to
-     these descriptors.  */
-  if (d == 0 || d == 1 || d == 2)
-    status = 0;
-  else
-    status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      /* Do not close stdin, stdout, or stderr. GDB may still need access to
+	 these descriptors.  */
+      if (d == 0 || d == 1 || d == 2)
+        {
+	  fd_closed[d] = 1;
+	  status = 0;
+	}
+      else
+	status = close(d);
+    }
 
   emul_write_status(processor, status, errno);
 }
@@ -554,7 +591,7 @@  do_dup(os_emul_data *emul,
        unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -645,7 +682,9 @@  do_ioctl(os_emul_data *emul,
       || dir & IOC_OUT
       || !(dir & IOC_VOID))
     error("do_ioctl() read or write of parameter not implemented\n");
-  status = ioctl(d, request, NULL);
+  status = fdbad (d);
+  if (status == 0)
+    status = ioctl(d, request, NULL);
   emul_write_status(processor, status, errno);
 #endif
 
@@ -686,7 +725,7 @@  do_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -716,7 +755,9 @@  do_fcntl(os_emul_data *emul,
     printf_filtered ("%d, %d, %d", fd, cmd, arg);
 
   SYS(fcntl);
-  status = fcntl(fd, cmd, arg);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fcntl(fd, cmd, arg);
   emul_write_status(processor, status, errno);
 }
 #endif
@@ -801,7 +842,9 @@  do_fstatfs(os_emul_data *emul,
     printf_filtered ("%d, 0x%lx", fd, (long)buf_addr);
 
   SYS(fstatfs);
-  status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
   emul_write_status(processor, status, errno);
   if (status == 0) {
     if (buf_addr != 0)
@@ -854,7 +897,9 @@  do_fstat(os_emul_data *emul,
   SYS(fstat);
 #endif
   /* Can't combine these statements, cuz fstat sets errno. */
-  status = fstat(fd, &buf);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstat(fd, &buf);
   emul_write_status(processor, status, errno);
   write_stat(stat_buf_addr, buf, processor, cia);
 }
@@ -956,7 +1001,9 @@  do_lseek(os_emul_data *emul,
   int whence = cpu_registers(processor)->gpr[arg0+4];
   off_t status;
   SYS(lseek);
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   if (status == -1)
     emul_write_status(processor, -1, errno);
   else {
@@ -1455,7 +1502,9 @@  static void
 emul_netbsd_init(os_emul_data *emul_data,
 		 int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
diff --git a/sim/ppc/emul_unix.c b/sim/ppc/emul_unix.c
index 47b2b98..a884e24 100644
--- a/sim/ppc/emul_unix.c
+++ b/sim/ppc/emul_unix.c
@@ -195,6 +195,31 @@  struct	unix_rusage {
 };
 
 
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+   
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
+
 static void
 do_unix_exit(os_emul_data *emul,
 	     unsigned call,
@@ -232,8 +257,10 @@  do_unix_read(os_emul_data *emul,
   /* check if buffer exists by reading it */
   emul_read_buffer(scratch_buffer, buf, nbytes, processor, cia);
 
+  status = fdbad (d);
   /* read */
-  status = read (d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -266,8 +293,10 @@  do_unix_write(os_emul_data *emul,
   emul_read_buffer(scratch_buffer, buf, nbytes,
 		   processor, cia);
 
+  status = fdbad (d);
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -310,10 +339,14 @@  do_unix_close(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d", d);
 
-  if (d == 0 || d == 1 || d == 2)
-    status = 0;
-  else
-    status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      if (d == 0 || d == 1 || d == 2)
+	status = 0;
+      else
+	status = close(d);
+    }
 
   emul_write_status(processor, status, errno);
 }
@@ -494,7 +527,7 @@  do_unix_dup(os_emul_data *emul,
 	    unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -516,7 +549,7 @@  do_unix_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -544,7 +577,9 @@  do_unix_lseek(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d %ld %d", fildes, (long)offset, whence);
 
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   emul_write_status(processor, (int)status, errno);
 }
 #endif
@@ -1200,7 +1235,9 @@  do_solaris_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_solaris_stat (stat_pkt, &buf, processor, cia);
 
@@ -1437,6 +1474,10 @@  do_solaris_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -1478,6 +1519,7 @@  do_solaris_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -1929,7 +1971,9 @@  static void
 emul_solaris_init(os_emul_data *emul_data,
 		  int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
@@ -2128,7 +2172,9 @@  do_linux_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_linux_stat (stat_pkt, &buf, processor, cia);
 
@@ -2392,6 +2438,10 @@  do_linux_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -2433,6 +2483,7 @@  do_linux_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -2799,7 +2850,9 @@  static void
 emul_linux_init(os_emul_data *emul_data,
 		int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void