From 7ca8a04c4f9b653033b948fd6391a09f5b8c50a7 Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Sun, 31 May 2015 06:21:40 +0300 Subject: [PATCH] Fix potential FPE in selection tests Always initialize dynamic evaluation results to zero during the static analysis pass. The old code tried to do that, but missed a few cases where the same expression was evaluated more than once. Even during static analysis, some dynamic arithmetic expressions are actually evaluated, which could lead to FPE if the memory was uninitialized, even though the results of the evaluation were never used. Part of #1677. Change-Id: I0cf73eb656d58f7ad08f6e12705c6d8a273adf84 --- src/gromacs/selection/compiler.cpp | 37 ++++++++++++++++-------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/gromacs/selection/compiler.cpp b/src/gromacs/selection/compiler.cpp index 87ab39f543..dde38d816e 100644 --- a/src/gromacs/selection/compiler.cpp +++ b/src/gromacs/selection/compiler.cpp @@ -1872,11 +1872,6 @@ init_method(const SelectionTreeElementPointer &sel, t_topology *top, int isize) "Output initialization must be provided for " "SMETH_VARNUMVAL selection methods"); alloc_selection_data(sel, isize, true); - if ((sel->flags & SEL_DYNAMIC) - && sel->v.type != GROUP_VALUE && sel->v.type != POS_VALUE) - { - sel->v.nr = ((sel->flags & SEL_SINGLEVAL) ? 1 : isize); - } /* If the method is char-valued, pre-allocate the strings. */ if (sel->u.expr.method->flags & SMETH_CHARVAL) { @@ -1897,16 +1892,6 @@ init_method(const SelectionTreeElementPointer &sel, t_topology *top, int isize) } } } - /* Clear the values for dynamic output to avoid valgrind warnings. */ - if ((sel->flags & SEL_DYNAMIC) && sel->v.type == REAL_VALUE) - { - int i; - - for (i = 0; i < sel->v.nr; ++i) - { - sel->v.u.r[i] = 0.0; - } - } } } @@ -2146,8 +2131,10 @@ analyze_static(gmx_sel_evaluate_t *data, case SEL_EXPRESSION: case SEL_MODIFIER: + { + const int isize = g ? g->isize : 0; _gmx_sel_evaluate_method_params(data, sel, g); - init_method(sel, data->top, g ? g->isize : 0); + init_method(sel, data->top, isize); if (!(sel->flags & SEL_DYNAMIC)) { sel->cdata->evaluate(data, sel, g); @@ -2165,13 +2152,29 @@ analyze_static(gmx_sel_evaluate_t *data, { sel->cdata->evaluate(data, sel, g); } + else + { + if (sel->v.type != GROUP_VALUE && sel->v.type != POS_VALUE) + { + sel->v.nr = ((sel->flags & SEL_SINGLEVAL) ? 1 : isize); + } + // Clear the values for dynamic output to avoid + // uninitialized memory. + if (sel->v.type == REAL_VALUE) + { + for (int i = 0; i < sel->v.nr; ++i) + { + sel->v.u.r[i] = 0.0; + } + } + } if (bDoMinMax && g) { gmx_ana_index_copy(sel->cdata->gmax, g, true); } } break; - + } case SEL_BOOLEAN: if (!(sel->flags & SEL_DYNAMIC)) { -- 2.22.0