[Pspp-cvs] pspp src/data/ChangeLog src/data/file-name.c sr...

[Ben Pfaff]

CVSROOT:        /cvsroot/pspp
Module name:    pspp
Changes by:     Ben Pfaff <blp> 07/10/09 03:50:21

Modified files:
        src/data       : ChangeLog file-name.c file-name.h 
                         por-file-writer.c sys-file-writer.c 
        tests          : ChangeLog automake.mk 
Added files:
        tests/bugs     : overwrite-input-file.sh 

Log message:
        Fix bug #21280.  Thanks to John Darrington for review.
        
        * 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.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/ChangeLog?cvsroot=pspp&r1=1.162&r2=1.163
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/file-name.c?cvsroot=pspp&r1=1.17&r2=1.18
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/file-name.h?cvsroot=pspp&r1=1.10&r2=1.11
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/por-file-writer.c?cvsroot=pspp&r1=1.17&r2=1.18
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/sys-file-writer.c?cvsroot=pspp&r1=1.31&r2=1.32
http://cvs.savannah.gnu.org/viewcvs/pspp/tests/ChangeLog?cvsroot=pspp&r1=1.111&r2=1.112
http://cvs.savannah.gnu.org/viewcvs/pspp/tests/automake.mk?cvsroot=pspp&r1=1.40&r2=1.41
http://cvs.savannah.gnu.org/viewcvs/pspp/tests/bugs/overwrite-input-file.sh?cvsroot=pspp&rev=1.1

Patches:
Index: src/data/ChangeLog
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/ChangeLog,v
retrieving revision 1.162
retrieving revision 1.163
diff -u -b -r1.162 -r1.163
--- src/data/ChangeLog  2 Oct 2007 04:13:07 -0000       1.162
+++ src/data/ChangeLog  9 Oct 2007 03:50:20 -0000       1.163
@@ -1,3 +1,16 @@
+2007-10-08  Ben Pfaff  <address@hidden>
+
+       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  <address@hidden>
 
        Fix bug #21192.  Thanks to John Darrington for review.

Index: src/data/file-name.c
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/file-name.c,v
retrieving revision 1.17
retrieving revision 1.18
diff -u -b -r1.17 -r1.18
--- src/data/file-name.c        16 Aug 2007 06:30:23 -0000      1.17
+++ src/data/file-name.c        9 Oct 2007 03:50:20 -0000       1.18
@@ -20,8 +20,10 @@
 
 #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 @@
     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: src/data/file-name.h
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/file-name.h,v
retrieving revision 1.10
retrieving revision 1.11
diff -u -b -r1.10 -r1.11
--- src/data/file-name.h        7 Jul 2007 06:14:08 -0000       1.10
+++ src/data/file-name.h        9 Oct 2007 03:50:20 -0000       1.11
@@ -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 @@
 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: src/data/por-file-writer.c
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/por-file-writer.c,v
retrieving revision 1.17
retrieving revision 1.18
diff -u -b -r1.17 -r1.18
--- src/data/por-file-writer.c  13 Aug 2007 00:41:35 -0000      1.17
+++ src/data/por-file-writer.c  9 Oct 2007 03:50:20 -0000       1.18
@@ -19,20 +19,19 @@
 
 #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 @@
 {
   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 @@
     write_documents (w, dict);
   buf_write (w, "F", 1);
   if (ferror (w->file))
-    goto error;
-  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;
+    }
+  return casewriter_create (dict_get_next_value_idx (dict),
+                            &por_file_casewriter_class, w);
 }
 
 /* Write NBYTES starting at BUF to the portable file represented by

Index: src/data/sys-file-writer.c
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/sys-file-writer.c,v
retrieving revision 1.31
retrieving revision 1.32
diff -u -b -r1.31 -r1.32
--- src/data/sys-file-writer.c  13 Aug 2007 00:41:35 -0000      1.31
+++ src/data/sys-file-writer.c  9 Oct 2007 03:50:20 -0000       1.32
@@ -21,12 +21,10 @@
 
 #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 @@
 {
   struct sfm_writer *w = NULL;
   mode_t mode;
-  int fd;
+  FILE *file;
   int idx;
   int i;
 
@@ -164,22 +163,27 @@
       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 @@
   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 @@
   write_int (w, 0);
 
   if (write_error (w))
-    goto error;
-
-  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;
+  return casewriter_create (dict_get_next_value_idx (d),
+                            &sys_file_casewriter_class, w);
 }
 
 /* Returns value of X truncated to two least-significant digits. */

Index: tests/ChangeLog
===================================================================
RCS file: /cvsroot/pspp/pspp/tests/ChangeLog,v
retrieving revision 1.111
retrieving revision 1.112
diff -u -b -r1.111 -r1.112
--- tests/ChangeLog     23 Sep 2007 16:47:28 -0000      1.111
+++ tests/ChangeLog     9 Oct 2007 03:50:20 -0000       1.112
@@ -1,3 +1,11 @@
+2007-10-08  Ben Pfaff  <address@hidden>
+
+       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  <address@hidden>
 
        Bug #21111.  Reviewed by John Darrington.

Index: tests/automake.mk
===================================================================
RCS file: /cvsroot/pspp/pspp/tests/automake.mk,v
retrieving revision 1.40
retrieving revision 1.41
diff -u -b -r1.40 -r1.41
--- tests/automake.mk   23 Sep 2007 16:47:28 -0000      1.40
+++ tests/automake.mk   9 Oct 2007 03:50:20 -0000       1.41
@@ -106,6 +106,7 @@
        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 \

Index: tests/bugs/overwrite-input-file.sh
===================================================================
RCS file: tests/bugs/overwrite-input-file.sh
diff -N tests/bugs/overwrite-input-file.sh
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ tests/bugs/overwrite-input-file.sh  9 Oct 2007 03:50:21 -0000       1.1
@@ -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;