From 343e367fb8c6550392bf46bcbe2390782292cd2e Mon Sep 17 00:00:00 2001 From: Nathan Glenn Date: Wed, 23 Aug 2023 17:17:27 -0500 Subject: [PATCH 1/2] harden smem export Harden the logic for exporting smem to prevent segfaults. Previously we had a segfault with just `smem -x file.soar` because the smem DB is not connected. Adding an LTI option, however, would connect to the DB first. Change it so that export (as with backup) requires connecting to the DB first, and gracefully returns false if not connected. --- Core/CLI/src/cli_load_save.cpp | 3 ++ Core/CLI/src/cli_smem.cpp | 48 +++---------------- .../src/semantic_memory/smem_print.cpp | 4 ++ 3 files changed, 14 insertions(+), 41 deletions(-) diff --git a/Core/CLI/src/cli_load_save.cpp b/Core/CLI/src/cli_load_save.cpp index e15bd3dec4..8da081ee5f 100644 --- a/Core/CLI/src/cli_load_save.cpp +++ b/Core/CLI/src/cli_load_save.cpp @@ -212,6 +212,9 @@ bool CommandLineInterface::DoSave(std::vector& argv, const std::str if (thisAgent->SMem->connected() && (thisAgent->SMem->statistics->nodes->get_value() > 0)) { result = thisAgent->SMem->export_smem(0, export_text, &(err)); + if (!result) { + SetError(*err); + } AddSaveText("# Semantic Memory\n"); if (!DoCLog(LOG_ADD, 0, &export_text, true)) return false; } else { diff --git a/Core/CLI/src/cli_smem.cpp b/Core/CLI/src/cli_smem.cpp index e7124e4529..c1b3ace747 100644 --- a/Core/CLI/src/cli_smem.cpp +++ b/Core/CLI/src/cli_smem.cpp @@ -403,14 +403,13 @@ bool CommandLineInterface::DoSMem(const char pOp, const std::string* pArg1, cons std::string* err = new std::string(""); uint64_t lti_id = NIL; - if (pArg2) + if (!thisAgent->SMem->connected()) { - thisAgent->SMem->attach(); - if (!thisAgent->SMem->connected()) - { - return SetError("Semantic memory database not connected."); - } + return SetError("Cannot export smem: smem database not connected."); + } + if (pArg2) + { const char* pAttr_c_str = pArg2->c_str(); soar::Lexer lexer(thisAgent, pAttr_c_str); if (!lexer.get_lexeme()) return SetError("LTI not found."); @@ -456,41 +455,8 @@ bool CommandLineInterface::DoSMem(const char pOp, const std::string* pArg1, cons return false; } - PrintCLIMessage("Exported semantic memory to file."); - } - delete err; - return result; - } - else if (pOp == 'x') - { - std::string* err = new std::string("smem_export.soar"); - uint64_t lti_id = NIL; - - std::string export_text; - bool result = thisAgent->SMem->export_smem(0, export_text, &(err)); - - if (!result) - { - SetError(*err); - } - else - { - if (!DoCLog(LOG_NEW, err, 0, true)) - { - return false; - } - - if (!DoCLog(LOG_ADD, 0, &export_text, true)) - { - return false; - } - - if (!DoCLog(LOG_CLOSE, 0, 0, true)) - { - return false; - } - - PrintCLIMessage("Exported semantic memory to file."); + tempString << "Exported semantic memory to file '" << pArg1->c_str() << "'."; + PrintCLIMessage(&tempString); } delete err; return result; diff --git a/Core/SoarKernel/src/semantic_memory/smem_print.cpp b/Core/SoarKernel/src/semantic_memory/smem_print.cpp index 57ef043b9b..38d32f0776 100644 --- a/Core/SoarKernel/src/semantic_memory/smem_print.cpp +++ b/Core/SoarKernel/src/semantic_memory/smem_print.cpp @@ -16,6 +16,10 @@ bool SMem_Manager::export_smem(uint64_t lti_id, std::string& result_text, std::string** err_msg) { ltm_set store_set; + if (!connected()) { + (*err_msg)->append("Cannot export semantic memory if it is not connected."); + return false; + } if (!lti_id) { From 2e322497fd5824e1f55cbe407c4feb484d821a5e Mon Sep 17 00:00:00 2001 From: Nathan Glenn Date: Wed, 23 Aug 2023 17:19:51 -0500 Subject: [PATCH 2/2] connect to SMem DB in init CLI command Previously a simple smem --init smem --clear at the beginning of a source file would fail and prevent the rest of the file from sourcing. Connect to the DB in the init CLI command so that any following actions can access the DB. I believe this is the intuitive behavior, and I don't see anything in the manual that would indicate that this incorrect. --- .gitignore | 2 +- Core/CLI/src/cli_smem.cpp | 8 +++++--- UnitTests/SoarTestAgents/testRegression370.soar | 10 ++++++++++ UnitTests/SoarUnitTests/MiscTests.cpp | 7 +++++++ UnitTests/SoarUnitTests/MiscTests.hpp | 2 ++ 5 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 UnitTests/SoarTestAgents/testRegression370.soar diff --git a/.gitignore b/.gitignore index 78f7d1e01c..9b07309489 100644 --- a/.gitignore +++ b/.gitignore @@ -252,7 +252,7 @@ compile_commands.json # NUNIT *.VisualState.xml -TestResult.xml +TestResults.xml # Build Results of an ATL Project [Dd]ebugPS/ diff --git a/Core/CLI/src/cli_smem.cpp b/Core/CLI/src/cli_smem.cpp index c1b3ace747..c0e1343b74 100644 --- a/Core/CLI/src/cli_smem.cpp +++ b/Core/CLI/src/cli_smem.cpp @@ -133,9 +133,11 @@ bool CommandLineInterface::DoSMem(const char pOp, const std::string* pArg1, cons } else if (pOp == 'i') { - /* Don't think we need clear out anything else any more */ - - thisAgent->SMem->reinit(); + if (thisAgent->SMem->connected()) { + thisAgent->SMem->reinit(); + } else { + thisAgent->SMem->attach(); + } PrintCLIMessage("Semantic memory system re-initialized."); if (thisAgent->SMem->settings->append_db->get_value() == on) diff --git a/UnitTests/SoarTestAgents/testRegression370.soar b/UnitTests/SoarTestAgents/testRegression370.soar new file mode 100644 index 0000000000..705f7115e9 --- /dev/null +++ b/UnitTests/SoarTestAgents/testRegression370.soar @@ -0,0 +1,10 @@ +# https://github.com/SoarGroup/Soar/issues/370 +# Make sure that smem --init connects smem DB so that --clear doesn't raise an error. +smem --init +smem --clear + +sp {pass + (state ) +--> + (succeeded) +} diff --git a/UnitTests/SoarUnitTests/MiscTests.cpp b/UnitTests/SoarUnitTests/MiscTests.cpp index 18e4506c10..1a2ea188d6 100644 --- a/UnitTests/SoarUnitTests/MiscTests.cpp +++ b/UnitTests/SoarUnitTests/MiscTests.cpp @@ -285,6 +285,13 @@ void MiscTests::testWrongAgentWmeFunctions() } +void MiscTests::testRegression370() +{ + source("testRegression370.soar"); + agent->RunSelf(5000); + SoarHelper::init_check_to_find_refcount_leaks(agent); +} + void MiscTests::testRHSRand() { kernel->AddRhsFunction("failed", Handlers::MyRhsFunctionFailureHandler, 0) ; diff --git a/UnitTests/SoarUnitTests/MiscTests.hpp b/UnitTests/SoarUnitTests/MiscTests.hpp index a1b397adf4..12770cc4fe 100644 --- a/UnitTests/SoarUnitTests/MiscTests.hpp +++ b/UnitTests/SoarUnitTests/MiscTests.hpp @@ -33,6 +33,8 @@ class MiscTests : public FunctionalTestHarness TEST(testWrongAgentWmeFunctions, -1) void testWrongAgentWmeFunctions(); + TEST(testRegression370, -1) + void testRegression370(); TEST(testRHSRand, -1) void testRHSRand(); TEST(testMultipleKernels, -1)