From patchwork Mon Dec 7 20:29:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Buettner X-Patchwork-Id: 9925 X-Patchwork-Delegate: vapier@gentoo.org Received: (qmail 54264 invoked by alias); 7 Dec 2015 20:29:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 54254 invoked by uid 89); 7 Dec 2015 20:29:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_50, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 07 Dec 2015 20:29:14 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id F411E8CF53; Mon, 7 Dec 2015 20:29:12 +0000 (UTC) Received: from pinnacle.lan (ovpn-113-161.phx2.redhat.com [10.3.113.161]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tB7KTCfM020361 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO); Mon, 7 Dec 2015 15:29:12 -0500 Date: Mon, 7 Dec 2015 13:29:10 -0700 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Mike Frysinger Subject: Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2 Message-ID: <20151207132910.71bf8289@pinnacle.lan> In-Reply-To: <20151117210540.GM31395@vapier.lan> References: <20151116145807.4aedd84f@pinnacle.lan> <20151116235317.GF31395@vapier.lan> <20151116210533.058520d2@pinnacle.lan> <20151117054133.GI31395@vapier.lan> <20151117133154.72b61a52@pinnacle.lan> <20151117210540.GM31395@vapier.lan> MIME-Version: 1.0 X-IsSubscribed: yes On Tue, 17 Nov 2015 16:05:40 -0500 Mike Frysinger 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 --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