T-TEST: Fix use-after-free with TEMPORARY and independent samples.

When TEMPORARY is in effect, proc_commit() destroys the temporary
dictionary.  This means that any procedure that does not somehow disable
temporary transformations and refers to a variable following proc_commit()
has a use-after-free error.

T-TEST has two different bugs of this type.  First, the loop that destroys
group statistics refers to destroyed variables.  This commit fixes this
problem by instead using variable aux data destructors to destroy group
statistics.

Second, when there is an independent variable, destroying its values
requires knowing the variable's width.  This commit fixes this problem by
destroying the values before calling proc_commit().

The AUTORECODE, DESCRIPTIVES, RANK, and REGRESSION procedures appear to
have similar issues (not fixed by this commit).

Reported by Jeremy Lavergne <jeremy@lavergne.gotdns.org>.

diff --git a/src/language/stats/t-test.q b/src/language/stats/t-test.q
index 3f3fa5d6e0d795f64940def95c82e62fec40705e..9bfa4f975d5c3ce399d511fdd26c47796d2b8dfc 100644 (file)
--- a/src/language/stats/t-test.q
+++ b/src/language/stats/t-test.q
@@ -1,5 +1,5 @@
 /* PSPP - a program for statistical analysis.
-   Copyright (C) 1997-9, 2000, 2009, 2010 Free Software Foundation, Inc.
+   Copyright (C) 1997-9, 2000, 2009, 2010, 2011 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
@@ -164,6 +164,8 @@ static int compare_group_binary (const struct group_statistics *a,
 static unsigned hash_group_binary (const struct group_statistics *g,
                                   const struct t_test_proc *p);
 
+static void t_test_proc_destroy (struct t_test_proc *proc);
+
 int
 cmd_t_test (struct lexer *lexer, struct dataset *ds)
 {
@@ -189,7 +191,7 @@ cmd_t_test (struct lexer *lexer, struct dataset *ds)
     {
       msg (SE, _("Exactly one of TESTVAL, GROUPS and PAIRS subcommands "
                  "must be specified."));
-      goto done;
+      goto error;
     }
 
   proc.mode = (cmd.sbc_testval ? T_1_SAMPLE
@@ -209,7 +211,7 @@ cmd_t_test (struct lexer *lexer, struct dataset *ds)
       if (cmd.sbc_variables)
        {
          msg (SE, _("VARIABLES subcommand may not be used with PAIRS."));
-          goto done;
+          goto error;
        }
 
       /* Fill proc.vars with the unique variables from pairs. */
@@ -228,7 +230,7 @@ cmd_t_test (struct lexer *lexer, struct dataset *ds)
       if (!cmd.n_variables)
         {
           msg (SE, _("One or more VARIABLES must be specified."));
-          goto done;
+          goto error;
         }
       proc.n_vars = cmd.n_variables;
       proc.vars = cmd.v_variables;
@@ -240,31 +242,33 @@ cmd_t_test (struct lexer *lexer, struct dataset *ds)
   while (casegrouper_get_next_group (grouper, &group))
     calculate (&proc, group, ds);
   ok = casegrouper_destroy (grouper);
+
+  /* Free 'proc' then commit the procedure.  Must happen in this order because
+     if proc->indep_var was created by a temporary transformation then
+     committing will destroy it.  */
+  t_test_proc_destroy (&proc);
   ok = proc_commit (ds) && ok;
 
-  if (proc.mode == T_IND_SAMPLES)
-    {
-      int v;
-      /* Destroy any group statistics we created */
-      for (v = 0; v < proc.n_vars; v++)
-       {
-         struct group_proc *grpp = group_proc_get (proc.vars[v]);
-         hsh_destroy (grpp->group_hash);
-       }
-    }
+  return ok ? CMD_SUCCESS : CMD_FAILURE;
 
-done:
+error:
   free_t_test (&cmd);
 parse_failed:
-  if (proc.indep_var != NULL)
+  t_test_proc_destroy (&proc);
+  return CMD_FAILURE;
+}
+
+static void
+t_test_proc_destroy (struct t_test_proc *proc)
+{
+  if (proc->indep_var != NULL)
     {
-      int width = var_get_width (proc.indep_var);
-      value_destroy (&proc.g_value[0], width);
-      value_destroy (&proc.g_value[1], width);
+      int width = var_get_width (proc->indep_var);
+      value_destroy (&proc->g_value[0], width);
+      value_destroy (&proc->g_value[1], width);
     }
-  free (proc.vars);
-  free (proc.pairs);
-  return ok ? CMD_SUCCESS : CMD_FAILURE;
+  free (proc->vars);
+  free (proc->pairs);
 }
 
 static int

diff --git a/src/math/group.c b/src/math/group.c
index ea4dbf95c365599cc6192390750dd9098c43e897..fabd4db289b42a46eab8368b35d82fd3ea532bb6 100644 (file)
--- a/src/math/group.c
+++ b/src/math/group.c
@@ -32,6 +32,14 @@ free_group (struct group_statistics *v, void *aux UNUSED)
   free(v);
 }
 
+static void
+group_proc_dtor (struct variable *var)
+{
+  struct group_proc *group = var_detach_aux (var);
+
+  hsh_destroy (group->group_hash);
+  free (group);
+}
 
 struct group_proc *
 group_proc_get (const struct variable *v)
@@ -40,8 +48,8 @@ group_proc_get (const struct variable *v)
   struct group_proc *group = var_get_aux (v);
   if (group == NULL)
     {
-      group = xmalloc (sizeof (struct group_proc));
-      var_attach_aux (v, group, var_dtor_free);
+      group = xzalloc (sizeof *group);
+      var_attach_aux (v, group, group_proc_dtor);
     }
   return group;
 }