From unknown Mon Jun 23 09:35:37 2025 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Mailer: MIME-tools 5.509 (Entity 5.509) Content-Type: text/plain; charset=utf-8 From: bug#9762 <9762@debbugs.gnu.org> To: bug#9762 <9762@debbugs.gnu.org> Subject: Status: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() Reply-To: bug#9762 <9762@debbugs.gnu.org> Date: Mon, 23 Jun 2025 16:35:37 +0000 retitle 9762 tac fails when given multiple non-seekable inputs due to misus= e of mkstemp() reassign 9762 coreutils submitter 9762 Ambrose Feinstein severity 9762 normal thanks From debbugs-submit-bounces@debbugs.gnu.org Sat Oct 15 16:57:08 2011 Received: (at submit) by debbugs.gnu.org; 15 Oct 2011 20:57:08 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFBI4-00064Y-0h for submit@debbugs.gnu.org; Sat, 15 Oct 2011 16:57:08 -0400 Received: from eggs.gnu.org ([140.186.70.92]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFB2b-0005hM-Sw for submit@debbugs.gnu.org; Sat, 15 Oct 2011 16:41:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFB1q-000613-GA for submit@debbugs.gnu.org; Sat, 15 Oct 2011 16:40:23 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD,T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from lists.gnu.org ([140.186.70.17]:53664) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFB1q-00060z-EY for submit@debbugs.gnu.org; Sat, 15 Oct 2011 16:40:22 -0400 Received: from eggs.gnu.org ([140.186.70.92]:36012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFB1p-0001Dq-Li for bug-coreutils@gnu.org; Sat, 15 Oct 2011 16:40:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFB1n-00060Z-Q7 for bug-coreutils@gnu.org; Sat, 15 Oct 2011 16:40:21 -0400 Received: from smtp-out.google.com ([216.239.44.51]:61162) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFB1n-00060K-M5 for bug-coreutils@gnu.org; Sat, 15 Oct 2011 16:40:19 -0400 Received: from wpaz5.hot.corp.google.com (wpaz5.hot.corp.google.com [172.24.198.69]) by smtp-out.google.com with ESMTP id p9FKeIpd006872 for ; Sat, 15 Oct 2011 13:40:18 -0700 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=google.com; s=beta; t=1318711218; bh=4XGSJFabByhlDY4F4H18iFr0gZA=; h=MIME-Version:Date:Message-ID:Subject:From:To:Content-Type; b=sqKJFBLWS9J0z0lzO8PkHJjzEIKuSym6BzEr2tCHrUzzrWE9/D6PPtMWR0sZNBuS2 rzxgLwyFeemmeEhaaBdPQ== DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=dkim-signature:mime-version:date:message-id:subject:from:to: content-type:x-system-of-record; b=VO2gRy0FdhvAQH2vx2r0mWUw21zSz9a3VWU5zTpvuE5K2TfkWjx071/vQDixQzloc xbeIeaA/GarArIyHhXAJQ== Received: from ywp31 (ywp31.prod.google.com [10.192.16.31]) by wpaz5.hot.corp.google.com with ESMTP id p9FKeHD5018842 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Sat, 15 Oct 2011 13:40:18 -0700 Received: by ywp31 with SMTP id 31so941698ywp.9 for ; Sat, 15 Oct 2011 13:40:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=beta; h=mime-version:date:message-id:subject:from:to:content-type :x-system-of-record; bh=c3RNZBYIiKdcXrbPUfZyJwnjQWCAsJird775EM0vSwY=; b=yl8a3gxWRvTp+OEsalBWvL2XcZHvuYXjsFqCBaPug9AYG3a1+mrizzWl/R0VrG8nmw nLDxc3JA0oRL/vpBdoNg== Received: by 10.229.94.208 with SMTP id a16mr758698qcn.95.1318711217731; Sat, 15 Oct 2011 13:40:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.94.208 with SMTP id a16mr758691qcn.95.1318711217452; Sat, 15 Oct 2011 13:40:17 -0700 (PDT) Received: by 10.229.169.212 with HTTP; Sat, 15 Oct 2011 13:40:17 -0700 (PDT) Date: Sat, 15 Oct 2011 13:40:17 -0700 Message-ID: Subject: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() From: Ambrose Feinstein To: bug-coreutils@gnu.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 140.186.70.17 X-Spam-Score: -6.0 (------) X-Debbugs-Envelope-To: submit X-Mailman-Approved-At: Sat, 15 Oct 2011 16:57:07 -0400 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -6.0 (------) Trivial reproduction: $ true | tac - - tac: cannot create temporary file in `/tmp': Invalid argument This is present in coreutils 8.14. The cause is the way "template" is reused in copy_to_temp(). The "XXXXXX" suffix is clobbered by the first call to mkstemp(), so the next call returns EINVAL. It looks like the intent is to call mkstemp() at most once and then reuse that file; for example, record_or_unlink_tempfile() will delete at most one file on exit. From debbugs-submit-bounces@debbugs.gnu.org Sat Oct 15 19:14:50 2011 Received: (at submit) by debbugs.gnu.org; 15 Oct 2011 23:14:50 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFDRJ-0000oC-Ev for submit@debbugs.gnu.org; Sat, 15 Oct 2011 19:14:50 -0400 Received: from eggs.gnu.org ([140.186.70.92]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFDRH-0000ny-6B for submit@debbugs.gnu.org; Sat, 15 Oct 2011 19:14:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFDQU-0007Mz-GO for submit@debbugs.gnu.org; Sat, 15 Oct 2011 19:13:59 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD autolearn=unavailable version=3.3.1 Received: from lists.gnu.org ([140.186.70.17]:54072) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFDQU-0007Mv-Ew for submit@debbugs.gnu.org; Sat, 15 Oct 2011 19:13:58 -0400 Received: from eggs.gnu.org ([140.186.70.92]:36864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFDQT-0000M4-H8 for bug-coreutils@gnu.org; Sat, 15 Oct 2011 19:13:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFDQS-0007Mg-JW for bug-coreutils@gnu.org; Sat, 15 Oct 2011 19:13:57 -0400 Received: from mailgw1.uni-kl.de ([131.246.120.220]:55717) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFDQS-0007MJ-Ba for bug-coreutils@gnu.org; Sat, 15 Oct 2011 19:13:56 -0400 Received: from sushi.unix-ag.uni-kl.de (sushi.unix-ag.uni-kl.de [IPv6:2001:638:208:ef34:0:ff:fe00:65]) by mailgw1.uni-kl.de (8.14.3/8.14.3/Debian-5+lenny1) with ESMTP id p9FNDqoV006406 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT) for ; Sun, 16 Oct 2011 01:13:52 +0200 Received: from sushi.unix-ag.uni-kl.de (ip6-localhost [IPv6:::1]) by sushi.unix-ag.uni-kl.de (8.14.3/8.14.3/Debian-5+lenny1) with ESMTP id p9FNDqLv010032 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Sun, 16 Oct 2011 01:13:52 +0200 Received: (from auerswal@localhost) by sushi.unix-ag.uni-kl.de (8.14.3/8.14.3/Submit) id p9FNDqJ3010031 for bug-coreutils@gnu.org; Sun, 16 Oct 2011 01:13:52 +0200 Date: Sun, 16 Oct 2011 01:13:52 +0200 From: Erik Auerswald To: bug-coreutils@gnu.org Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() Message-ID: <20111015231352.GB9762@sushi.unix-ag.uni-kl.de> Mail-Followup-To: Erik Auerswald , bug-coreutils@gnu.org References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 140.186.70.17 X-Spam-Score: -6.6 (------) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -6.6 (------) Hi, On Sat, Oct 15, 2011 at 01:40:17PM -0700, Ambrose Feinstein wrote: > Trivial reproduction: > > $ true | tac - - > tac: cannot create temporary file in `/tmp': Invalid argument > > This is present in coreutils 8.14. This is present in coreutils 8.13 as well: $ tac <(echo a) <(echo b) tac: cannot create temporary file in `/tmp': Invalid argument a $ tac --version tac (GNU coreutils) 8.13 Copyright (C) 2011 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later . This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Written by Jay Lepreau and David MacKenzie. Erik From debbugs-submit-bounces@debbugs.gnu.org Sun Oct 16 06:23:37 2011 Received: (at 9762) by debbugs.gnu.org; 16 Oct 2011 10:23:37 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFNsX-00084Q-AX for submit@debbugs.gnu.org; Sun, 16 Oct 2011 06:23:37 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFNsT-00084H-7u for 9762@debbugs.gnu.org; Sun, 16 Oct 2011 06:23:35 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 7B73B60123; Sun, 16 Oct 2011 12:22:47 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: (Ambrose Feinstein's message of "Sat, 15 Oct 2011 13:40:17 -0700") References: Date: Sun, 16 Oct 2011 12:22:47 +0200 Message-ID: <87sjmtgrrc.fsf@rho.meyering.net> Lines: 265 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.9 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.9 (--) Ambrose Feinstein wrote: > Trivial reproduction: > > $ true | tac - - > tac: cannot create temporary file in `/tmp': Invalid argument > > This is present in coreutils 8.14. > > The cause is the way "template" is reused in copy_to_temp(). The > "XXXXXX" suffix is clobbered by the first call to mkstemp(), so the > next call returns EINVAL. > > It looks like the intent is to call mkstemp() at most once and then > reuse that file; for example, record_or_unlink_tempfile() will delete > at most one file on exit. Thank you for the report. In fixing that, I made three changes: maint: tac: remove sole use of sprintf in favor of stpcpy tac: don't misbehave with multiple non-seekable inputs tac: don't leak a file descriptor for each non-seekable input Before the primary bug fix (2/3), tac could leak only one file descriptor, because that bug prevented us from opening 2nd and subsequent files. But once fixed, it would leak an FD for each "-" (or other nonseekable) command line argument. >From cdd328f232a93fb40aec25d0681ef191eaeba2da Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sun, 16 Oct 2011 10:35:56 +0200 Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of stpcpy * src/tac.c (copy_to_temp): Use stpcpy rather than sprintf. Move some declarations "down" to point of initialization. --- src/tac.c | 17 +++++++---------- 1 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/tac.c b/src/tac.c index 65ac6a6..c572862 100644 --- a/src/tac.c +++ b/src/tac.c @@ -426,20 +426,17 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) { static char *template = NULL; static char const *tempdir; - char *tempfile; - FILE *tmp; - int fd; if (template == NULL) { - char const * const Template = "%s/tacXXXXXX"; + char const * const Template = "tacXXXXXX"; tempdir = getenv ("TMPDIR"); if (tempdir == NULL) tempdir = DEFAULT_TMPDIR; - /* Subtract 2 for `%s' and add 1 for the trailing NUL byte. */ - template = xmalloc (strlen (tempdir) + strlen (Template) - 2 + 1); - sprintf (template, Template, tempdir); + /* Add 1 for the slash and one for the trailing NUL byte. */ + template = xmalloc (strlen (tempdir) + strlen (Template) + 1 + 1); + stpcpy (stpcpy (stpcpy (template, tempdir), "/"), Template); } /* FIXME: there's a small window between a successful mkstemp call @@ -451,8 +448,8 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) FIXME: clean up upon fatal signal. Don't block them, in case $TMPFILE is a remote file system. */ - tempfile = template; - fd = mkstemp (template); + char *tempfile = template; + int fd = mkstemp (template); if (fd < 0) { error (0, errno, _("cannot create temporary file in %s"), @@ -460,7 +457,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) return false; } - tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); + FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); if (! tmp) { error (0, errno, _("cannot open %s for writing"), quote (tempfile)); -- 1.7.7 >From 608f9d9d0daef48c957fe38570e5d0f293c0f1eb Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sun, 16 Oct 2011 12:07:05 +0200 Subject: [PATCH 2/3] tac: don't misbehave with multiple non-seekable inputs * src/tac.c (copy_to_temp): Do not reuse the template buffer. Instead, scribble only on a freshly-xstrdup'd copy each time. Free that buffer both here, upon failure, and ... (tac_nonseekable): ...free the buffer in caller, upon success. * tests/misc/tac-2-nonseekable: New file. * tests/Makefile.am (TESTS): Add it. * NEWS (Bug fixes): Mention it. Reported by Ambrose Feinstein in http://debbugs.gnu.org/9762. --- NEWS | 5 +++++ src/tac.c | 19 +++++++++++++++---- tests/Makefile.am | 1 + tests/misc/tac-2-nonseekable | 27 +++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 4 deletions(-) create mode 100755 tests/misc/tac-2-nonseekable diff --git a/NEWS b/NEWS index 4c8e162..3ed44b2 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,11 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] +** Bug fixes + + tac no longer fails to handle two or more non-seekable inputs + [bug introduced in coreutils-5.3.0] + * Noteworthy changes in release 8.14 (2011-10-12) [stable] diff --git a/src/tac.c b/src/tac.c index c572862..2d8d6ea 100644 --- a/src/tac.c +++ b/src/tac.c @@ -448,12 +448,13 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) FIXME: clean up upon fatal signal. Don't block them, in case $TMPFILE is a remote file system. */ - char *tempfile = template; - int fd = mkstemp (template); + char *tempfile = xstrdup (template); + int fd = mkstemp (tempfile); if (fd < 0) { error (0, errno, _("cannot create temporary file in %s"), quote (tempdir)); + free (tempfile); return false; } @@ -463,6 +464,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) error (0, errno, _("cannot open %s for writing"), quote (tempfile)); close (fd); unlink (tempfile); + free (tempfile); return false; } @@ -498,6 +500,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) Fail: fclose (tmp); + free (tempfile); return false; } @@ -509,8 +512,16 @@ tac_nonseekable (int input_fd, const char *file) { FILE *tmp_stream; char *tmp_file; - return (copy_to_temp (&tmp_stream, &tmp_file, input_fd, file) - && tac_seekable (fileno (tmp_stream), tmp_file)); + if (copy_to_temp (&tmp_stream, &tmp_file, input_fd, file)) + { + if (tac_seekable (fileno (tmp_stream), tmp_file)) + { + free (tmp_file); + return true; + } + } + + return false; } /* Print FILE in reverse, copying it to a temporary diff --git a/tests/Makefile.am b/tests/Makefile.am index 9c9a1b8..5021c18 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -274,6 +274,7 @@ TESTS = \ misc/sum-sysv \ misc/tac \ misc/tac-continue \ + misc/tac-2-nonseekable \ misc/tail \ misc/tee \ misc/tee-dash \ diff --git a/tests/misc/tac-2-nonseekable b/tests/misc/tac-2-nonseekable new file mode 100755 index 0000000..7b48773 --- /dev/null +++ b/tests/misc/tac-2-nonseekable @@ -0,0 +1,27 @@ +#!/bin/sh +# ensure that tac works with two or more non-seekable inputs + +# Copyright (C) 2011 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +print_ver_ tac + +echo x | tac - - > out 2> err || fail=1 +echo x > exp || fail=1 +compare out exp || fail=1 +compare err /dev/null || fail=1 + +Exit $fail -- 1.7.7 >From 95fa11b63f0a5c983723f02afa7c5b896e5a0a97 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sun, 16 Oct 2011 12:14:05 +0200 Subject: [PATCH 3/3] tac: don't leak a file descriptor for each non-seekable input * src/tac.c (tac_nonseekable): Call fclose after each successful call to copy_to_temp. --- src/tac.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/tac.c b/src/tac.c index 2d8d6ea..ebafa18 100644 --- a/src/tac.c +++ b/src/tac.c @@ -516,9 +516,11 @@ tac_nonseekable (int input_fd, const char *file) { if (tac_seekable (fileno (tmp_stream), tmp_file)) { + fclose (tmp_stream); free (tmp_file); return true; } + fclose (tmp_stream); } return false; -- 1.7.7 From debbugs-submit-bounces@debbugs.gnu.org Sun Oct 16 15:13:23 2011 Received: (at 9762) by debbugs.gnu.org; 16 Oct 2011 19:13:23 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFW9C-0004rX-Oj for submit@debbugs.gnu.org; Sun, 16 Oct 2011 15:13:22 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFW9A-0004rN-5R for 9762@debbugs.gnu.org; Sun, 16 Oct 2011 15:13:21 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 296D660081; Sun, 16 Oct 2011 21:12:32 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <87sjmtgrrc.fsf@rho.meyering.net> (Jim Meyering's message of "Sun, 16 Oct 2011 12:22:47 +0200") References: <87sjmtgrrc.fsf@rho.meyering.net> Date: Sun, 16 Oct 2011 21:12:31 +0200 Message-ID: <874nz8hhsw.fsf@rho.meyering.net> Lines: 88 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.9 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Jim Meyering wrote: ... > Thank you for the report. > In fixing that, I made three changes: > > maint: tac: remove sole use of sprintf in favor of stpcpy > tac: don't misbehave with multiple non-seekable inputs > tac: don't leak a file descriptor for each non-seekable input > > Before the primary bug fix (2/3), tac could leak only one file > descriptor, because that bug prevented us from opening 2nd and > subsequent files. But once fixed, it would leak an FD for each "-" > (or other nonseekable) command line argument. ... >>>From 95fa11b63f0a5c983723f02afa7c5b896e5a0a97 Mon Sep 17 00:00:00 2001 > From: Jim Meyering > Date: Sun, 16 Oct 2011 12:14:05 +0200 > Subject: [PATCH 3/3] tac: don't leak a file descriptor for each non-seekable > input > > * src/tac.c (tac_nonseekable): Call fclose after each successful > call to copy_to_temp. > --- > src/tac.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/src/tac.c b/src/tac.c > index 2d8d6ea..ebafa18 100644 > --- a/src/tac.c > +++ b/src/tac.c > @@ -516,9 +516,11 @@ tac_nonseekable (int input_fd, const char *file) > { > if (tac_seekable (fileno (tmp_stream), tmp_file)) > { > + fclose (tmp_stream); > free (tmp_file); > return true; > } > + fclose (tmp_stream); > } > > return false; There was also an error-path leak of "tmp_file", even with the patch above (which was not pushed). I've adjusted it to fix that, too: >From 3f468dfce95ae301359511e19b13658f35a90444 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sun, 16 Oct 2011 12:14:05 +0200 Subject: [PATCH] tac: don't leak a file descriptor for each non-seekable input * src/tac.c (tac_nonseekable): Call fclose and free tmp_file after each successful call to copy_to_temp. --- src/tac.c | 15 ++++++--------- 1 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/tac.c b/src/tac.c index 2d8d6ea..13b05fb 100644 --- a/src/tac.c +++ b/src/tac.c @@ -512,16 +512,13 @@ tac_nonseekable (int input_fd, const char *file) { FILE *tmp_stream; char *tmp_file; - if (copy_to_temp (&tmp_stream, &tmp_file, input_fd, file)) - { - if (tac_seekable (fileno (tmp_stream), tmp_file)) - { - free (tmp_file); - return true; - } - } + if (!copy_to_temp (&tmp_stream, &tmp_file, input_fd, file)) + return false; - return false; + bool ok = tac_seekable (fileno (tmp_stream), tmp_file); + fclose (tmp_stream); + free (tmp_file); + return ok; } /* Print FILE in reverse, copying it to a temporary -- 1.7.7 From debbugs-submit-bounces@debbugs.gnu.org Mon Oct 17 11:19:24 2011 Received: (at 9762) by debbugs.gnu.org; 17 Oct 2011 15:19:24 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFoyK-0002mx-BA for submit@debbugs.gnu.org; Mon, 17 Oct 2011 11:19:24 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFoyH-0002mo-7W for 9762@debbugs.gnu.org; Mon, 17 Oct 2011 11:19:22 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 751FA60081; Mon, 17 Oct 2011 17:18:28 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <87sjmtgrrc.fsf@rho.meyering.net> (Jim Meyering's message of "Sun, 16 Oct 2011 12:22:47 +0200") References: <87sjmtgrrc.fsf@rho.meyering.net> Date: Mon, 17 Oct 2011 17:18:28 +0200 Message-ID: <87fwirejej.fsf@rho.meyering.net> Lines: 137 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Jim Meyering wrote: > Ambrose Feinstein wrote: >> Trivial reproduction: >> >> $ true | tac - - >> tac: cannot create temporary file in `/tmp': Invalid argument >> >> This is present in coreutils 8.14. >> >> The cause is the way "template" is reused in copy_to_temp(). The >> "XXXXXX" suffix is clobbered by the first call to mkstemp(), so the >> next call returns EINVAL. >> >> It looks like the intent is to call mkstemp() at most once and then >> reuse that file; for example, record_or_unlink_tempfile() will delete >> at most one file on exit. > > Thank you for the report. > In fixing that, I made three changes: > > maint: tac: remove sole use of sprintf in favor of stpcpy ... I realized that this part of the first patch wasn't quite right... > Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of > stpcpy > > * src/tac.c (copy_to_temp): Use stpcpy rather than sprintf. > Move some declarations "down" to point of initialization. > --- > src/tac.c | 17 +++++++---------- > 1 files changed, 7 insertions(+), 10 deletions(-) > > diff --git a/src/tac.c b/src/tac.c > index 65ac6a6..c572862 100644 > --- a/src/tac.c > +++ b/src/tac.c > @@ -426,20 +426,17 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) > { > static char *template = NULL; > static char const *tempdir; > - char *tempfile; > - FILE *tmp; > - int fd; > > if (template == NULL) > { > - char const * const Template = "%s/tacXXXXXX"; > + char const * const Template = "tacXXXXXX"; > tempdir = getenv ("TMPDIR"); > if (tempdir == NULL) > tempdir = DEFAULT_TMPDIR; > > - /* Subtract 2 for `%s' and add 1 for the trailing NUL byte. */ > - template = xmalloc (strlen (tempdir) + strlen (Template) - 2 + 1); > - sprintf (template, Template, tempdir); > + /* Add 1 for the slash and one for the trailing NUL byte. */ > + template = xmalloc (strlen (tempdir) + strlen (Template) + 1 + 1); > + stpcpy (stpcpy (stpcpy (template, tempdir), "/"), Template); > } > > /* FIXME: there's a small window between a successful mkstemp call Rather than hard-coding the "/", computing lengths, and using separate xmalloc and 3 stpcpy calls, I prefer to use file_name_concat: >From 54eab68b8c50f93484194090cdc1f079af5533b6 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Sun, 16 Oct 2011 10:35:56 +0200 Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of filenamecat * src/tac.c: Include filenamecat.h. (copy_to_temp): Use filenamecat rather than xmalloc and sprintf. Move some declarations "down" to point of initialization. --- src/tac.c | 16 ++++++---------- 1 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/tac.c b/src/tac.c index 65ac6a6..95a5df7 100644 --- a/src/tac.c +++ b/src/tac.c @@ -49,6 +49,7 @@ tac -r -s '.\| #include "safe-read.h" #include "stdlib--.h" #include "xfreopen.h" +#include "filenamecat.h" /* The official name of this program (e.g., no `g' prefix). */ #define PROGRAM_NAME "tac" @@ -426,20 +427,15 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) { static char *template = NULL; static char const *tempdir; - char *tempfile; - FILE *tmp; - int fd; if (template == NULL) { - char const * const Template = "%s/tacXXXXXX"; + char const * const Template = "tacXXXXXX"; tempdir = getenv ("TMPDIR"); if (tempdir == NULL) tempdir = DEFAULT_TMPDIR; - /* Subtract 2 for `%s' and add 1 for the trailing NUL byte. */ - template = xmalloc (strlen (tempdir) + strlen (Template) - 2 + 1); - sprintf (template, Template, tempdir); + template = file_name_concat (tempdir, Template, NULL); } /* FIXME: there's a small window between a successful mkstemp call @@ -451,8 +447,8 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) FIXME: clean up upon fatal signal. Don't block them, in case $TMPFILE is a remote file system. */ - tempfile = template; - fd = mkstemp (template); + char *tempfile = template; + int fd = mkstemp (template); if (fd < 0) { error (0, errno, _("cannot create temporary file in %s"), @@ -460,7 +456,7 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) return false; } - tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); + FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); if (! tmp) { error (0, errno, _("cannot open %s for writing"), quote (tempfile)); -- 1.7.7 From debbugs-submit-bounces@debbugs.gnu.org Mon Oct 17 11:56:01 2011 Received: (at 9762) by debbugs.gnu.org; 17 Oct 2011 15:56:01 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFpXl-0003c2-0E for submit@debbugs.gnu.org; Mon, 17 Oct 2011 11:56:01 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFpXj-0003bv-3f for 9762@debbugs.gnu.org; Mon, 17 Oct 2011 11:56:00 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 4C1AE6004E; Mon, 17 Oct 2011 17:55:06 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <87fwirejej.fsf@rho.meyering.net> (Jim Meyering's message of "Mon, 17 Oct 2011 17:18:28 +0200") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> Date: Mon, 17 Oct 2011 17:55:06 +0200 Message-ID: <87aa8zehph.fsf@rho.meyering.net> Lines: 41 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Jim Meyering wrote: ... >> maint: tac: remove sole use of sprintf in favor of stpcpy > ... > > I realized that this part of the first patch wasn't quite right... > >> Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of >> stpcpy ... >> /* FIXME: there's a small window between a successful mkstemp call > > Rather than hard-coding the "/", computing lengths, and using separate > xmalloc and 3 stpcpy calls, I prefer to use file_name_concat: > >>>From 54eab68b8c50f93484194090cdc1f079af5533b6 Mon Sep 17 00:00:00 2001 > From: Jim Meyering > Date: Sun, 16 Oct 2011 10:35:56 +0200 > Subject: [PATCH 1/3] maint: tac: remove sole use of sprintf in favor of > filenamecat > > * src/tac.c: Include filenamecat.h. > (copy_to_temp): Use filenamecat rather than xmalloc and sprintf. > Move some declarations "down" to point of initialization. > --- > src/tac.c | 16 ++++++---------- > 1 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/src/tac.c b/src/tac.c > index 65ac6a6..95a5df7 100644 > --- a/src/tac.c > +++ b/src/tac.c > @@ -49,6 +49,7 @@ tac -r -s '.\| > #include "safe-read.h" > #include "stdlib--.h" > #include "xfreopen.h" > +#include "filenamecat.h" And I nearly left it that way. Tweaked to insert the new include in alphabetical order like all of the others, and pushed. From debbugs-submit-bounces@debbugs.gnu.org Mon Oct 17 13:25:20 2011 Received: (at 9762) by debbugs.gnu.org; 17 Oct 2011 17:25:21 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFqwC-0007Bs-PH for submit@debbugs.gnu.org; Mon, 17 Oct 2011 13:25:20 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFqwA-0007Bg-Ce for 9762@debbugs.gnu.org; Mon, 17 Oct 2011 13:25:19 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id F0764A60001; Mon, 17 Oct 2011 10:24:19 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uyLB5quwvPhE; Mon, 17 Oct 2011 10:24:19 -0700 (PDT) Received: from [131.179.64.200] (Penguin.CS.UCLA.EDU [131.179.64.200]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 8DDC239E8008; Mon, 17 Oct 2011 10:24:19 -0700 (PDT) Message-ID: <4E9C64C3.3010104@cs.ucla.edu> Date: Mon, 17 Oct 2011 10:24:19 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110928 Fedora/3.1.15-1.fc14 Thunderbird/3.1.15 MIME-Version: 1.0 To: Jim Meyering Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> In-Reply-To: <87fwirejej.fsf@rho.meyering.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.1 (---) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.1 (---) On 10/17/11 08:18, Jim Meyering wrote: >> + char const * const Template = "tacXXXXXX"; That "const * const" prompted me to suggest a minor improvement. Since Template's address is never taken, better would be char const Template[] = "tacXXXXXX"; Hmm, or better yet, get rid of Template entirely: --- a/src/tac.c +++ b/src/tac.c @@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) if (template == NULL) { - char const * const Template = "tacXXXXXX"; tempdir = getenv ("TMPDIR"); if (tempdir == NULL) tempdir = DEFAULT_TMPDIR; - template = file_name_concat (tempdir, Template, NULL); + template = file_name_concat (tempdir, "tacXXXXXX", NULL); } /* FIXME: there's a small window between a successful mkstemp call From debbugs-submit-bounces@debbugs.gnu.org Mon Oct 17 13:32:12 2011 Received: (at 9762) by debbugs.gnu.org; 17 Oct 2011 17:32:12 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFr2p-0007MH-M6 for submit@debbugs.gnu.org; Mon, 17 Oct 2011 13:32:12 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFr2m-0007M8-WE for 9762@debbugs.gnu.org; Mon, 17 Oct 2011 13:32:10 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 3D78B6004E; Mon, 17 Oct 2011 19:31:15 +0200 (CEST) From: Jim Meyering To: Paul Eggert Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <4E9C64C3.3010104@cs.ucla.edu> (Paul Eggert's message of "Mon, 17 Oct 2011 10:24:19 -0700") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <4E9C64C3.3010104@cs.ucla.edu> Date: Mon, 17 Oct 2011 19:31:15 +0200 Message-ID: <874nz7ed98.fsf@rho.meyering.net> Lines: 30 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Paul Eggert wrote: > On 10/17/11 08:18, Jim Meyering wrote: >>> + char const * const Template = "tacXXXXXX"; > > That "const * const" prompted me to suggest a minor > improvement. Since Template's address is never taken, > better would be > > char const Template[] = "tacXXXXXX"; > > Hmm, or better yet, get rid of Template entirely: Cleaner and one line shorter. Thanks! > --- a/src/tac.c > +++ b/src/tac.c > @@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) > > if (template == NULL) > { > - char const * const Template = "tacXXXXXX"; > tempdir = getenv ("TMPDIR"); > if (tempdir == NULL) > tempdir = DEFAULT_TMPDIR; > > - template = file_name_concat (tempdir, Template, NULL); > + template = file_name_concat (tempdir, "tacXXXXXX", NULL); > } > > /* FIXME: there's a small window between a successful mkstemp call From debbugs-submit-bounces@debbugs.gnu.org Mon Oct 17 21:09:33 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 01:09:33 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFyBR-0002NV-1a for submit@debbugs.gnu.org; Mon, 17 Oct 2011 21:09:33 -0400 Received: from smtp-out.google.com ([216.239.44.51]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RFyBO-0002NG-RW for 9762@debbugs.gnu.org; Mon, 17 Oct 2011 21:09:31 -0400 Received: from wpaz5.hot.corp.google.com (wpaz5.hot.corp.google.com [172.24.198.69]) by smtp-out.google.com with ESMTP id p9I18Ura003639 for <9762@debbugs.gnu.org>; Mon, 17 Oct 2011 18:08:30 -0700 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=google.com; s=beta; t=1318900111; bh=m6NI4mLqbB8PIYmDvSfnMqYjbc8=; h=MIME-Version:In-Reply-To:References:Date:Message-ID:Subject:From: To:Cc:Content-Type; b=AvHtOWc7IKLGOz90gVW8BmPXwifxoqJ3p2VLzZKhsnTCUM5JkuSp0/lzj9QGDszCx 6ofgd4i+rk9wmmifcZPXw== DomainKey-Signature: a=rsa-sha1; s=beta; d=google.com; c=nofws; q=dns; h=dkim-signature:mime-version:in-reply-to:references:date: message-id:subject:from:to:cc:content-type:x-system-of-record; b=jNiDw8UXgpoWZq9C/uNO9cQN08FMFzHSTp4CSNvMcBIv54y5TcEPrhblaiyEluejl toAMoi09uKw+xgtvT5XrA== Received: from vws14 (vws14.prod.google.com [10.241.21.142]) by wpaz5.hot.corp.google.com with ESMTP id p9I17Km3011742 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <9762@debbugs.gnu.org>; Mon, 17 Oct 2011 18:08:30 -0700 Received: by vws14 with SMTP id 14so13640vws.7 for <9762@debbugs.gnu.org>; Mon, 17 Oct 2011 18:08:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=beta; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record; bh=8j4v+fe+aFHU71qbGRdAM9aRL3kNBZf77DhZeUBezO0=; b=VgZ8Qo038ki65oGogp1Jg3+1qyYQvRFVN01mAs2EzPECsjHm6tj1DXNxpqjNM1+5Es xY1IsCHLiIvX8aOiHwRQ== Received: by 10.229.64.154 with SMTP id e26mr37241qci.171.1318900109757; Mon, 17 Oct 2011 18:08:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.229.64.154 with SMTP id e26mr37231qci.171.1318900109530; Mon, 17 Oct 2011 18:08:29 -0700 (PDT) Received: by 10.229.169.212 with HTTP; Mon, 17 Oct 2011 18:08:29 -0700 (PDT) In-Reply-To: <87aa8zehph.fsf@rho.meyering.net> References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <87aa8zehph.fsf@rho.meyering.net> Date: Mon, 17 Oct 2011 18:08:29 -0700 Message-ID: Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() From: Ambrose Feinstein To: Jim Meyering Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Spam-Score: -6.3 (------) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -6.4 (------) Thanks for the quick fix! Aside from it requiring rearranging other code to not close the fd, was there a reason not to reuse a single temp file? From debbugs-submit-bounces@debbugs.gnu.org Tue Oct 18 01:56:12 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 05:56:12 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RG2eq-0001vm-IA for submit@debbugs.gnu.org; Tue, 18 Oct 2011 01:56:12 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RG2en-0001ve-Vz for 9762@debbugs.gnu.org; Tue, 18 Oct 2011 01:56:11 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id CFF43602C6; Tue, 18 Oct 2011 07:46:33 +0200 (CEST) From: Jim Meyering To: Paul Eggert Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <4E9C64C3.3010104@cs.ucla.edu> (Paul Eggert's message of "Mon, 17 Oct 2011 10:24:19 -0700") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <4E9C64C3.3010104@cs.ucla.edu> Date: Tue, 18 Oct 2011 07:46:33 +0200 Message-ID: <87sjmqc0na.fsf@rho.meyering.net> Lines: 73 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Paul Eggert wrote: > On 10/17/11 08:18, Jim Meyering wrote: >>> + char const * const Template = "tacXXXXXX"; > > That "const * const" prompted me to suggest a minor > improvement. Since Template's address is never taken, > better would be > > char const Template[] = "tacXXXXXX"; > > Hmm, or better yet, get rid of Template entirely: > > --- a/src/tac.c > +++ b/src/tac.c > @@ -430,12 +430,11 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) > > if (template == NULL) > { > - char const * const Template = "tacXXXXXX"; > tempdir = getenv ("TMPDIR"); > if (tempdir == NULL) > tempdir = DEFAULT_TMPDIR; > > - template = file_name_concat (tempdir, Template, NULL); > + template = file_name_concat (tempdir, "tacXXXXXX", NULL); > } > > /* FIXME: there's a small window between a successful mkstemp call I wrote the log for you, and while doing that realized that I found the following clearer still: (and shorter) char *t = getenv ("TMPDIR"); tempdir = t ? t : DEFAULT_TMPDIR; template = file_name_concat (tempdir, "tacXXXXXX", NULL); With your name on it, I'll wait for an ACK before pushing: >From 385634c8dd512fc4fae46310266247b8fcf2a85a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Tue, 18 Oct 2011 07:43:58 +0200 Subject: [PATCH] maint: make tac.c slightly cleaner * src/tac.c (copy_to_temp): Now that the template string tacXXXXXX is used in only one place, don't bother using a separate variable. Also, using three unconditional assignments seems slightly clearer. --- src/tac.c | 9 +++------ 1 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/tac.c b/src/tac.c index 97b19ae..7d99595 100644 --- a/src/tac.c +++ b/src/tac.c @@ -430,12 +430,9 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) if (template == NULL) { - char const * const Template = "tacXXXXXX"; - tempdir = getenv ("TMPDIR"); - if (tempdir == NULL) - tempdir = DEFAULT_TMPDIR; - - template = file_name_concat (tempdir, Template, NULL); + char *t = getenv ("TMPDIR"); + tempdir = t ? t : DEFAULT_TMPDIR; + template = file_name_concat (tempdir, "tacXXXXXX", NULL); } /* FIXME: there's a small window between a successful mkstemp call -- 1.7.6.4 From debbugs-submit-bounces@debbugs.gnu.org Tue Oct 18 02:23:27 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 06:23:27 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RG35B-0002Y6-K9 for submit@debbugs.gnu.org; Tue, 18 Oct 2011 02:23:26 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RG358-0002Xq-9m for 9762@debbugs.gnu.org; Tue, 18 Oct 2011 02:23:23 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id 2819E39E8007; Mon, 17 Oct 2011 23:22:21 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4ZuNX92vySuh; Mon, 17 Oct 2011 23:22:20 -0700 (PDT) Received: from [192.168.1.10] (pool-71-189-109-235.lsanca.fios.verizon.net [71.189.109.235]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id CE27839E8006; Mon, 17 Oct 2011 23:22:20 -0700 (PDT) Message-ID: <4E9D1B1C.4090201@cs.ucla.edu> Date: Mon, 17 Oct 2011 23:22:20 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.23) Gecko/20110922 Thunderbird/3.1.15 MIME-Version: 1.0 To: Jim Meyering Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <4E9C64C3.3010104@cs.ucla.edu> <87sjmqc0na.fsf@rho.meyering.net> In-Reply-To: <87sjmqc0na.fsf@rho.meyering.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Score: -2.9 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.9 (--) On 10/17/11 22:46, Jim Meyering wrote: > With your name on it, I'll wait for an ACK before pushing: Looks good. Your name should really be on it now.... Thanks. From debbugs-submit-bounces@debbugs.gnu.org Tue Oct 18 10:05:22 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 14:05:22 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGAID-0006NB-TK for submit@debbugs.gnu.org; Tue, 18 Oct 2011 10:05:22 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGAIA-0006N0-Uq for 9762@debbugs.gnu.org; Tue, 18 Oct 2011 10:05:20 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id 906BB6002D; Tue, 18 Oct 2011 14:38:21 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: (Ambrose Feinstein's message of "Mon, 17 Oct 2011 18:08:29 -0700") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <87aa8zehph.fsf@rho.meyering.net> Date: Tue, 18 Oct 2011 14:38:21 +0200 Message-ID: <87d3dubhky.fsf@rho.meyering.net> Lines: 287 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Ambrose Feinstein wrote: > Thanks for the quick fix! > > Aside from it requiring rearranging other code to not close the fd, > was there a reason not to reuse a single temp file? Actually that's what I started doing, but the change seemed like it'd end up being bigger than I wanted for a bug fix. In retrospect, now that I've done it and see the extent, it would have been ok. Especially once I considered another factor: we want to avoid use of functions like xmalloc and file_name_concat that call exit upon failure. See the log on the second patch below for why. >From fef2bc68e36c8891780311d8869db23753c093d0 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Tue, 18 Oct 2011 11:44:39 +0200 Subject: [PATCH 1/3] tac: use only one temporary file, with multiple nonseekable inputs * src/tac.c (temp_stream): New function, factored out of... (copy_to_temp): ...here. (tac_nonseekable): Don't free or fclose, now that we reuse the file. --- src/tac.c | 120 ++++++++++++++++++++++++++++++++++++------------------------- 1 files changed, 71 insertions(+), 49 deletions(-) diff --git a/src/tac.c b/src/tac.c index 7d99595..6a7e510 100644 --- a/src/tac.c +++ b/src/tac.c @@ -418,53 +418,78 @@ record_or_unlink_tempfile (char const *fn, FILE *fp ATTRIBUTE_UNUSED) #endif -/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to - a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream - and file name. Return true if successful. */ - +/* A wrapper around mkstemp that gives us both an open stream pointer, + FP, and the corresponding FILE_NAME. Always return the same FP/name + pair, rewinding/truncating it upon each reuse. */ static bool -copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) +temp_stream (FILE **fp, char **file_name) { - static char *template = NULL; - static char const *tempdir; - - if (template == NULL) + static char *tempfile = NULL; + static FILE *tmp_fp; + if (tempfile == NULL) { - char *t = getenv ("TMPDIR"); - tempdir = t ? t : DEFAULT_TMPDIR; - template = file_name_concat (tempdir, "tacXXXXXX", NULL); - } + char const *t = getenv ("TMPDIR"); + char const *tempdir = t ? t : DEFAULT_TMPDIR; + tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL); + + /* FIXME: there's a small window between a successful mkstemp call + and the unlink that's performed by record_or_unlink_tempfile. + If we're interrupted in that interval, this code fails to remove + the temporary file. On systems that define DONT_UNLINK_WHILE_OPEN, + the window is much larger -- it extends to the atexit-called + unlink_tempfile. + FIXME: clean up upon fatal signal. Don't block them, in case + $TMPFILE is a remote file system. */ + + int fd = mkstemp (tempfile); + if (fd < 0) + { + error (0, errno, _("cannot create temporary file in %s"), + quote (tempdir)); + goto Reset; + } - /* FIXME: there's a small window between a successful mkstemp call - and the unlink that's performed by record_or_unlink_tempfile. - If we're interrupted in that interval, this code fails to remove - the temporary file. On systems that define DONT_UNLINK_WHILE_OPEN, - the window is much larger -- it extends to the atexit-called - unlink_tempfile. - FIXME: clean up upon fatal signal. Don't block them, in case - $TMPFILE is a remote file system. */ - - char *tempfile = xstrdup (template); - int fd = mkstemp (tempfile); - if (fd < 0) - { - error (0, errno, _("cannot create temporary file in %s"), - quote (tempdir)); - free (tempfile); - return false; - } + tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); + if (! tmp_fp) + { + error (0, errno, _("cannot open %s for writing"), quote (tempfile)); + close (fd); + unlink (tempfile); + Reset: + free (tempfile); + tempfile = NULL; + return false; + } - FILE *tmp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); - if (! tmp) + record_or_unlink_tempfile (tempfile, tmp_fp); + } + else { - error (0, errno, _("cannot open %s for writing"), quote (tempfile)); - close (fd); - unlink (tempfile); - free (tempfile); - return false; + if (fseek (tmp_fp, 0, SEEK_SET) < 0 + || ftruncate (fileno (tmp_fp), 0) < 0) + { + error (0, errno, _("failed to rewind stream for %s"), + quote (tempfile)); + return false; + } } - record_or_unlink_tempfile (tempfile, tmp); + *fp = tmp_fp; + *file_name = tempfile; + return true; +} + +/* Copy from file descriptor INPUT_FD (corresponding to the named FILE) to + a temporary file, and set *G_TMP and *G_TEMPFILE to the resulting stream + and file name. Return true if successful. */ + +static bool +copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) +{ + FILE *fp; + char *file_name; + if (!temp_stream (&fp, &file_name)) + return false; while (1) { @@ -477,26 +502,25 @@ copy_to_temp (FILE **g_tmp, char **g_tempfile, int input_fd, char const *file) goto Fail; } - if (fwrite (G_buffer, 1, bytes_read, tmp) != bytes_read) + if (fwrite (G_buffer, 1, bytes_read, fp) != bytes_read) { - error (0, errno, _("%s: write error"), quotearg_colon (tempfile)); + error (0, errno, _("%s: write error"), quotearg_colon (file_name)); goto Fail; } } - if (fflush (tmp) != 0) + if (fflush (fp) != 0) { - error (0, errno, _("%s: write error"), quotearg_colon (tempfile)); + error (0, errno, _("%s: write error"), quotearg_colon (file_name)); goto Fail; } - *g_tmp = tmp; - *g_tempfile = tempfile; + *g_tmp = fp; + *g_tempfile = file_name; return true; Fail: - fclose (tmp); - free (tempfile); + fclose (fp); return false; } @@ -512,8 +536,6 @@ tac_nonseekable (int input_fd, const char *file) return false; bool ok = tac_seekable (fileno (tmp_stream), tmp_file); - fclose (tmp_stream); - free (tmp_file); return ok; } -- 1.7.6.4 >From 1b9bb7a2d29cdf3977bbe23deca3cebe6c03bfaf Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Tue, 18 Oct 2011 12:04:02 +0200 Subject: [PATCH 2/3] tac: do not let failed allocation cause immediate exit * src/tac.c (temp_stream): Don't exit immediately upon failed heap allocation, here. That would inhibit processing of any additional command-line arguments. --- src/tac.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/tac.c b/src/tac.c index 6a7e510..7cc562e 100644 --- a/src/tac.c +++ b/src/tac.c @@ -430,7 +430,12 @@ temp_stream (FILE **fp, char **file_name) { char const *t = getenv ("TMPDIR"); char const *tempdir = t ? t : DEFAULT_TMPDIR; - tempfile = file_name_concat (tempdir, "tacXXXXXX", NULL); + tempfile = mfile_name_concat (tempdir, "tacXXXXXX", NULL); + if (tempdir == NULL) + { + error (0, 0, _("memory exhausted")); + return false; + } /* FIXME: there's a small window between a successful mkstemp call and the unlink that's performed by record_or_unlink_tempfile. -- 1.7.6.4 >From 424ed2cd9173ef6191bf5cf38fabc1ed7903c6ef Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Tue, 18 Oct 2011 14:20:36 +0200 Subject: [PATCH 3/3] maint: tac: prefer "failed to" diagnostic over "cannot" --- src/tac.c | 6 +++--- tests/misc/tac | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/tac.c b/src/tac.c index 7cc562e..36122b0 100644 --- a/src/tac.c +++ b/src/tac.c @@ -449,7 +449,7 @@ temp_stream (FILE **fp, char **file_name) int fd = mkstemp (tempfile); if (fd < 0) { - error (0, errno, _("cannot create temporary file in %s"), + error (0, errno, _("failed to create temporary file in %s"), quote (tempdir)); goto Reset; } @@ -457,7 +457,7 @@ temp_stream (FILE **fp, char **file_name) tmp_fp = fdopen (fd, (O_BINARY ? "w+b" : "w+")); if (! tmp_fp) { - error (0, errno, _("cannot open %s for writing"), quote (tempfile)); + error (0, errno, _("failed to open %s for writing"), quote (tempfile)); close (fd); unlink (tempfile); Reset: @@ -569,7 +569,7 @@ tac_file (const char *filename) fd = open (filename, O_RDONLY | O_BINARY); if (fd < 0) { - error (0, errno, _("cannot open %s for reading"), quote (filename)); + error (0, errno, _("failed to open %s for reading"), quote (filename)); return false; } } diff --git a/tests/misc/tac b/tests/misc/tac index 7bd07bd..5602128 100755 --- a/tests/misc/tac +++ b/tests/misc/tac @@ -68,7 +68,7 @@ my @Tests = {ENV => "TMPDIR=$bad_dir"}, {IN_PIPE => "a\n"}, {ERR_SUBST => "s,`$bad_dir': .*,...,"}, - {ERR => "$prog: cannot create temporary file in ...\n"}, + {ERR => "$prog: failed to create temporary file in ...\n"}, {EXIT => 1}], # coreutils-8.5's tac would double-free its primary buffer. -- 1.7.6.4 From debbugs-submit-bounces@debbugs.gnu.org Tue Oct 18 12:10:04 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 16:10:04 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGCEt-0000oj-Mc for submit@debbugs.gnu.org; Tue, 18 Oct 2011 12:10:04 -0400 Received: from smtp.cs.ucla.edu ([131.179.128.62]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGCEq-0000oC-K6 for 9762@debbugs.gnu.org; Tue, 18 Oct 2011 12:10:02 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by smtp.cs.ucla.edu (Postfix) with ESMTP id CB150A60001; Tue, 18 Oct 2011 09:08:56 -0700 (PDT) X-Virus-Scanned: amavisd-new at smtp.cs.ucla.edu Received: from smtp.cs.ucla.edu ([127.0.0.1]) by localhost (smtp.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qvJxLJyakp8L; Tue, 18 Oct 2011 09:08:56 -0700 (PDT) Received: from [131.179.64.200] (Penguin.CS.UCLA.EDU [131.179.64.200]) by smtp.cs.ucla.edu (Postfix) with ESMTPSA id 58CBB39E8006; Tue, 18 Oct 2011 09:08:56 -0700 (PDT) Message-ID: <4E9DA498.30705@cs.ucla.edu> Date: Tue, 18 Oct 2011 09:08:56 -0700 From: Paul Eggert Organization: UCLA Computer Science Department User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110928 Fedora/3.1.15-1.fc14 Thunderbird/3.1.15 MIME-Version: 1.0 To: Jim Meyering Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <87aa8zehph.fsf@rho.meyering.net> <87d3dubhky.fsf@rho.meyering.net> In-Reply-To: <87d3dubhky.fsf@rho.meyering.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Score: -3.1 (---) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org, Ambrose Feinstein X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -3.1 (---) On 10/18/11 05:38, Jim Meyering wrote: > * src/tac.c (temp_stream): Don't exit immediately upon failed heap > allocation, here. That would inhibit processing of any additional > command-line arguments. It doesn't matter that much either way here, but in general I'm leery about any attempt to do useful work after a memory allocation failure in C. The output of 'tac' will be wrong anyway, and many users would prefer 'tac' to simply exit, than to go on and generate a much larger (but still wrong) output, consuming needless resources. Also (as my recent experiences with Emacs have shown), once malloc has failed, other code can start to go wrong, including library code that we have no control over. Sometimes the failures are subtle, and sometimes they're spectacular; either way it's typically better to steer clear of that particular chasm. After malloc fails, Emacs typically enters a "restricted" mode, where it first releases a memory reserve, and then continually asks you to save your work and exit as soon as possible. This is good advice, and even then one is not entirely sure of exiting safely. From debbugs-submit-bounces@debbugs.gnu.org Tue Oct 18 12:28:48 2011 Received: (at 9762) by debbugs.gnu.org; 18 Oct 2011 16:28:48 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGCX2-0001Eo-I2 for submit@debbugs.gnu.org; Tue, 18 Oct 2011 12:28:48 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGCWz-0001Eg-LL for 9762@debbugs.gnu.org; Tue, 18 Oct 2011 12:28:46 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id DEFBB6007E; Tue, 18 Oct 2011 18:27:46 +0200 (CEST) From: Jim Meyering To: Paul Eggert Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <4E9DA498.30705@cs.ucla.edu> (Paul Eggert's message of "Tue, 18 Oct 2011 09:08:56 -0700") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <87aa8zehph.fsf@rho.meyering.net> <87d3dubhky.fsf@rho.meyering.net> <4E9DA498.30705@cs.ucla.edu> Date: Tue, 18 Oct 2011 18:27:46 +0200 Message-ID: <8739eqfenx.fsf@rho.meyering.net> Lines: 52 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762 Cc: 9762@debbugs.gnu.org, Ambrose Feinstein X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Paul Eggert wrote: > On 10/18/11 05:38, Jim Meyering wrote: >> * src/tac.c (temp_stream): Don't exit immediately upon failed heap >> allocation, here. That would inhibit processing of any additional >> command-line arguments. Hi Paul, Perhaps I should add this to the log: ...which may require no additional memory allocation. > It doesn't matter that much either way here, but in general I'm > leery about any attempt to do useful work after a memory allocation > failure in C. The output of 'tac' will be wrong anyway, and many > users would prefer 'tac' to simply exit, than to go on and generate > a much larger (but still wrong) output, consuming needless resources. > > Also (as my recent experiences with Emacs have shown), > once malloc has failed, other code can start to go wrong, > including library code that we have no control over. Sometimes > the failures are subtle, and sometimes they're spectacular; either > way it's typically better to steer clear of that particular chasm. > > After malloc fails, Emacs typically enters a "restricted" mode, > where it first releases a memory reserve, and then continually > asks you to save your work and exit as soon as possible. This > is good advice, and even then one is not entirely sure of exiting > safely. That makes sense for a monolithic, long-running program like emacs, but command-line tools like tac... There is little risk. tac will still exit nonzero, and that should be sufficient to tell any caller that there's been a failure. What is to be gained? Diagnostics about other file arguments and a greater percentage of output. In this case continuing to process arguments after such a failure is easy and imho, desirable, since a non-seekable input is unusual and likely to be followed by a seekable one (if by anything). i.e., if this command fails while processing arg2, tac foo <(some command) bar unreadable it should be able to process "bar" and then diagnose that the final argument is an unreadable file. The principle is the same as the one that prompts POSIX to require (at least from what I recall) that all arguments be processed, whenever possible. From debbugs-submit-bounces@debbugs.gnu.org Wed Oct 19 03:35:46 2011 Received: (at 9762-done) by debbugs.gnu.org; 19 Oct 2011 07:35:46 +0000 Received: from localhost ([127.0.0.1] helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGQgk-0007o6-3Y for submit@debbugs.gnu.org; Wed, 19 Oct 2011 03:35:46 -0400 Received: from mx.meyering.net ([88.168.87.75]) by debbugs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1RGQgg-0007nx-3I for 9762-done@debbugs.gnu.org; Wed, 19 Oct 2011 03:35:44 -0400 Received: from rho.meyering.net (localhost.localdomain [127.0.0.1]) by rho.meyering.net (Acme Bit-Twister) with ESMTP id C723960150; Wed, 19 Oct 2011 09:34:39 +0200 (CEST) From: Jim Meyering To: Ambrose Feinstein Subject: Re: bug#9762: tac fails when given multiple non-seekable inputs due to misuse of mkstemp() In-Reply-To: <87d3dubhky.fsf@rho.meyering.net> (Jim Meyering's message of "Tue, 18 Oct 2011 14:38:21 +0200") References: <87sjmtgrrc.fsf@rho.meyering.net> <87fwirejej.fsf@rho.meyering.net> <87aa8zehph.fsf@rho.meyering.net> <87d3dubhky.fsf@rho.meyering.net> Date: Wed, 19 Oct 2011 09:34:39 +0200 Message-ID: <87ty75cu40.fsf@rho.meyering.net> Lines: 45 MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.8 (--) X-Debbugs-Envelope-To: 9762-done Cc: 9762-done@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: debbugs-submit-bounces@debbugs.gnu.org Errors-To: debbugs-submit-bounces@debbugs.gnu.org X-Spam-Score: -2.8 (--) Jim Meyering wrote: > Ambrose Feinstein wrote: >> Thanks for the quick fix! >> >> Aside from it requiring rearranging other code to not close the fd, >> was there a reason not to reuse a single temp file? > > Actually that's what I started doing, but the change seemed like it'd > end up being bigger than I wanted for a bug fix. In retrospect, now > that I've done it and see the extent, it would have been ok. > > Especially once I considered another factor: we want to avoid use of > functions like xmalloc and file_name_concat that call exit upon failure. > See the log on the second patch below for why. > > >>>From fef2bc68e36c8891780311d8869db23753c093d0 Mon Sep 17 00:00:00 2001 > From: Jim Meyering > Date: Tue, 18 Oct 2011 11:44:39 +0200 > Subject: [PATCH 1/3] tac: use only one temporary file, with multiple > nonseekable inputs > > * src/tac.c (temp_stream): New function, factored out of... > (copy_to_temp): ...here. > (tac_nonseekable): Don't free or fclose, now that we reuse the file. Added this, and pushed: Suggested by Ambrose Feinstein. * THANKS.in: Update. diff --git a/THANKS.in b/THANKS.in index 23ac679..83a7864 100644 --- a/THANKS.in +++ b/THANKS.in @@ -37,6 +37,7 @@ Alexandre Duret-Lutz duret_g@epita.fr Alexey Solovyov alekso@math.uu.se Alexey Vyskubov alexey@pippuri.mawhrin.net Alfred M. Szmidt ams@kemisten.nu +Ambrose Feinstein ambrose@google.com Andi Kleen freitag@alancoxonachip.com Andre Novaes Cunha Andre.Cunha@br.global-one.net Andreas Frische andreasfrische@gmail.com From unknown Mon Jun 23 09:35:37 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Wed, 16 Nov 2011 12:24:03 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator