From fbc33cab7b95e1f6795ef3783ce54387925143a4 Mon Sep 17 00:00:00 2001
From: Paul Bauer <paul.bauer.q@gmail.com>
Date: Mon, 16 Apr 2018 16:16:23 +0200
Subject: [PATCH] Fix legacy topology memory error

The change to port mtop to c++ has exposed a number of issues in the
legacy topology data structure, where pointers would be copied from a
temporary mtop object to the local topology, and subsequently
invalidated when mtop was correctly free'd.

This change is a bit of wooden mallet applied to the problem, by brute
forcing the assignment of the values that were previously copied by
pointer.

Fixes #2479

Change-Id: I7d702666fa48cb3840f914ee3bd28787d3caeec8
---
 src/gromacs/topology/mtop_util.cpp | 50 +++++++++++++++++++++++++++++++++++---
 src/gromacs/topology/topology.cpp  |  5 ++++
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/src/gromacs/topology/mtop_util.cpp b/src/gromacs/topology/mtop_util.cpp
index 120677ed8d..cc439f21ab 100644
--- a/src/gromacs/topology/mtop_util.cpp
+++ b/src/gromacs/topology/mtop_util.cpp
@@ -845,21 +845,63 @@ static void gen_local_top(const gmx_mtop_t *mtop,
     gmx_mtop_atomloop_all_t aloop;
     int                     ag;
 
-    top->atomtypes = mtop->atomtypes;
+    /* no copying of pointers possible now */
+
+    top->atomtypes.nr = mtop->atomtypes.nr;
+    if (mtop->atomtypes.atomnumber)
+    {
+        snew(top->atomtypes.atomnumber, top->atomtypes.nr);
+        std::copy(mtop->atomtypes.atomnumber, mtop->atomtypes.atomnumber + top->atomtypes.nr, top->atomtypes.atomnumber);
+    }
+    else
+    {
+        top->atomtypes.atomnumber = nullptr;
+    }
 
     ffp = &mtop->ffparams;
 
     idef                          = &top->idef;
     idef->ntypes                  = ffp->ntypes;
     idef->atnr                    = ffp->atnr;
-    idef->functype                = ffp->functype;
-    idef->iparams                 = ffp->iparams;
+    /* we can no longer copy the pointers to the mtop members,
+     * because they will become invalid as soon as mtop gets free'd.
+     * We also need to make sure to only operate on valid data!
+     */
+
+    if (ffp->functype)
+    {
+        snew(idef->functype, ffp->ntypes);
+        std::copy(ffp->functype, ffp->functype + ffp->ntypes, idef->functype);
+    }
+    else
+    {
+        idef->functype = nullptr;
+    }
+    if (ffp->iparams)
+    {
+        snew(idef->iparams, ffp->ntypes);
+        std::copy(ffp->iparams, ffp->iparams + ffp->ntypes, idef->iparams);
+    }
+    else
+    {
+        idef->iparams = nullptr;
+    }
     idef->iparams_posres          = nullptr;
     idef->iparams_posres_nalloc   = 0;
     idef->iparams_fbposres        = nullptr;
     idef->iparams_fbposres_nalloc = 0;
     idef->fudgeQQ                 = ffp->fudgeQQ;
-    idef->cmap_grid               = ffp->cmap_grid;
+    idef->cmap_grid.ngrid         = ffp->cmap_grid.ngrid;
+    idef->cmap_grid.grid_spacing  = ffp->cmap_grid.grid_spacing;
+    if (ffp->cmap_grid.cmapdata)
+    {
+        snew(idef->cmap_grid.cmapdata, ffp->cmap_grid.ngrid);
+        std::copy(ffp->cmap_grid.cmapdata, ffp->cmap_grid.cmapdata + ffp->cmap_grid.ngrid, idef->cmap_grid.cmapdata);
+    }
+    else
+    {
+        idef->cmap_grid.cmapdata = nullptr;
+    }
     idef->ilsort                  = ilsortUNKNOWN;
 
     init_block(&top->cgs);
diff --git a/src/gromacs/topology/topology.cpp b/src/gromacs/topology/topology.cpp
index 08d6b0676e..af7a5a5033 100644
--- a/src/gromacs/topology/topology.cpp
+++ b/src/gromacs/topology/topology.cpp
@@ -185,6 +185,7 @@ void done_top(t_topology *top)
 {
     sfree(top->idef.functype);
     sfree(top->idef.iparams);
+    sfree(top->idef.cmap_grid.cmapdata);
     for (int f = 0; f < F_NRE; ++f)
     {
         sfree(top->idef.il[f].iatoms);
@@ -221,6 +222,10 @@ void done_top_mtop(t_topology *top, gmx_mtop_t *mtop)
             done_block(&top->mols);
             done_symtab(&top->symtab);
             open_symtab(&mtop->symtab);
+            sfree(top->idef.functype);
+            sfree(top->idef.iparams);
+            sfree(top->idef.cmap_grid.cmapdata);
+            done_atomtypes(&(top->atomtypes));
         }
 
         // Note that the rest of mtop will be freed by the destructor
-- 
2.11.4.GIT