CellBuild topology page easily crashes when deleting sections with co…

…ntinuous create on (#3005)


* almost complete coverage of secref.cpp

share/lib/hoc/celbild/celset.hoc

@@ -1,5 +1,6 @@
 begintemplate SNList // Like a Section List
 public name, name_, list, differ, name_differ, add, del_all, ml, geo, geobox
+public remove
 public domainparms, is_subset
 public export, sectionlist
 objref list, this, ml, geo, geobox, domainparms, sectionlist
@@ -14,6 +15,15 @@ proc init() {
 	name_differ = 0
 }
 func is_subset() { return 1 }
+
+proc remove() {local i
+	for (i = list.count - 1; i >= 0; i = i-1) {
+		if (list.o(i) == $o1) {
+			list.remove(i)
+		}
+	}
+}
+
 proc add() {
 	if ($o1.sets.index(this) == -1) { // only add if not already in list
 		list.append($o1)
@@ -70,7 +80,7 @@ endtemplate SDIDispListItem
 begintemplate SectionSubsets
 public topol, hbox, g, update, snlist, save_data, showsel, bild, pr
 public consist, esub, cexport, dpi, have_domainparm, xmlwrite
-public domain_iter, delsdi, neuroml
+public domain_iter, delsdi, neuroml, remove
 objref g, this, snlist, tobj, tobj1, tobj2, bild, smap, imap
 objref hb1, hb2, vb1, vb2, vb3, dk1, dk2
 objref sdidisplist, nil, dpi
@@ -93,6 +103,12 @@ proc init() {
 	dk1.flip_to(0)
 }
 
+proc remove() {local i
+	for i = 0, snlist.count-1 {
+		snlist.o(i).remove($o1)
+	}
+}
+
 func have_domainparm() {local i
 	for i = 0, snlist.count-1 {
 		if (object_id(snlist.object(i).domainparms) != 0) {

share/lib/hoc/celbild/celtopol.hoc

@@ -479,7 +479,10 @@ proc del() {local i
 	//if only one section and it is selected then delete it
 	if (slist.count == 1) {
 		if (slist.object(0).selected == 1) {
-			if (etop) slist.object(0).export_rm()
+			if (etop) {
+				bild.subsets.remove(slist.object(0))
+				slist.object(0).export_rm()
+			}
 			slist.remove(0)
 		}
 	}
@@ -494,7 +497,10 @@ proc del() {local i
 	for (i=slist.count-1; i >= 1; i -=1) {// can't delete root
 		tobj = slist.object(i)
 		if (tobj.selected == 1) {
-			if (etop) tobj.export_rm()
+			if (etop) {
+				bild.subsets.remove(tobj)
+				tobj.export_rm()
+			}
 			slist.remove(i)
 		}
 	}

src/nrnoc/secref.cpp

@@ -73,6 +73,15 @@ static double s_unname(void* v) {
 #endif
     pitm = sec2pitm(sec);
     *pitm = (hoc_Item*) 0;
+    auto* ob = sec->prop->dparam[6].get<Object*>();
+    if (ob) {
+        // Avoid use of freed memory in CellBuild when making a section
+        // after deleting a section when continuous create is on.
+        // CellBuild repeatedly creates a single CellBuildTopology[0].dummy
+        // section and then unnames and renamed it at the top level.
+        ob->secelm_ = nullptr;
+        sec->prop->dparam[6] = static_cast<Object*>(nullptr);
+    }
     sec->prop->dparam[0] = static_cast<Symbol*>(nullptr);
     return 1.;
 }
@@ -169,6 +178,10 @@ static double s_rename(void* v) {
                 hoc_objectdata = obdsav;
                 return 0;
             }
+            if (sec->prop->dparam[0].get<Symbol*>()) {
+                Printf("Item %d of second list arg, %s, must first be unnamed\n", i, secname(sec));
+                return 0;
+            }
             qsec = sec->prop->dparam[8].get<hoc_Item*>();
             sec->prop->dparam[0] = sym;
             sec->prop->dparam[5] = i;
@@ -231,13 +244,11 @@ int nrn_secref_nchild(Section* sec) {
 }
 
 static double s_nchild(void* v) {
-    int n;
     hoc_return_type_code = 1; /* integer */
     return (double) nrn_secref_nchild((Section*) v);
 }
 
 static double s_has_parent(void* v) {
-    int n;
     Section* sec = (Section*) v;
     hoc_return_type_code = 2; /* boolean */
     if (!sec->prop) {
@@ -247,7 +258,6 @@ static double s_has_parent(void* v) {
 }
 
 static double s_has_trueparent(void* v) {
-    int n;
     Section* sec = (Section*) v;
     hoc_return_type_code = 2; /* boolean */
     if (!sec->prop) {
@@ -257,7 +267,6 @@ static double s_has_trueparent(void* v) {
 }
 
 static double s_exists(void* v) {
-    int n;
     hoc_return_type_code = 2; /* boolean */
     Section* sec = (Section*) v;
     return (double) (sec->prop != (Prop*) 0);

test/hoctests/tests/test_secref.py

@@ -0,0 +1,138 @@
+from neuron import h
+from neuron.expect_hocerr import expect_err, set_quiet
+
+set_quiet(0)
+
+# SectionRef unname and rename are undocumented but used in implementation
+# of CellBuild
+
+h("create a")
+sr = h.SectionRef(sec=h.a)
+assert sr.sec.name() == "a"
+assert sr.rename("b") == 0  # must first be unnamed
+sr.unname()
+expect_err("sr.unname()")  # already unnamed
+h("foo = 1")
+assert sr.rename("foo") == 0  # already exists
+sr.rename("b")
+assert sr.sec.name() == "b"
+h.delete_section(sec=h.b)
+assert sr.rename("c") == 0
+
+h("create a, b[3]")
+sr = h.SectionRef(sec=h.a)
+sr.unname()
+sr.rename("b")  # no longer an array and the old 3 are deleted
+assert sr.sec.name() == "b"
+h.delete_section(sec=h.b)
+
+# rename a bunch as an array
+h("create a, b, c, d")
+sr = None  # let's start over at 0
+sr = [h.SectionRef(sec=i) for i in h.allsec()]
+assert len(sr) == 4
+
+# the ones to rename into a new array
+lst = h.List()
+for i in [0, 2, 3]:
+    lst.append(sr[i])
+sr[0].unname()
+assert sr[0].rename("x", lst) == 0  # have not unnamed c and d
+for i in lst:
+    i.unname()
+assert sr[0].rename("x", lst) == 1
+h.topology()
+print("cover a deleted section message")
+h.delete_section(sec=h.x[1])
+sr[0].unname()
+assert sr[0].rename("y", lst) == 0
+h.topology()
+
+
+# cleanup
+def cleanup():
+    for sec in h.allsec():
+        h.delete_section(sec=sec)
+
+
+cleanup()
+
+# Python sections cannot be unnamed or renamed
+a = h.Section("a")
+sr = h.SectionRef(sec=a)
+assert sr.unname() == 0
+assert a.name() == "a"
+assert sr.rename("b") == 0
+assert a.name() == "a"
+cleanup()
+
+
+# remaining "documented" methods
+sr = None
+lst = None
+h("create a, a1, a2, a3")
+h.a1.connect(h.a(0))
+h.a2.connect(h.a(0.5))
+h.a3.connect(h.a(1))
+h.topology()
+sr = [h.SectionRef(sec=sec) for sec in h.allsec()]
+assert sr[0].nchild() == 3
+assert sr[0].has_parent() is False
+assert sr[0].has_trueparent() is False
+assert sr[1].has_parent() is True
+assert sr[1].has_trueparent() is False
+assert sr[2].has_parent() is True
+assert sr[2].has_trueparent() is True
+
+assert sr[0].is_cas(sec=sr[0].sec) is True
+assert sr[1].is_cas(sec=sr[0].sec) is False
+
+assert sr[1].parent == sr[0].sec
+assert sr[2].trueparent == sr[0].sec
+assert sr[2].root == sr[0].sec
+for i in range(sr[0].nchild()):
+    assert sr[0].child[i] == sr[i + 1].sec
+
+# can only get to these lines via hoc? (when implementation uses nrn_inpython)
+# May want to eliminate the nrn_inpython fragments for complete coverage
+# I believe hoc_execerror would work also in the python context.
+assert h("%s.child[3] print secname()" % sr[0]) == False
+assert h("%s.child[1][1] print secname()" % sr[0]) == False
+assert h("%s.child print secname()" % sr[0]) == False
+assert h("%s.parent print secname()" % sr[0]) == False
+assert h("%s.trueparent print secname()" % sr[0]) == False
+
+
+h.topology()
+
+h.delete_section(sec=sr[0].sec)
+expect_err("sr[0].nchild()")
+expect_err("sr[0].has_parent()")
+expect_err("sr[0].has_trueparent()")
+expect_err("sr[0].is_cas()")
+expect_err("sr[1].parent")
+expect_err("sr[2].trueparent")
+assert sr[2].root == sr[2].sec
+expect_err("sr[1].child[5]")
+
+h("create soma")
+sr = h.SectionRef(sec=h.soma)
+assert sr.exists() is True
+h.delete_section(sec=h.soma)
+assert sr.exists() is False
+
+h(
+    """
+begintemplate Cell
+public soma
+create soma
+endtemplate Cell
+"""
+)
+
+cell = h.Cell()
+sr = h.SectionRef(sec=cell.soma)
+print(sr.sec)
+sr.unname()
+
+h("create c")  # fix segfault