From: Teemu Murtola <teemu.murtola@gmail.com>
Date: Sun, 31 May 2015 03:21:40 +0000 (+0300)
Subject: Fix potential FPE in selection tests
X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?a=commitdiff_plain;h=7ca8a04c4f9b653033b948fd6391a09f5b8c50a7;p=alexxy%2Fgromacs.git

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
---

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))
             {