From db161068bfd5b6685f909f39b2a4701ed4941b03 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 20 May 2013 23:49:54 -0700 Subject: [PATCH 1/1] 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 (®ression, 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 (®ression, group); ok = casegrouper_destroy (grouper); ok = proc_commit (ds) && ok; } - if (regression.pred || regression.resid ) + if (save) { subcommand_save (®ression); } 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