[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH] Fix Savannah bug #24873, Duplicate fprint option corrupts output
From: |
James Youngman |
Subject: |
[PATCH] Fix Savannah bug #24873, Duplicate fprint option corrupts output by opening each output file only once. |
Date: |
Sun, 30 Nov 2008 14:53:16 +0000 |
Fix Savannah bug #24873, Duplicate fprint option corrupts output
by opening each output file only once.
* find/sharefile.c: New file; keeps a mapping from dev/inode to
FILE*, allowing us to determine when the file we just opened is
the same as something else we already opened.
* import-gnulib.config (modules): Import the hash module, used by
sharefile.c.
* find/sharefile.h: Function declarations for same.
* find/find.c (main): Call sharefile_init().
* find/ftsfind.c (main): Likewise.
* find/parser.c (open_output_file): Call sharfile_fopen to open an
output file, instead of fopen_safer.
* find/util.c (cleanup): Close any shared output files that are
open. Also fflush stdout.
(undangle_file_pointers): Set the relevant FILE* pointers to
NULL.
(flush_and_close_output_files): Remove (since sharefile_destroy
has the desired effect).
* find/Makefile.am (libfindtools_a_SOURCES): Add sharefile.c.
(EXTRA_DIST): Add sharefile.h.
* find/defs.h: #include sharefile.h.
(struct state): Add member shared_files, holding a handle to the
shared-file hash table implemented in sharefile.[ch].
* find/testsuite/find.gnu/fprintf-samefile.exp: New test,
exercising -fprintf (though it is not able to detect bug #24873).
* find/testsuite/Makefile.am (EXTRA_DIST_EXP): Add
find.gnu/fprintf-samefile.exp.
Signed-off-by: James Youngman <address@hidden>
---
find/Makefile.am | 4 +-
find/defs.h | 4 +
find/find.c | 6 +
find/ftsfind.c | 6 +
find/parser.c | 6 +-
find/sharefile.c | 197 ++++++++++++++++++++++++++
find/sharefile.h | 31 ++++
find/testsuite/Makefile.am | 1 +
find/testsuite/find.gnu/fprintf-samefile.exp | 26 ++++
find/util.c | 56 +++-----
import-gnulib.config | 1 +
11 files changed, 300 insertions(+), 38 deletions(-)
create mode 100644 find/sharefile.c
create mode 100644 find/sharefile.h
create mode 100644 find/testsuite/find.gnu/fprintf-samefile.exp
diff --git a/find/Makefile.am b/find/Makefile.am
index b001509..e603fed 100644
--- a/find/Makefile.am
+++ b/find/Makefile.am
@@ -4,7 +4,7 @@ localedir = $(datadir)/locale
# regexprops_SOURCES = regexprops.c
noinst_LIBRARIES = libfindtools.a
-libfindtools_a_SOURCES = finddata.c fstype.c parser.c pred.c tree.c util.c
+libfindtools_a_SOURCES = finddata.c fstype.c parser.c pred.c tree.c util.c
sharefile.c
# We always build two versions of find, one with fts, one without.
@@ -24,7 +24,7 @@ find_SOURCES = find.c
ftsfind_SOURCES = ftsfind.c
endif
-EXTRA_DIST = defs.h $(man_MANS)
+EXTRA_DIST = defs.h sharefile.h $(man_MANS)
INCLUDES = -I../gnulib/lib -I$(top_srcdir)/lib -I$(top_srcdir)/gnulib/lib
-I../intl -DLOCALEDIR=\"$(localedir)\"
LDADD = ./libfindtools.a ../lib/libfind.a ../gnulib/lib/libgnulib.a @INTLLIBS@
@LIB_CLOCK_GETTIME@ @FINDLIBS@
man_MANS = find.1
diff --git a/find/defs.h b/find/defs.h
index 1708d83..44dae72 100644
--- a/find/defs.h
+++ b/find/defs.h
@@ -64,6 +64,7 @@ typedef bool boolean;
#include "timespec.h"
#include "buildcmd.h"
#include "quotearg.h"
+#include "sharefile.h"
/* These days we will assume ANSI/ISO C protootypes work on our compiler. */
#define PARAMS(Args) Args
@@ -648,6 +649,9 @@ struct state
* an optimisation. Set to true if you want to be conservative.
*/
boolean execdirs_outstanding;
+
+ /* Shared files, opened via the interface in sharefile.h. */
+ sharefile_handle shared_files;
};
/* finddata.c */
diff --git a/find/find.c b/find/find.c
index 171988f..0571e13 100644
--- a/find/find.c
+++ b/find/find.c
@@ -131,6 +131,12 @@ main (int argc, char **argv)
program_name = argv[0];
state.exit_status = 0;
+ state.shared_files = sharefile_init("w");
+ if (NULL == state.shared_files)
+ {
+ error (1, errno, _("Failed initialise shared-file hash table"));
+ }
+
/* Set the option defaults before we do the locale
* initialisation as check_nofollow() needs to be executed in the
* POSIX locale.
diff --git a/find/ftsfind.c b/find/ftsfind.c
index 543b80f..30845eb 100644
--- a/find/ftsfind.c
+++ b/find/ftsfind.c
@@ -664,6 +664,12 @@ main (int argc, char **argv)
state.execdirs_outstanding = false;
state.cwd_dir_fd = AT_FDCWD;
+ state.shared_files = sharefile_init("w");
+ if (NULL == state.shared_files)
+ {
+ error (1, errno, _("Failed initialise shared-file hash table"));
+ }
+
/* Set the option defaults before we do the locale initialisation as
* check_nofollow() needs to be executed in the POSIX locale.
*/
diff --git a/find/parser.c b/find/parser.c
index 102eb7d..378421c 100644
--- a/find/parser.c
+++ b/find/parser.c
@@ -3506,16 +3506,16 @@ open_output_file (const char *path, struct format_val
*p)
}
else
{
- p->stream = fopen_safer (path, "w");
+ p->stream = sharefile_fopen (state.shared_files, path);
p->filename = path;
if (p->stream == NULL)
{
- fatal_file_error(path);
+ fatal_file_error (path);
}
}
- p->dest_is_tty = stream_is_tty(p->stream);
+ p->dest_is_tty = stream_is_tty (p->stream);
}
static void
diff --git a/find/sharefile.c b/find/sharefile.c
new file mode 100644
index 0000000..a5cc90f
--- /dev/null
+++ b/find/sharefile.c
@@ -0,0 +1,197 @@
+/* sharefile.c -- open files just once.
+ Copyright (C) 2008 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 <http://www.gnu.org/licenses/>.
+*/
+
+
+#include <config.h>
+
+#include <errno.h>
+#include <string.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include "stdio-safer.h"
+#include "hash.h"
+#include "sharefile.h"
+#include "defs.h"
+
+
+enum
+ {
+ DefaultHashTableSize = 11
+ };
+
+struct sharefile
+{
+ char *mode;
+ Hash_table *table;
+};
+
+
+/*
+ * We cannot use the name to determine that two strings represent the
+ * same file, since that test would be fooled by symbolic links.
+ * Instead we use the device and inode number.
+ *
+ * However, we remember the name of each file that we opened. This
+ * allows us to issue a fatal error message when (flushing and)
+ * closing a file fails.
+ */
+struct SharefileEntry
+{
+ dev_t device;
+ ino_t inode;
+ char *name; /* not the only name for this file; error messages only */
+ FILE *fp;
+};
+
+
+static bool
+entry_comparator (const void *av, const void *bv)
+{
+ const struct SharefileEntry *a=av, *b=bv;
+ return (a->inode == b->inode) && (a->device == b->device);
+}
+
+static void
+entry_free (void *pv)
+{
+ struct SharefileEntry *p = pv;
+ if (p->fp)
+ {
+ if (0 != fclose (p->fp))
+ fatal_file_error (p->name);
+ }
+ free (p->name);
+ free (p);
+}
+
+static size_t
+entry_hashfunc (const void *pv, size_t buckets)
+{
+ const struct SharefileEntry *p = pv;
+ return (p->device ^ p->inode) % buckets;
+}
+
+
+
+sharefile_handle
+sharefile_init (const char *mode)
+{
+ struct Hash_tuning;
+
+ struct sharefile *p = malloc (sizeof(struct sharefile));
+ if (p)
+ {
+ p->mode = strdup (mode);
+ if (p->mode)
+ {
+ p->table = hash_initialize (DefaultHashTableSize, NULL,
+ entry_hashfunc,
+ entry_comparator,
+ entry_free);
+ if (p->table)
+ {
+ return p;
+ }
+ else
+ {
+ free (p->mode);
+ free (p);
+ }
+ }
+ else
+ {
+ free (p);
+ }
+ }
+ return NULL;
+}
+
+void
+sharefile_destroy (sharefile_handle pv)
+{
+ struct sharefile *p = pv;
+ free (p->mode);
+ hash_free (p->table);
+}
+
+
+FILE *
+sharefile_fopen (sharefile_handle h, const char *filename)
+{
+ struct sharefile *p = h;
+ struct SharefileEntry *new_entry;
+
+ new_entry = malloc (sizeof (struct SharefileEntry));
+ if (!new_entry)
+ return NULL;
+
+ new_entry->name = strdup (filename);
+ if (NULL == new_entry->name)
+ {
+ free (new_entry);
+ return NULL;
+ }
+
+ if (NULL == (new_entry->fp = fopen_safer (filename, p->mode)))
+ {
+ free (new_entry);
+ return NULL;
+ }
+ else
+ {
+ struct stat st;
+ const int fd = fileno (new_entry->fp);
+ assert (fd >= 0);
+
+ if (fstat (fd, &st) < 0)
+ {
+ entry_free (new_entry);
+ return NULL;
+ }
+ else
+ {
+ void *existing;
+
+ new_entry->device = st.st_dev;
+ new_entry->inode = st.st_ino;
+
+ existing = hash_lookup (p->table, new_entry);
+ if (existing) /* We have previously opened that file. */
+ {
+ entry_free (new_entry); /* don't need new_entry. */
+ return ((const struct SharefileEntry*)existing)->fp;
+ }
+ else /* We didn't open it already */
+ {
+ if (hash_insert (p->table, new_entry))
+ {
+ return new_entry->fp;
+ }
+ else /* failed to insert in hashtable. */
+ {
+ const int save_errno = errno;
+ entry_free (new_entry);
+ errno = save_errno;
+ return NULL;
+ }
+ }
+ }
+ }
+}
diff --git a/find/sharefile.h b/find/sharefile.h
new file mode 100644
index 0000000..7d196d6
--- /dev/null
+++ b/find/sharefile.h
@@ -0,0 +1,31 @@
+/* sharefile.h -- open files just once.
+ Copyright (C) 2008 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 <http://www.gnu.org/licenses/>.
+*/
+
+
+#ifndef INC_SHAREFILE_H
+#define INC_SHAREFILE_H 1
+
+#include <stdlib.h>
+#include <stdio.h>
+
+typedef void * sharefile_handle;
+
+sharefile_handle sharefile_init(const char *mode);
+FILE *sharefile_fopen(sharefile_handle, const char *filename);
+void sharefile_destroy(sharefile_handle);
+
+#endif
diff --git a/find/testsuite/Makefile.am b/find/testsuite/Makefile.am
index 1b68b0d..efe7b06 100644
--- a/find/testsuite/Makefile.am
+++ b/find/testsuite/Makefile.am
@@ -124,6 +124,7 @@ find.gnu/false.exp \
find.gnu/follow-arg-parent-symlink.exp \
find.gnu/follow-basic.exp \
find.gnu/fprint0_stdout.exp \
+find.gnu/fprintf-samefile.exp \
find.gnu/fprint-unwritable.exp \
find.gnu/gnuand.exp \
find.gnu/gnunot.exp \
diff --git a/find/testsuite/find.gnu/fprintf-samefile.exp
b/find/testsuite/find.gnu/fprintf-samefile.exp
new file mode 100644
index 0000000..708c3a6
--- /dev/null
+++ b/find/testsuite/find.gnu/fprintf-samefile.exp
@@ -0,0 +1,26 @@
+# This test was added as part of the fix for Savannah bug #24873, but it
+# does not exercise the relevant condition (which is a race). While making
+# the fix I found that there were no tests for -fprintf at all, so I added
+# one.
+exec rm -rf tmp
+exec mkdir tmp
+exec touch tmp/file tmp/top
+exec ln -s file tmp/same
+# This command line should exercise the case where sharefile_fopen
+# Detects that two destination files are actually the same.
+find_start p {tmp/top -fprintf tmp/file "1: %p\n" -fprintf tmp/same "2: %p\n" }
+
+# We get here after the final iteration through the various
+# find binaries and -O option. However -fprintf truncates the
+# output file, so there should be just one set of output in there
+# from
+
+# Check that we got the right output in tmp/file.
+set f [open "tmp/file" "r"]
+set data [read $f]
+close $f
+set expected "1: tmp/top\n2: tmp/top\n"
+if { [string compare $data $expected] } {
+ fail "fprintf-samefile: expected output:\n$expected\nbut got:\n$data"
+}
+exec rm -rf tmp
diff --git a/find/util.c b/find/util.c
index 27db863..ffada2b 100644
--- a/find/util.c
+++ b/find/util.c
@@ -378,55 +378,45 @@ traverse_tree(struct predicate *tree,
if (tree->pred_right)
traverse_tree(tree->pred_right, callback);
}
-
+
+/* After sharefile_destroy is called, our output file
+ * pointers will be dangling (fclose will already have
+ * been called on them). NULL these out.
+ */
static void
-flush_and_close_output_files(struct predicate *p)
+undangle_file_pointers (struct predicate *p)
{
- if (pred_is(p, pred_fprint)
- || pred_is(p, pred_fprintf)
- || pred_is(p, pred_fls)
- || pred_is(p, pred_fprint0))
+ if (pred_is (p, pred_fprint)
+ || pred_is (p, pred_fprintf)
+ || pred_is (p, pred_fls)
+ || pred_is (p, pred_fprint0))
{
- FILE *f = p->args.printf_vec.stream;
- bool failed;
-
- if (f == stdout || f == stderr)
- failed = fflush(p->args.printf_vec.stream) == EOF;
- else
- failed = fclose(p->args.printf_vec.stream) == EOF;
-
- if (failed)
- nonfatal_file_error(p->args.printf_vec.filename);
- }
- else if (pred_is(p, pred_print))
- {
- if (fflush(p->args.printf_vec.stream) == EOF)
- {
- nonfatal_file_error(p->args.printf_vec.filename);
- }
- }
- else if (pred_is(p, pred_ls) || pred_is(p, pred_print0))
- {
- if (fflush(stdout) == EOF)
- {
- /* XXX: migrate to printf_vec. */
- nonfatal_file_error("standard output");
- }
+ /* The file was already fclose()d by sharefile_destroy. */
+ p->args.printf_vec.stream = NULL;
}
}
+
/* Complete any outstanding commands.
+ * Flush and close any open files.
*/
void
-cleanup(void)
+cleanup (void)
{
struct predicate *eval_tree = get_eval_tree();
if (eval_tree)
{
traverse_tree(eval_tree, complete_pending_execs);
complete_pending_execdirs(get_current_dirfd());
- traverse_tree(eval_tree, flush_and_close_output_files);
}
+
+ /* Close ouptut files and NULL out references to them. */
+ sharefile_destroy (state.shared_files);
+ if (eval_tree)
+ traverse_tree(eval_tree, undangle_file_pointers);
+
+ if (fflush (stdout) == EOF)
+ nonfatal_file_error ("standard output");
}
/* Savannah bug #16378 manifests as an assertion failure in pred_type()
diff --git a/import-gnulib.config b/import-gnulib.config
index 31efdfd..74a7ca3 100644
--- a/import-gnulib.config
+++ b/import-gnulib.config
@@ -49,6 +49,7 @@ getline
getopt
gettext
gpl-3.0
+hash
human
idcache
inline
--
1.5.6.5