Fix bug #21280. Thanks to John Darrington for review.
authorBen Pfaff <blp@gnu.org>
Tue, 9 Oct 2007 03:50:20 +0000 (03:50 +0000)
committerBen Pfaff <blp@gnu.org>
Tue, 9 Oct 2007 03:50:20 +0000 (03:50 +0000)
* automake.mk: Add new file.

* bugs/overwrite-input-file.sh: New test.

* file-name.c (create_stream): New function.

* por-file-writer.c (pfm_open_writer): Use fh_open to open the
file handle before creating the file, to ensure that we don't
truncate a file that we're reading.  Make code easier to read by
using create_stream.

src/data/ChangeLog
src/data/file-name.c
src/data/file-name.h
src/data/por-file-writer.c
src/data/sys-file-writer.c
tests/ChangeLog
tests/automake.mk
tests/bugs/overwrite-input-file.sh [new file with mode: 0755]

index 819cb1279822a9ac7d55cbef0004a599660e1c4f..a8f897ef18a38d067474248e66a3d2395ab27f79 100644 (file)
@@ -1,3 +1,16 @@
+2007-10-08  Ben Pfaff  <blp@gnu.org>
+
+       Fix bug #21280.  Thanks to John Darrington for review.
+
+       * file-name.c (create_stream): New function.
+
+       * por-file-writer.c (pfm_open_writer): Use fh_open to open the
+       file handle before creating the file, to ensure that we don't
+       truncate a file that we're reading.  Make code easier to read by
+       using create_stream.
+
+       * sys-file-write.c (sfm_open_writer): Ditto.
+
 2007-10-01  Ben Pfaff  <blp@gnu.org>
 
        Fix bug #21192.  Thanks to John Darrington for review.
index f3b168176c0c63918d741ac9637fb9e81e8f6657..f8ad11d07a342f22fd5bf4cfa026d9a93a412ce0 100644 (file)
 
 #include <ctype.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
 
 #include "intprops.h"
 #include "minmax.h"
@@ -320,6 +322,30 @@ fn_close (const char *fn, FILE *f)
     return fclose (f);
 }
 
+/* Creates a new file named FN with the given PERMISSIONS bits,
+   and returns a stream for it or a null pointer on failure.
+   MODE should be "w" or "wb". */
+FILE *
+create_stream (const char *fn, const char *mode, mode_t permissions)
+{
+  int fd;
+  FILE *stream;
+
+  fd = open (fn, O_WRONLY | O_CREAT | O_TRUNC, permissions);
+  if (fd < 0)
+    return NULL;
+
+  stream = fdopen (fd, mode);
+  if (stream == NULL)
+    {
+      int save_errno = errno;
+      close (fd);
+      errno = save_errno;
+    }
+
+  return stream;
+}
+
 #if !(defined _WIN32 || defined __WIN32__)
 /* A file's identity. */
 struct file_identity
index d18ce22a5f51d418ea16c6d838e14ccca1412cc9..5564280bf45a3b4b0404fd2a58692a8c3915fbf0 100644 (file)
@@ -20,6 +20,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <libpspp/str.h>
+#include <sys/types.h>
 
 /* Search path for configuration files. */
 extern const char *config_path;
@@ -43,6 +44,8 @@ const char *fn_getenv_default (const char *variable, const char *def);
 FILE *fn_open (const char *fn, const char *mode);
 int fn_close (const char *fn, FILE *file);
 
+FILE *create_stream (const char *fn, const char *mode, mode_t permissions);
+
 struct file_identity *fn_get_identity (const char *file_name);
 void fn_free_identity (struct file_identity *);
 int fn_compare_file_identities (const struct file_identity *,
index 27f7c5c4f57ef15879a6cd2d8e1b5de273593db8..f3a1101cf9801aeae0d90c7d8c36f0f33b178066 100644 (file)
 
 #include <ctype.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <float.h>
 #include <math.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/stat.h>
 #include <time.h>
-#include <unistd.h>
 
 #include <data/case.h>
 #include <data/casewriter-provider.h>
 #include <data/casewriter.h>
 #include <data/dictionary.h>
 #include <data/file-handle-def.h>
+#include <data/file-name.h>
 #include <data/format.h>
 #include <data/missing-values.h>
 #include <data/short-names.h>
@@ -109,30 +108,31 @@ pfm_open_writer (struct file_handle *fh, struct dictionary *dict,
 {
   struct pfm_writer *w = NULL;
   mode_t mode;
-  int fd;
+  FILE *file;
   size_t i;
 
+  /* Open file handle. */
+  if (!fh_open (fh, FH_REF_FILE, "portable file", "we"))
+    return NULL;
+
   /* Create file. */
   mode = S_IRUSR | S_IRGRP | S_IROTH;
   if (opts.create_writeable)
     mode |= S_IWUSR | S_IWGRP | S_IWOTH;
-  fd = open (fh_get_file_name (fh), O_WRONLY | O_CREAT | O_TRUNC, mode);
-  if (fd < 0)
-    goto open_error;
-
-  /* Open file handle. */
-  if (!fh_open (fh, FH_REF_FILE, "portable file", "we"))
-    goto error;
+  file = create_stream (fh_get_file_name (fh), "w", mode);
+  if (file == NULL)
+    {
+      fh_close (fh, "portable file", "we");
+      msg (ME, _("An error occurred while opening \"%s\" for writing "
+                 "as a portable file: %s."),
+           fh_get_file_name (fh), strerror (errno));
+      return NULL;
+    }
 
   /* Initialize data structures. */
   w = xmalloc (sizeof *w);
   w->fh = fh;
-  w->file = fdopen (fd, "w");
-  if (w->file == NULL)
-    {
-      close (fd);
-      goto open_error;
-    }
+  w->file = file;
 
   w->lc = 0;
   w->var_cnt = 0;
@@ -165,19 +165,12 @@ pfm_open_writer (struct file_handle *fh, struct dictionary *dict,
     write_documents (w, dict);
   buf_write (w, "F", 1);
   if (ferror (w->file))
-    goto error;
+    {
+      close_writer (w);
+      return NULL;
+    }
   return casewriter_create (dict_get_next_value_idx (dict),
                             &por_file_casewriter_class, w);
-
- error:
-  close_writer (w);
-  return NULL;
-
- open_error:
-  msg (ME, _("An error occurred while opening \"%s\" for writing "
-             "as a portable file: %s."),
-       fh_get_file_name (fh), strerror (errno));
-  goto error;
 }
 \f
 /* Write NBYTES starting at BUF to the portable file represented by
index d58fd835beaa3a8ade0d750a6909d4aed80c1d3e..21d89a0d696c991f588a5cd3877e20a47f4504a0 100644 (file)
 
 #include <ctype.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/stat.h>
 #include <time.h>
-#include <unistd.h>
 
 #include <libpspp/alloc.h>
 #include <libpspp/float-format.h>
@@ -42,6 +40,7 @@
 #include <data/casewriter.h>
 #include <data/dictionary.h>
 #include <data/file-handle-def.h>
+#include <data/file-name.h>
 #include <data/format.h>
 #include <data/missing-values.h>
 #include <data/settings.h>
@@ -152,7 +151,7 @@ sfm_open_writer (struct file_handle *fh, struct dictionary *d,
 {
   struct sfm_writer *w = NULL;
   mode_t mode;
-  int fd;
+  FILE *file;
   int idx;
   int i;
 
@@ -164,22 +163,27 @@ sfm_open_writer (struct file_handle *fh, struct dictionary *d,
       opts.version = 3;
     }
 
-  /* Create file. */
+  /* Open file handle as an exclusive writer. */
+  if (!fh_open (fh, FH_REF_FILE, "system file", "we"))
+    return NULL;
+
+  /* Create the file on disk. */
   mode = S_IRUSR | S_IRGRP | S_IROTH;
   if (opts.create_writeable)
     mode |= S_IWUSR | S_IWGRP | S_IWOTH;
-  fd = open (fh_get_file_name (fh), O_WRONLY | O_CREAT | O_TRUNC, mode);
-  if (fd < 0)
-    goto open_error;
-
-  /* Open file handle. */
-  if (!fh_open (fh, FH_REF_FILE, "system file", "we"))
-    goto error;
+  file = create_stream (fh_get_file_name (fh), "w", mode);
+  if (file == NULL)
+    {
+      msg (ME, _("Error opening \"%s\" for writing as a system file: %s."),
+           fh_get_file_name (fh), strerror (errno));
+      fh_close (fh, "system file", "we");
+      return NULL;
+    }
 
   /* Create and initialize writer. */
   w = xmalloc (sizeof *w);
   w->fh = fh;
-  w->file = fdopen (fd, "w");
+  w->file = file;
 
   w->compress = opts.compress;
   w->case_cnt = 0;
@@ -193,13 +197,6 @@ sfm_open_writer (struct file_handle *fh, struct dictionary *d,
   w->segment_cnt = sfm_dictionary_to_sfm_vars (d, &w->sfm_vars,
                                                &w->sfm_var_cnt);
 
-  /* Check that file create succeeded. */
-  if (w->file == NULL)
-    {
-      close (fd);
-      goto open_error;
-    }
-
   /* Write the file header. */
   write_header (w, d);
 
@@ -236,19 +233,13 @@ sfm_open_writer (struct file_handle *fh, struct dictionary *d,
   write_int (w, 0);
 
   if (write_error (w))
-    goto error;
+    {
+      close_writer (w);
+      return NULL;
+    }
 
   return casewriter_create (dict_get_next_value_idx (d),
                             &sys_file_casewriter_class, w);
-
- error:
-  close_writer (w);
-  return NULL;
-
- open_error:
-  msg (ME, _("Error opening \"%s\" for writing as a system file: %s."),
-       fh_get_file_name (fh), strerror (errno));
-  goto error;
 }
 
 /* Returns value of X truncated to two least-significant digits. */
index 3a912d56157090be5d73a224e6b043edb01848d7..c98f53e23157cdb03cd45a447414bc901602c6d5 100644 (file)
@@ -1,3 +1,11 @@
+2007-10-08  Ben Pfaff  <blp@gnu.org>
+
+       Bug #21280.  Thanks to John Darrington for review.
+
+       * automake.mk: Add new file.
+
+       * bugs/overwrite-input-file.sh: New test.
+
 2007-09-23  Ben Pfaff  <blp@gnu.org>
 
        Bug #21111.  Reviewed by John Darrington.
index 25eb1ab9ce8c7f3ff31a0ad7b908a2cf891dc7be..5e6e2369cd38b68100608fa3b70083e618d2e741 100644 (file)
@@ -106,6 +106,7 @@ dist_TESTS = \
        tests/bugs/list-overflow.sh \
        tests/bugs/match-files-scratch.sh \
        tests/bugs/multipass.sh \
+       tests/bugs/overwrite-input-file.sh \
        tests/bugs/random.sh \
        tests/bugs/signals.sh \
        tests/bugs/t-test-with-temp.sh \
diff --git a/tests/bugs/overwrite-input-file.sh b/tests/bugs/overwrite-input-file.sh
new file mode 100755 (executable)
index 0000000..56d569c
--- /dev/null
@@ -0,0 +1,125 @@
+#!/bin/sh
+
+# This program tests for a bug that caused SAVE to the file currently
+# being read with GET to truncate the save file to zero length, and
+# similarly for IMPORT/EXPORT.
+
+
+TEMPDIR=/tmp/pspp-tst-$$
+TESTFILE=$TEMPDIR/`basename $0`.sps
+
+# ensure that top_builddir  are absolute
+if [ -z "$top_builddir" ] ; then top_builddir=. ; fi
+if [ -z "$top_srcdir" ] ; then top_srcdir=. ; fi
+top_builddir=`cd $top_builddir; pwd`
+PSPP=$top_builddir/src/ui/terminal/pspp
+
+# ensure that top_srcdir is absolute
+top_srcdir=`cd $top_srcdir; pwd`
+
+STAT_CONFIG_PATH=$top_srcdir/config
+export STAT_CONFIG_PATH
+
+
+cleanup()
+{
+     cd /
+     rm -rf $TEMPDIR
+}
+
+
+fail()
+{
+    echo $activity
+    echo FAILED
+    cleanup;
+    exit 1;
+}
+
+
+no_result()
+{
+    echo $activity
+    echo NO RESULT;
+    cleanup;
+    exit 2;
+}
+
+pass()
+{
+    cleanup;
+    exit 0;
+}
+
+mkdir -p $TEMPDIR
+
+cd $TEMPDIR
+
+activity="create program 1"
+cat > $TESTFILE <<EOF
+DATA LIST /X 1.
+BEGIN DATA.
+1
+2
+3
+4
+5
+END DATA.
+
+SAVE OUTFILE='foo.sav'.
+EXPORT OUTFILE='foo.por'.
+EOF
+if [ $? -ne 0 ] ; then no_result ; fi
+
+
+activity="run program 1"
+$SUPERVISOR $PSPP --testing-mode $TESTFILE
+if [ $? -ne 0 ] ; then no_result ; fi
+
+
+activity="check and save copy of output files"
+# Check that the files are nonzero length.
+test -s foo.sav || fail
+test -s foo.por || fail
+# Save copies of them.
+cp foo.sav foo.sav.backup || fail
+cp foo.por foo.por.backup || fail
+
+
+activity="create program 2"
+cat > $TESTFILE <<EOF
+GET 'foo.sav'.
+SAVE OUTFILE='foo.sav'.
+EOF
+if [ $? -ne 0 ] ; then no_result ; fi
+
+
+activity="run program 2"
+$SUPERVISOR $PSPP --testing-mode $TESTFILE -e /dev/null
+# This should have failed with an error message.
+if [ $? -eq 0 ] ; then no_result ; fi
+
+
+activity="create program 3"
+cat > $TESTFILE <<EOF
+IMPORT 'foo.por'.
+EXPORT OUTFILE='foo.por'.
+EOF
+if [ $? -ne 0 ] ; then no_result ; fi
+
+
+activity="run program 3"
+$SUPERVISOR $PSPP --testing-mode $TESTFILE -e /dev/null
+# This should have failed with an error message.
+if [ $? -eq 0 ] ; then no_result ; fi
+
+
+activity="compare output 1"
+cmp foo.sav foo.sav.backup
+if [ $? -ne 0 ] ; then fail ; fi
+
+activity="compare output 2"
+cmp foo.por foo.por.backup
+if [ $? -ne 0 ] ; then fail ; fi
+
+pass;