From 7afb798a6f1990cf4af9c5398598376530e2aa67 Mon Sep 17 00:00:00 2001
From: rlar <rlar>
Date: Mon, 23 Jan 2017 19:26:34 +0100
Subject: [PATCH] check for illegal DEVsetup() DEVunsetup() patterns.

Consider the following silent contracts:
1)
 CKTsetup() invocations must be separated by a CKTunsetup() invocation
 But CKTsetup() has an internal flag,
   which will prevent re-invocation of DEVsetup()
 But DEVsetup() will be called during sensitivity analysis,
   bypassing this precaution. It is fatal if this will
   cause another node allocation (CKTmk..()).
   This commit tries to detect such cases.
   (Note: many DEVsetup routines (all ?) have their CKTmk..() invocations
      guarded to avoid reallocation of local nodes,
      see commit f7f454c0a15cf22dbe4dc912b1b9842bbab09a61
        bug fix, fix the guard for device generated internal nodes (via CKTmkVolt())
   )
   FIXME:
     DEVsetup() is seriously obfuscated by these guards.
     If would be far better, if the sensitivity analysis
       wouldn't sidestep into DEVsetup()
     consider a device local variant of the CKTisSetup flag
2)
 DEVunsetup() must delete all, each and every,
   local allocated node in DEVsetup()
 Otherwise CKTmk..() invocations in a following CKTsetup() will
   return duplicate node numbers, effectively shorting device nodes.
 This commit tries to detect incomplete CKTdltNNum() invocations.
3)
  DEVunsetup() must not delete a netlist node.
  This can easily happen in those devices which have optional ports,
    which have code in DEVsetup() which copies node numbers to
    local node variables.
  This commit tries to detect such errors.
---
 src/include/ngspice/cktdefs.h    |  1 +
 src/spicelib/analysis/cktdltn.c  |  5 +++++
 src/spicelib/analysis/cktsens.c  | 13 +++++++++----
 src/spicelib/analysis/cktsetup.c | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/include/ngspice/cktdefs.h b/src/include/ngspice/cktdefs.h
index 48b021db4..ed52166b2 100644
--- a/src/include/ngspice/cktdefs.h
+++ b/src/include/ngspice/cktdefs.h
@@ -152,6 +152,7 @@ struct CKTcircuit {
 
     CKTnode *CKTnodes;          /* ??? */
     CKTnode *CKTlastNode;       /* ??? */
+    CKTnode *prev_CKTlastNode;  /* just before model setup */
 
     /* This define should be somewhere else ??? */
 #define NODENAME(ckt,nodenum) CKTnodName(ckt,nodenum)
diff --git a/src/spicelib/analysis/cktdltn.c b/src/spicelib/analysis/cktdltn.c
index 69f0c988b..deca3b294 100644
--- a/src/spicelib/analysis/cktdltn.c
+++ b/src/spicelib/analysis/cktdltn.c
@@ -23,6 +23,11 @@ CKTdltNNum(CKTcircuit *ckt, int num)
     CKTnode *n, *prev, *node, *sprev;
     int	error;
 
+    if (!ckt->prev_CKTlastNode->number || num <= ckt->prev_CKTlastNode->number) {
+        fprintf(stderr, "Internal Error: CKTdltNNum() removing a non device-local node, this will cause serious problems, please report this issue !\n");
+        controlled_exit(EXIT_FAILURE);
+    }
+
     prev  = NULL;
     node  = NULL;
     sprev = NULL;
diff --git a/src/spicelib/analysis/cktsens.c b/src/spicelib/analysis/cktsens.c
index 49adb07ce..e55c20d3b 100644
--- a/src/spicelib/analysis/cktsens.c
+++ b/src/spicelib/analysis/cktsens.c
@@ -364,10 +364,15 @@ int sens_sens(CKTcircuit *ckt, int restart)
 			ckt->CKTnumStates = sg->istate;
 
 			fn = DEVices[sg->dev]->DEVsetup;
-			if (fn)
-				fn (delta_Y, sg->model, ckt,
-					/* XXXX insert old state base here ?? */
-					&ckt->CKTnumStates);
+			if (fn) {
+                            CKTnode *n = ckt->CKTlastNode;
+                            /* XXXX insert old state base here ?? */
+                            fn (delta_Y, sg->model, ckt, &ckt->CKTnumStates);
+                            if (n != ckt->CKTlastNode) {
+                                fprintf(stderr, "Internal Error: node allocation in DEVsetup() during sensitivity analysis, this will cause serious troubles !, please report this issue !\n");
+                                controlled_exit(EXIT_FAILURE);
+                            }
+                        }
 
 			/* ? CKTsetup would call NIreinit instead */
 			ckt->CKTniState = NISHOULDREORDER | NIACSHOULDREORDER;
diff --git a/src/spicelib/analysis/cktsetup.c b/src/spicelib/analysis/cktsetup.c
index 220c24632..8804f24c4 100644
--- a/src/spicelib/analysis/cktsetup.c
+++ b/src/spicelib/analysis/cktsetup.c
@@ -77,6 +77,13 @@ CKTsetup(CKTcircuit *ckt)
     SetAnalyse("Device Setup", 0);
 #endif
 
+    /* preserve CKTlastNode before invoking DEVsetup()
+     * so we can check for incomplete CKTdltNNum() invocations
+     * during DEVunsetup() causing an erronous circuit matrix
+     *   when reinvoking CKTsetup()
+     */
+    ckt->prev_CKTlastNode = ckt->CKTlastNode;
+
     for (i=0;i<DEVmaxnum;i++) {
         if ( DEVices[i] && DEVices[i]->DEVsetup && ckt->CKThead[i] ) {
             error = DEVices[i]->DEVsetup (matrix, ckt->CKThead[i], ckt,
@@ -163,6 +170,13 @@ CKTunsetup(CKTcircuit *ckt)
                 error = e2;
         }
     }
+
+    if (ckt->prev_CKTlastNode != ckt->CKTlastNode) {
+        fprintf(stderr, "Internal Error: incomplete CKTunsetup(), this will cause serious problems, please report this issue !\n");
+        controlled_exit(EXIT_FAILURE);
+    }
+    ckt->prev_CKTlastNode = NULL;
+
     ckt->CKTisSetup = 0;
     if(error) return(error);