From 91090b4762710ae236704d7f08426d30e840e28c Mon Sep 17 00:00:00 2001 From: nrnhines Date: Mon, 16 Sep 2024 16:55:52 -0400 Subject: [PATCH] CellBuild topology page easily crashes when deleting sections with continuous create on (#3005) * almost complete coverage of secref.cpp --- share/lib/hoc/celbild/celset.hoc | 18 +++- share/lib/hoc/celbild/celtopol.hoc | 10 ++- src/nrnoc/secref.cpp | 17 +++- test/hoctests/tests/test_secref.py | 138 +++++++++++++++++++++++++++++ 4 files changed, 176 insertions(+), 7 deletions(-) create mode 100644 test/hoctests/tests/test_secref.py diff --git a/share/lib/hoc/celbild/celset.hoc b/share/lib/hoc/celbild/celset.hoc index c13aedc87c..fb4fa31896 100755 --- a/share/lib/hoc/celbild/celset.hoc +++ b/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) { diff --git a/share/lib/hoc/celbild/celtopol.hoc b/share/lib/hoc/celbild/celtopol.hoc index bedb38e70e..57c9b32b0c 100755 --- a/share/lib/hoc/celbild/celtopol.hoc +++ b/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) } } diff --git a/src/nrnoc/secref.cpp b/src/nrnoc/secref.cpp index a0ec032b91..87071ed2a8 100644 --- a/src/nrnoc/secref.cpp +++ b/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(); + 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(nullptr); + } sec->prop->dparam[0] = static_cast(nullptr); return 1.; } @@ -169,6 +178,10 @@ static double s_rename(void* v) { hoc_objectdata = obdsav; return 0; } + if (sec->prop->dparam[0].get()) { + Printf("Item %d of second list arg, %s, must first be unnamed\n", i, secname(sec)); + return 0; + } qsec = sec->prop->dparam[8].get(); 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); diff --git a/test/hoctests/tests/test_secref.py b/test/hoctests/tests/test_secref.py new file mode 100644 index 0000000000..2fdf7c605f --- /dev/null +++ b/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