Fix PropTSVecTable out-of-bounds from non-uniform geometry traces (#27)

Root cause is on the parser side. gaussian-*.desc routes THREE parse
types into one property: [GEOMTRACE] from "Input orientation:", another
[GEOMTRACE] from "Z-Matrix orientation:", and [GEOMTRACE1] from
"Standard orientation:". All three run gaussian-*.geomtrace, which
hardcodes 'key: GEOMTRACE' regardless of which parse type invoked it,
and the stored key comes from that line, not the parse type -- so all
three land in GEOMTRACE. (There is no GEOMTRACE1 property at all.)

Those blocks do not have the same atom count. Gaussian prints Z-matrix
dummy centres (atomic number -1) in "Z-Matrix orientation" but omits
them from "Standard orientation". Verified on a real g16 NH3 Z-matrix
optimization with one dummy centre: the same job alternates 5-row and
4-row steps into one PropTSVecTable.

PropTSVecTable then takes its declared shape from the FIRST step only
(DavPropCache::putTSProp writes rows/columns just once, on document
creation; later steps append bare <step> children), and value() checked
row/col against that declared shape without ever reconciling it against
the actual vector length. With p_numRows=5/p_numColumns=3 from a
5-row first step, value(1,4,2) computes index 4*3+2=14 into a 12-element
vector -- reading 2 elements past the end. Reproduced as a real
AddressSanitizer heap-buffer-overflow.

A prior commit (f8e3be6) attributed #27 to PropTSVecVector::value()'s
inverted bounds check. That was a different, real bug; the accessor
named in this issue's title was not in its list, so #27 was still live.

Fixed on both sides:

Parser (the root cause) -- skip dummy centres (atomic number < 0) and
renumber row labels sequentially, in all 8 gaussian-*.geomtrace{,-input}
scripts. Ghost/Bq centres (atomic number 0) are deliberately kept, since
they appear consistently in every orientation block.

This also fixes an independent silent-corruption bug: a dummy can sit
anywhere in the centre list, so every atom after it was given the
WRONG atom's coordinates. On the NH3 fixture the old output was
"1-N 2-Uns 3-H 4-H 5-H" -- a phantom atom (the "Uns" label is
$atomicSymbol[-1], Perl's negative index landing on the last element of
the periodic table) with the real hydrogens shifted down a row. Any
Z-matrix job with dummy atoms had a silently wrong geometry trace,
independent of the crash.

Storage (the backstop) -- across PropTSVecTable, PropVecTable,
PropTSTable and PropTSVecVector: value() now bounds-checks the computed
index against the actual vector size, not just the declared shape;
values(int) bounds-checks its table index (it was unchecked); setValues()
drops steps whose size disagrees with rows*columns and no longer leaks
the previous p_values; and appendTable() refuses malformed tables
instead of warning and pushing them anyway. EE_FATAL (exit(1)) in these
paths is downgraded to a warning plus refusal -- malformed parser output
killed the entire GUI rather than dropping one step, and
PropertyTask::updatePropTSVecTable already drops mismatched steps
silently, so refusing is the consistent behaviour.

Both sides were needed: hardening the accessor alone would leave the
trace silently wrong (dummy coordinates on real atoms), and fixing the
parser alone would leave every other code able to produce the same
over-read through an unvalidated setValues().

src/tdat/test/PropTSVecTableIssue27Test.C is a standalone reproduction
(not wired into CMake, matching the src/dsm/dav/test convention); build
instructions are in its header. Pre-fix it trips ASan, post-fix it is
clean.

Verified: ninja builder and a full ninja are clean. Old vs new parser on
the real fixture gives 5-row/4-row steps before and uniform 4-row steps
after.

NOT verified: no live GUI this session, so the end-to-end path (dummy-
atom job through ECCE, geometry trace in Builder) is untested. The
original reporter's G09 file is unavailable, so their specific trigger
is unconfirmed -- see the follow-up issue on VIB being cast to
PropTSVecTable*, which reports the same assertion location.

Per this bug family's usual caveat, already-parsed jobs are not
retroactively fixed; their stored steps are now dropped with a warning
rather than over-read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: andy

scripts/parsers/gaussian-03.geomtrace

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-03.geomtrace-input

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-09.geomtrace

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-09.geomtrace-input

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-16.geomtrace

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-16.geomtrace-input

@@ -40,10 +40,27 @@ $| = 1;
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$atomicType,$xcoord,$ycoord,$zcoord) =
                                                           split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1, i.e. an "X" in a
+      # Z-matrix).  They are printed in the "Z-Matrix orientation" block but
+      # NOT in the "Input orientation"/"Standard orientation" blocks, and all
+      # three blocks are routed into the single GEOMTRACE property key by the
+      # gaussian-*.desc files (this script hardcodes 'key: GEOMTRACE'
+      # regardless of which parse type invoked it).  Including them therefore
+      # produced a PropTSVecTable whose steps have different row counts --
+      # GitHub issue #27 -- and, because a dummy can sit anywhere in the
+      # centre list, shifted every following atom's coordinates onto the
+      # wrong atom in the geometry trace.  ECCE's chemical system has no
+      # dummy atoms, so dropping them here makes each step line up 1:1 with
+      # the fragment.  Ghost/Bq centres (atomic number 0) are kept: they
+      # appear consistently in every orientation block.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-94.geomtrace

@@ -39,10 +39,16 @@ while (<STDIN>) {
       s/^\s*//;    # Get rid of leading blanks
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$xcoord,$ycoord,$zcoord) = split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1) -- see GitHub issue
+      # #27 and the identical guard in gaussian-{03,09,16}.geomtrace.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

scripts/parsers/gaussian-94.geomtrace-input

@@ -39,10 +39,16 @@ $| = 1;
       s/^\s*//;    # Get rid of leading blanks
       s/D/e/g;     # Use "e" in exponentials
       ($atomIndex,$atomicNumber,$xcoord,$ycoord,$zcoord) = split(/ +/,$_);
+      # Skip Gaussian dummy centres (atomic number -1) -- see GitHub issue
+      # #27 and the identical guard in gaussian-{03,09,16}.geomtrace.
+      next if ($atomicNumber < 0);
       $line = join(" ",($xcoord,$ycoord,$zcoord));
 #    $line = join(" ",($atomicSymbol[$atomicNumber],$xcoord,$ycoord,$zcoord));
       push(@coordlines,$line);    
-      $line = join("-",$atomIndex,($atomicSymbol[$atomicNumber]));
+      # Renumber sequentially: with dummy centres skipped, Gaussian's own
+      # centre numbers no longer match the row index (or ECCE's atom
+      # numbering), which is all this label is for.
+      $line = join("-",++$rowIndex,($atomicSymbol[$atomicNumber]));
       $rowLabels .= " $line";
     } else {
       last;

src/tdat/properties/PropTSTable.C

@@ -106,7 +106,7 @@ double PropTSTable::value(int table, int row, int col) const
   // Return the value for the given table/row/column index
   // Assumes index is within bounds of the vectors
 
-  if (table >= p_values->size() || row >= p_numRows || col >= p_numColumns
+  if (table >= (int)p_values->size() || row >= p_numRows || col >= p_numColumns
       || table < 0 || row < 0 || col < 0) {
     // Same fall-through-after-warning bug shape fixed in PropTable::value()
     // -- EE_WARNING doesn't stop execution, so return early instead of
@@ -117,13 +117,29 @@ double PropTSTable::value(int table, int row, int col) const
   }
 
   int index = row * p_numColumns + col;
+  // p_numRows/p_numColumns describe the declared shape; setValues() accepts
+  // whatever the stored document contains, so an individual table can be
+  // shorter than rows*columns (see GitHub issue #27 and PropTSVecTable.C).
+  // Check the real vector size too - otherwise this is a silent over-read.
+  if (index >= (int)(*p_values)[table].size()) {
+    EE_RT_ASSERT(false, EE_WARNING,
+          "trying to access out-of-bounds index in PropTSTable (this table is "
+          "shorter than the declared rows*columns)");
+    return 0.0;
+  }
   return (*p_values)[table][index];
 }
 
 /////////////////////////////////////////
 
 const vector<double>& PropTSTable::values(int table) const
 {
+  static const vector<double> empty;
+  if (p_values == 0 || table < 0 || table >= (int)p_values->size()) {
+    EE_RT_ASSERT(false, EE_WARNING,
+          "trying to access out-of-bounds table in PropTSTable");
+    return empty;
+  }
   return (*p_values)[table]; // return values for one table
 }
 
@@ -261,16 +277,23 @@ void PropTSTable::appendTable(int rows, int cols,
   }
 
 // make sure input vector matches its specified size
-   if (values.size() != rows * cols) 
-     EE_RT_ASSERT(false, EE_FATAL,
-                  "input vector length does not match specified size");
+   if (values.size() != (size_t)(rows * cols)) {
+     // Was EE_FATAL (exit(1)): malformed parser output killed the whole
+     // application instead of dropping one table.  Refuse the append instead.
+     EE_RT_ASSERT(false, EE_WARNING,
+                  "input vector length does not match specified size - "
+                  "table not appended");
+     return;
+   }
 
 // make sure the table being appended is the same size as the rest
 // of the tables in the vector
-   if (rows != p_numRows || cols != p_numColumns)
-      EE_RT_ASSERT(false, EE_FATAL,
+   if (rows != p_numRows || cols != p_numColumns) {
+      EE_RT_ASSERT(false, EE_WARNING,
             "trying to append a table that is not the same size as "
-            "the other tables in the vector");
+            "the other tables in the vector - table not appended");
+      return;
+   }
 
    p_values->push_back(values);
 }