bug-findutils
[Top][All Lists]
Advanced

[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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]