From 441d7a0854c5836e1a14e3c215899bcc3a9531c7 Mon Sep 17 00:00:00 2001 From: Michael Hines Date: Wed, 12 Mar 2025 20:33:59 -0400 Subject: [PATCH 1/3] eliminate Sprintf warnings --- src/modlunit/model.cpp | 2 +- src/nocmodl/io.cpp | 14 +++++++------- src/nocmodl/kinetic.cpp | 24 +++++++++++++----------- src/nocmodl/nocpout.cpp | 10 +++++----- src/oc/mk_hocusr_h.py | 2 +- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/modlunit/model.cpp b/src/modlunit/model.cpp index c10e3f39b4..b8309a1042 100644 --- a/src/modlunit/model.cpp +++ b/src/modlunit/model.cpp @@ -116,7 +116,7 @@ int main(int argc, char* argv[]) { static void openfiles(int argc, char* argv[]) { char *cp, modprefix[NRN_BUFSIZE - 5]; if (argc > 1) { - assert(strlen(argv[1]) < NRN_BUFSIZE); + assert(strlen(argv[1]) < NRN_BUFSIZE - 5); Sprintf(modprefix, "%s", argv[1]); cp = strstr(modprefix, ".mod"); if (cp) { diff --git a/src/nocmodl/io.cpp b/src/nocmodl/io.cpp index 313fbb1348..a1f2dd7964 100644 --- a/src/nocmodl/io.cpp +++ b/src/nocmodl/io.cpp @@ -216,15 +216,15 @@ void unGets(char* buf) /* all this because we don't have an ENDBLOCK char* current_line() { /* assumes we actually want the previous line */ static char buf[NRN_BUFSIZE]; char* p; - Sprintf(buf, - "at line %d in file %s:\\n%s", - linenum - 1, + assert(Sprintf(buf, + "at line %d in file %s:\\n%s", + linenum - 1, #if !defined(NRN_AVOID_ABSOLUTE_PATHS) - finname, + finname, #else - fs::absolute(finname).filename().c_str(), + fs::absolute(finname).filename().c_str(), #endif - inlinebuf[whichbuf ? 0 : 1] + 30); + inlinebuf[whichbuf ? 0 : 1] + 30) < sizeof(buf)); for (p = buf; *p; ++p) { if (*p == '\n') { *p = '\0'; @@ -432,7 +432,7 @@ void include_file(Item* q) { } static void pop_file_stack() { - Sprintf(buf, ":::end INCLUDE %s\n", finname); + assert(Sprintf(buf, ":::end INCLUDE %s\n", finname) < sizeof(buf)); lappendstr(filetxtlist, buf); FileStackItem* fsi; fsi = (FileStackItem*) (SYM(filestack->prev)); diff --git a/src/nocmodl/kinetic.cpp b/src/nocmodl/kinetic.cpp index b4d92845be..eb4eb37385 100644 --- a/src/nocmodl/kinetic.cpp +++ b/src/nocmodl/kinetic.cpp @@ -987,7 +987,7 @@ int genconservterms(int eqnum, Reaction* r, int fn, Rlist* rlst) { Sprintf(eqstr, "%d(%d", fn, eqnum); eqnum++; } - Sprintf(buf, "_RHS%s) = %s;\n", eqstr, r->krate[0]); + assert(Sprintf(buf, "_RHS%s) = %s;\n", eqstr, r->krate[0]) < sizeof(buf)); Insertstr(q, buf); for (rt = r->rterm[0]; rt; rt = rt->rnext) { char buf1[NRN_BUFSIZE]; @@ -1004,19 +1004,21 @@ int genconservterms(int eqnum, Reaction* r, int fn, Rlist* rlst) { Sprintf(buf, "_i = %s;\n", rt->str); Insertstr(q, buf); } - Sprintf(buf, - "_MATELM%s, %d + %s) = %d%s;\n", - eqstr, - rt->sym->varnum, - rt->str, - rt->num, - buf1); + assert(Sprintf(buf, + "_MATELM%s, %d + %s) = %d%s;\n", + eqstr, + rt->sym->varnum, + rt->str, + rt->num, + buf1) < sizeof(buf)); Insertstr(q, buf); - Sprintf(buf, "_RHS%s) -= %s[%s]%s", eqstr, rt->sym->name, rt->str, buf1); + assert(Sprintf(buf, "_RHS%s) -= %s[%s]%s", eqstr, rt->sym->name, rt->str, buf1) < + sizeof(buf)); } else { - Sprintf(buf, "_MATELM%s, %d) = %d%s;\n", eqstr, rt->sym->varnum, rt->num, buf1); + assert(Sprintf(buf, "_MATELM%s, %d) = %d%s;\n", eqstr, rt->sym->varnum, rt->num, buf1) < + sizeof(buf)); Insertstr(q, buf); - Sprintf(buf, "_RHS%s) -= %s%s", eqstr, rt->sym->name, buf1); + assert(Sprintf(buf, "_RHS%s) -= %s%s", eqstr, rt->sym->name, buf1) < sizeof(buf)); } Insertstr(q, buf); if (rt->num != 1) { diff --git a/src/nocmodl/nocpout.cpp b/src/nocmodl/nocpout.cpp index 3a37745448..086daae00e 100644 --- a/src/nocmodl/nocpout.cpp +++ b/src/nocmodl/nocpout.cpp @@ -1775,7 +1775,7 @@ void units_reg() { if (s->nrntype & NRNGLOBAL) { decode_ustr(s, &d1, &d2, u); if (u[0]) { - Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, suffix, u); + assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, suffix, u) < sizeof(buf)); lappendstr(defs_list, buf); } } @@ -1784,7 +1784,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); + assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); lappendstr(defs_list, buf); } } @@ -1792,7 +1792,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); + assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); lappendstr(defs_list, buf); } } @@ -1800,7 +1800,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); + assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); lappendstr(defs_list, buf); } } @@ -1808,7 +1808,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); + assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); lappendstr(defs_list, buf); } } diff --git a/src/oc/mk_hocusr_h.py b/src/oc/mk_hocusr_h.py index 56f60ff688..b754487e23 100644 --- a/src/oc/mk_hocusr_h.py +++ b/src/oc/mk_hocusr_h.py @@ -25,7 +25,7 @@ def processvar(a, names): def remove_multiline_comments(string): # remove all occurance comments (/*COMMENT */) from string - return re.sub(r"/\*.*?\*/", "", string, 0, re.DOTALL) + return re.sub(r"/\*.*?\*/", "", string, count=0, flags=re.DOTALL) types = {} From c1cd333e4b1bbada5dc3b1a314a67502b552fbb2 Mon Sep 17 00:00:00 2001 From: Michael Hines Date: Thu, 13 Mar 2025 07:14:35 -0400 Subject: [PATCH 2/3] SprintfAsrt asserts that Sprintf format data fits into buf arg. --- src/modlunit/model.h | 1 + src/nocmodl/io.cpp | 14 +++++++------- src/nocmodl/kinetic.cpp | 24 +++++++++++------------- src/nocmodl/modl.h | 1 + src/nocmodl/nocpout.cpp | 10 +++++----- src/oc/hocdec.h | 1 + src/oc/wrap_sprintf.h | 11 +++++++++++ 7 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/modlunit/model.h b/src/modlunit/model.h index fcc644d6b0..02dd675da6 100644 --- a/src/modlunit/model.h +++ b/src/modlunit/model.h @@ -224,6 +224,7 @@ extern Item* qlint; #define IGNORE(arg) arg #endif using neuron::Sprintf; +using neuron::SprintfAsrt; /* model.h,v * Revision 1.2 1997/11/24 16:19:13 hines diff --git a/src/nocmodl/io.cpp b/src/nocmodl/io.cpp index a1f2dd7964..c84d3b20ae 100644 --- a/src/nocmodl/io.cpp +++ b/src/nocmodl/io.cpp @@ -216,15 +216,15 @@ void unGets(char* buf) /* all this because we don't have an ENDBLOCK char* current_line() { /* assumes we actually want the previous line */ static char buf[NRN_BUFSIZE]; char* p; - assert(Sprintf(buf, - "at line %d in file %s:\\n%s", - linenum - 1, + SprintfAsrt(buf, + "at line %d in file %s:\\n%s", + linenum - 1, #if !defined(NRN_AVOID_ABSOLUTE_PATHS) - finname, + finname, #else - fs::absolute(finname).filename().c_str(), + fs::absolute(finname).filename().c_str(), #endif - inlinebuf[whichbuf ? 0 : 1] + 30) < sizeof(buf)); + inlinebuf[whichbuf ? 0 : 1] + 30); for (p = buf; *p; ++p) { if (*p == '\n') { *p = '\0'; @@ -432,7 +432,7 @@ void include_file(Item* q) { } static void pop_file_stack() { - assert(Sprintf(buf, ":::end INCLUDE %s\n", finname) < sizeof(buf)); + SprintfAsrt(buf, ":::end INCLUDE %s\n", finname); lappendstr(filetxtlist, buf); FileStackItem* fsi; fsi = (FileStackItem*) (SYM(filestack->prev)); diff --git a/src/nocmodl/kinetic.cpp b/src/nocmodl/kinetic.cpp index eb4eb37385..6aeb9e1292 100644 --- a/src/nocmodl/kinetic.cpp +++ b/src/nocmodl/kinetic.cpp @@ -987,7 +987,7 @@ int genconservterms(int eqnum, Reaction* r, int fn, Rlist* rlst) { Sprintf(eqstr, "%d(%d", fn, eqnum); eqnum++; } - assert(Sprintf(buf, "_RHS%s) = %s;\n", eqstr, r->krate[0]) < sizeof(buf)); + SprintfAsrt(buf, "_RHS%s) = %s;\n", eqstr, r->krate[0]); Insertstr(q, buf); for (rt = r->rterm[0]; rt; rt = rt->rnext) { char buf1[NRN_BUFSIZE]; @@ -1004,21 +1004,19 @@ int genconservterms(int eqnum, Reaction* r, int fn, Rlist* rlst) { Sprintf(buf, "_i = %s;\n", rt->str); Insertstr(q, buf); } - assert(Sprintf(buf, - "_MATELM%s, %d + %s) = %d%s;\n", - eqstr, - rt->sym->varnum, - rt->str, - rt->num, - buf1) < sizeof(buf)); + SprintfAsrt(buf, + "_MATELM%s, %d + %s) = %d%s;\n", + eqstr, + rt->sym->varnum, + rt->str, + rt->num, + buf1); Insertstr(q, buf); - assert(Sprintf(buf, "_RHS%s) -= %s[%s]%s", eqstr, rt->sym->name, rt->str, buf1) < - sizeof(buf)); + SprintfAsrt(buf, "_RHS%s) -= %s[%s]%s", eqstr, rt->sym->name, rt->str, buf1); } else { - assert(Sprintf(buf, "_MATELM%s, %d) = %d%s;\n", eqstr, rt->sym->varnum, rt->num, buf1) < - sizeof(buf)); + SprintfAsrt(buf, "_MATELM%s, %d) = %d%s;\n", eqstr, rt->sym->varnum, rt->num, buf1); Insertstr(q, buf); - assert(Sprintf(buf, "_RHS%s) -= %s%s", eqstr, rt->sym->name, buf1) < sizeof(buf)); + SprintfAsrt(buf, "_RHS%s) -= %s%s", eqstr, rt->sym->name, buf1); } Insertstr(q, buf); if (rt->num != 1) { diff --git a/src/nocmodl/modl.h b/src/nocmodl/modl.h index 4f516b3504..929c1a0963 100644 --- a/src/nocmodl/modl.h +++ b/src/nocmodl/modl.h @@ -330,6 +330,7 @@ extern Item* qlint; #define Free(arg) free((void*) (arg)) #endif using neuron::Sprintf; +using neuron::SprintfAsrt; void verbatim_adjust(char* q); /** @} */ // end of hoc_functions diff --git a/src/nocmodl/nocpout.cpp b/src/nocmodl/nocpout.cpp index 086daae00e..946fcacf8f 100644 --- a/src/nocmodl/nocpout.cpp +++ b/src/nocmodl/nocpout.cpp @@ -1775,7 +1775,7 @@ void units_reg() { if (s->nrntype & NRNGLOBAL) { decode_ustr(s, &d1, &d2, u); if (u[0]) { - assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, suffix, u) < sizeof(buf)); + SprintfAsrt(buf, "{\"%s%s\", \"%s\"},\n", s->name, suffix, u); lappendstr(defs_list, buf); } } @@ -1784,7 +1784,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); + SprintfAsrt(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); lappendstr(defs_list, buf); } } @@ -1792,7 +1792,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); + SprintfAsrt(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); lappendstr(defs_list, buf); } } @@ -1800,7 +1800,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); + SprintfAsrt(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); lappendstr(defs_list, buf); } } @@ -1808,7 +1808,7 @@ void units_reg() { s = SYM(q); decode_ustr(s, &d1, &d2, u); if (u[0]) { - assert(Sprintf(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u) < sizeof(buf)); + SprintfAsrt(buf, "{\"%s%s\", \"%s\"},\n", s->name, rsuffix, u); lappendstr(defs_list, buf); } } diff --git a/src/oc/hocdec.h b/src/oc/hocdec.h index c0b31ae6af..2a4fef36b3 100644 --- a/src/oc/hocdec.h +++ b/src/oc/hocdec.h @@ -272,6 +272,7 @@ int ilint; #define Strncpy strncpy #endif using neuron::Sprintf; +using neuron::SprintfAsrt; // No longer used because of clang format difficulty // #define IFGUI if (hoc_usegui) { diff --git a/src/oc/wrap_sprintf.h b/src/oc/wrap_sprintf.h index bba7ddd80f..b42dc7cd65 100644 --- a/src/oc/wrap_sprintf.h +++ b/src/oc/wrap_sprintf.h @@ -1,5 +1,6 @@ #include #include // std::forward +#include namespace neuron { /** @@ -17,4 +18,14 @@ int Sprintf(char (&buf)[N], const char* fmt, Args&&... args) { return std::snprintf(buf, N, fmt, std::forward(args)...); } } + +/** + * @brief Assert that the Sprintf format data fits into buf + */ +template +void SprintfAsrt(char (&buf)[N], const char* fmt, Args&&... args) { + int sz = Sprintf(buf, fmt, std::forward(args)...); + assert(sz >= 0 && std::size_t(sz) < N); +} + } // namespace neuron From 8fe9420fb4f5d4be09e1e2ce5876fdc181ecb964 Mon Sep 17 00:00:00 2001 From: Michael Hines Date: Thu, 13 Mar 2025 08:36:36 -0400 Subject: [PATCH 3/3] unit test for wrap_sprintf.h --- src/oc/wrap_sprintf.h | 9 ++++++++- test/CMakeLists.txt | 1 + test/unit_tests/utils/Sprintf.cpp | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 test/unit_tests/utils/Sprintf.cpp diff --git a/src/oc/wrap_sprintf.h b/src/oc/wrap_sprintf.h index b42dc7cd65..8cbcf36f6c 100644 --- a/src/oc/wrap_sprintf.h +++ b/src/oc/wrap_sprintf.h @@ -1,5 +1,6 @@ #include #include // std::forward +#include #include namespace neuron { @@ -20,12 +21,18 @@ int Sprintf(char (&buf)[N], const char* fmt, Args&&... args) { } /** - * @brief Assert that the Sprintf format data fits into buf + * @brief assert if the Sprintf format data does not fit into buf */ template void SprintfAsrt(char (&buf)[N], const char* fmt, Args&&... args) { int sz = Sprintf(buf, fmt, std::forward(args)...); +#ifdef UNIT_TESTING + if (sz < 0 || std::size_t(sz) >= N) { + throw std::runtime_error("SprintfAsrt buffer too small or snprintf error"); + } +#else assert(sz >= 0 && std::size_t(sz) < N); +#endif } } // namespace neuron diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 2543586cab..641ba0cd63 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -24,6 +24,7 @@ add_executable( unit_tests/container/node.cpp unit_tests/node_order_optim/permutations.cpp unit_tests/utils/enumerate.cpp + unit_tests/utils/Sprintf.cpp unit_tests/oc/hoc_interpreter.cpp cover/unit_tests/cover.cpp) set(catch2_targets testneuron) diff --git a/test/unit_tests/utils/Sprintf.cpp b/test/unit_tests/utils/Sprintf.cpp new file mode 100644 index 0000000000..63c6dff186 --- /dev/null +++ b/test/unit_tests/utils/Sprintf.cpp @@ -0,0 +1,19 @@ +#include + +#define UNIT_TESTING +#include "oc/wrap_sprintf.h" + +using neuron::SprintfAsrt; + +TEST_CASE("buf fits", "[NEURON]") { + char buf[20]; + SprintfAsrt(buf, "%s", "hello"); + REQUIRE(strcmp(buf, "hello") == 0); +} + +TEST_CASE("buf too small", "[NEURON]") { + char buf[5]; + char* s = strdup("hello"); + REQUIRE_THROWS(SprintfAsrt(buf, "%s", s)); + free(s); +}