From db161068bfd5b6685f909f39b2a4701ed4941b03 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@cs.stanford.edu>
Date: Mon, 20 May 2013 23:49:54 -0700
Subject: [PATCH] DELETE VARIABLES: Fix crash with FILTER.

FILTER has the surprising property that it introduces a temporary
transformation, even if proc_make_temporary_transformations_permanent()
was previously called.  This causes a crash if the procedure implementation
really relies on there being no temporary transformations.

This commit fixes the problem in the procedures I was able to identify as
having it, adds a comment to proc_make_temporary_transformations_permanent()
to make it a little less likely to happen again, and adds a test
specifically to check for regression on DELETE VARIABLES.

Reported by John Darrington.
Bug #38843.
---
 src/data/dataset.c                            | 10 +++++--
 src/language/data-io/combine-files.c          |  2 +-
 src/language/dictionary/delete-variables.c    |  4 +--
 src/language/stats/descriptives.c             |  3 +-
 src/language/stats/flip.c                     |  2 +-
 src/language/stats/regression.c               |  9 ++++--
 tests/automake.mk                             |  1 +
 tests/language/dictionary/delete-variables.at | 29 +++++++++++++++++++
 8 files changed, 49 insertions(+), 11 deletions(-)
 create mode 100644 tests/language/dictionary/delete-variables.at

diff --git a/src/data/dataset.c b/src/data/dataset.c
index 7448bd38ef..7a5a6a4a6a 100644
--- a/src/data/dataset.c
+++ b/src/data/dataset.c
@@ -746,9 +746,13 @@ proc_start_temporary_transformations (struct dataset *ds)
     }
 }
 
-/* Converts all the temporary transformations, if any, to
-   permanent transformations.  Further transformations will be
-   permanent.
+/* Converts all the temporary transformations, if any, to permanent
+   transformations.  Further transformations will be permanent.
+
+   The FILTER command is implemented as a temporary transformation, so a
+   procedure that uses this function should usually use proc_open_filtering()
+   with FILTER false, instead of plain proc_open().
+
    Returns true if anything changed, false otherwise. */
 bool
 proc_make_temporary_transformations_permanent (struct dataset *ds)
diff --git a/src/language/data-io/combine-files.c b/src/language/data-io/combine-files.c
index d803ccdc86..9d353c81a1 100644
--- a/src/language/data-io/combine-files.c
+++ b/src/language/data-io/combine-files.c
@@ -445,7 +445,7 @@ combine_files (enum comb_command_type command,
           if (active_file == NULL)
             {
               proc_discard_output (ds);
-              file->reader = active_file = proc_open (ds);
+              file->reader = active_file = proc_open_filtering (ds, false);
             }
           else
             file->reader = casereader_clone (active_file);
diff --git a/src/language/dictionary/delete-variables.c b/src/language/dictionary/delete-variables.c
index c9040adc93..ec5d453715 100644
--- a/src/language/dictionary/delete-variables.c
+++ b/src/language/dictionary/delete-variables.c
@@ -1,5 +1,5 @@
 /* PSPP - a program for statistical analysis.
-   Copyright (C) 2006, 2007, 2010, 2011 Free Software Foundation, Inc.
+   Copyright (C) 2006, 2007, 2010, 2011, 2013 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
@@ -50,7 +50,7 @@ cmd_delete_variables (struct lexer *lexer, struct dataset *ds)
       goto error;
     }
 
-  ok = casereader_destroy (proc_open (ds));
+  ok = casereader_destroy (proc_open_filtering (ds, false));
   ok = proc_commit (ds) && ok;
   if (!ok)
     goto error;
diff --git a/src/language/stats/descriptives.c b/src/language/stats/descriptives.c
index de05d2b89b..413826cc9c 100644
--- a/src/language/stats/descriptives.c
+++ b/src/language/stats/descriptives.c
@@ -441,7 +441,8 @@ cmd_descriptives (struct lexer *lexer, struct dataset *ds)
       dsc->vars[i].moments = moments_create (dsc->max_moment);
 
   /* Data pass. */
-  grouper = casegrouper_create_splits (proc_open (ds), dict);
+  grouper = casegrouper_create_splits (proc_open_filtering (ds, z_cnt == 0),
+                                       dict);
   while (casegrouper_get_next_group (grouper, &group))
     calc_descriptives (dsc, group, ds);
   ok = casegrouper_destroy (grouper);
diff --git a/src/language/stats/flip.c b/src/language/stats/flip.c
index 6378ffeb90..e1ae9330dd 100644
--- a/src/language/stats/flip.c
+++ b/src/language/stats/flip.c
@@ -166,7 +166,7 @@ cmd_flip (struct lexer *lexer, struct dataset *ds)
   flip->encoding = dict_get_encoding (new_dict);
   dict_clear (new_dict);
 
-  input = proc_open (ds);
+  input = proc_open_filtering (ds, false);
   while ((c = casereader_read (input)) != NULL)
     {
       flip->n_cases++;
diff --git a/src/language/stats/regression.c b/src/language/stats/regression.c
index b63b28e876..1ec00c9f33 100644
--- a/src/language/stats/regression.c
+++ b/src/language/stats/regression.c
@@ -270,6 +270,7 @@ cmd_regression (struct lexer *lexer, struct dataset *ds)
   int k;
   struct regression regression;
   const struct dictionary *dict = dataset_dict (ds);
+  bool save;
 
   memset (&regression, 0, sizeof (struct regression));
 
@@ -388,7 +389,8 @@ cmd_regression (struct lexer *lexer, struct dataset *ds)
 
   regression.models = xcalloc (regression.n_dep_vars, sizeof *regression.models);
 
-  if (regression.pred || regression.resid)
+  save = regression.pred || regression.resid;
+  if (save)
     {
       if (proc_make_temporary_transformations_permanent (ds))
         msg (SW, _("REGRESSION with SAVE ignores TEMPORARY.  "
@@ -400,14 +402,15 @@ cmd_regression (struct lexer *lexer, struct dataset *ds)
     struct casereader *group;
     bool ok;
     
-    grouper = casegrouper_create_splits (proc_open (ds), dict);
+    grouper = casegrouper_create_splits (proc_open_filtering (ds, !save),
+                                         dict);
     while (casegrouper_get_next_group (grouper, &group))
       run_regression (&regression, group);
     ok = casegrouper_destroy (grouper);
     ok = proc_commit (ds) && ok;
   }
 
-  if (regression.pred || regression.resid )
+  if (save)
     {
       subcommand_save (&regression);
     }
diff --git a/tests/automake.mk b/tests/automake.mk
index 3ec278c6ff..47951c5392 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -266,6 +266,7 @@ TESTSUITE_AT = \
 	tests/language/data-io/save-translate.at \
 	tests/language/data-io/update.at \
 	tests/language/dictionary/attributes.at \
+	tests/language/dictionary/delete-variables.at \
 	tests/language/dictionary/formats.at \
 	tests/language/dictionary/missing-values.at \
 	tests/language/dictionary/mrsets.at \
diff --git a/tests/language/dictionary/delete-variables.at b/tests/language/dictionary/delete-variables.at
new file mode 100644
index 0000000000..48bc40a69f
--- /dev/null
+++ b/tests/language/dictionary/delete-variables.at
@@ -0,0 +1,29 @@
+AT_BANNER([DELETE VARIABLES])
+
+dnl Checks for regressions against a crash reported in bug #38843.
+AT_SETUP([DELETE VARIABLES with FILTER])
+AT_DATA([delete-variables.sps], [dnl
+DATA LIST LIST /a b.
+BEGIN DATA.
+1 3
+4 6
+7 9
+END DATA.
+
+FILTER BY b.
+DELETE VARIABLES a.
+LIST.
+])
+AT_CHECK([pspp -O format=csv delete-variables.sps], [0], [dnl
+Table: Reading free-form data from INLINE.
+Variable,Format
+a,F8.0
+b,F8.0
+
+Table: Data List
+b
+3.00
+6.00
+9.00
+])
+AT_CLEANUP
-- 
2.30.2