From 12212dfd8afc14405274703b511c32d362ec0ab5 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 10 Mar 2011 22:53:17 -0800 Subject: [PATCH] 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 . --- src/language/stats/t-test.q | 48 ++++++++++++++++++++----------------- src/math/group.c | 12 ++++++++-- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/language/stats/t-test.q b/src/language/stats/t-test.q index 3f3fa5d6..9bfa4f97 100644 --- 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 ea4dbf95..fabd4db2 100644 --- 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; } -- 2.30.2