From 674f48d0c9d83301055f2b6128d1d9cdf22a9a1d Mon Sep 17 00:00:00 2001 From: Harmen Wierenga Date: Fri, 17 Apr 2026 11:35:40 +0200 Subject: [PATCH 1/2] UNST-9861 Solve memory leaks and logic bugs in xmltree and dimr, use std::string for memory management --- .../dimr_lib/include/dimr_component.h | 18 +- .../packages/dimr_lib/include/dimr_coupler.h | 4 +- .../dimr/packages/dimr_lib/include/xmltree.h | 25 +-- .../dimr/packages/dimr_lib/src/dimr.cpp | 77 ++++---- .../dimr/packages/dimr_lib/src/xmltree.cpp | 173 +++++++----------- 5 files changed, 126 insertions(+), 171 deletions(-) diff --git a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_component.h b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_component.h index 29805427257..09ee32a45b0 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_component.h +++ b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_component.h @@ -39,20 +39,20 @@ typedef void(CDECLOPT* BMI_GETVARSHAPE)(const char*, int*); typedef struct dimr_component dimr_component; struct dimr_component { - const char* name; // Component name: must be unique in the config.xml file (e.g. myNameFlow) - char* library; // Component library name, without extension/prefix - int type; // COMP_TYPE_FM, COMP_TYPE_RTC or COMP_TYPE_WAVE + const char* name; // Component name: must be unique in the config.xml file (e.g. myNameFlow) + const char* library; // Component library name, without extension/prefix + int type; // COMP_TYPE_FM, COMP_TYPE_RTC or COMP_TYPE_WAVE #ifdef _WIN32 HINSTANCE libHandle; // (Windows) Handle to the loaded library for this component. #else void* libHandle; // (Linux) Handle to the loaded library for this component. #endif - char* inputFile; // Component inputFile name - char* workingDir; // Component working directory - int* processes; // (Optional) list of processes ranks that this component needs to run in. - int numProcesses; // Count of processes array. - bool onThisRank; // Whether this component needs to run on current process rank. - char* mpiCommVar; // (Optional) Variable name for component's MPI communicator (must be accesible via BMI). + const char* inputFile; // Component inputFile name + char* workingDir; // Component working directory (owned, heap-allocated) + int* processes; // (Optional) list of processes ranks that this component needs to run in. + int numProcesses; // Count of processes array. + bool onThisRank; // Whether this component needs to run on current process rank. + const char* mpiCommVar; // (Optional) Variable name for component's MPI communicator (must be accesible via BMI). MPI_Comm mpiComm; // An MPI communicator specific for this component (may run on less processes than master dimr). BMI_INITIALIZE dllInitialize; // entry point in dll BMI_UPDATE dllUpdate; // entry point in dll diff --git a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_coupler.h b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_coupler.h index 8d4dbeae98e..92185c25444 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_coupler.h +++ b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr_coupler.h @@ -7,8 +7,8 @@ typedef struct dimr_coupler dimr_coupler; struct dimr_coupler { const char* name; // Coupler name: must be unique in the config.xml file (e.g. rtc2flow) - char* sourceComponentName; // Name of the component providing data to be communicated by the coupler - char* targetComponentName; // Name of the component receiving data to be communicated by the coupler + const char* sourceComponentName; // Name of the component providing data to be communicated by the coupler + const char* targetComponentName; // Name of the component receiving data to be communicated by the coupler dimr_component* sourceComponent; // Pointer to the related component dimr_component* targetComponent; // idem unsigned int numItems; diff --git a/src/engines_gpl/dimr/packages/dimr_lib/include/xmltree.h b/src/engines_gpl/dimr/packages/dimr_lib/include/xmltree.h index 4b4e3e4f140..7639cd69fbf 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/include/xmltree.h +++ b/src/engines_gpl/dimr/packages/dimr_lib/include/xmltree.h @@ -98,6 +98,11 @@ class XmlTree void Print(void); + std::string charData; + XmlTree* parent; + std::vector children; + std::string name; + private: void init(void); @@ -107,21 +112,7 @@ class XmlTree static std::string EnvSubst(std::string instr); -public: - static const int maxCharData = 1000000; // maximum size of an XML character data block - static const int maxPathname = 2560; // maximum length of a full path name - - XmlTree* parent; - char* name; - char* pathname; - - std::vector attribNames; - std::vector attribValues; - - std::vector children; - - char* charData; - int charDataLen; - -private: + std::string pathname; + std::vector attribNames; + std::vector attribValues; }; diff --git a/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp b/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp index 0c915e8ff2a..d241bb4cdfa 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp +++ b/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp @@ -198,6 +198,10 @@ Dimr::~Dimr(void) delete log; delete[] mainArgs; // componentsList + for (int i = 0; i < componentsList.numComponents; i++) + { + delete[] componentsList.components[i].workingDir; + } free(componentsList.components); // couplersList for (int i = 0; i < couplersList.numCouplers; i++) @@ -1590,7 +1594,7 @@ void Dimr::scanConfigFile(void) configfile); // Check version number - const char* versionnr = fileversion->charData; + const char* versionnr = fileversion->charData.c_str(); float versionnumber; int intRead = sscanf(versionnr, "%f", &versionnumber); if (intRead != 1) @@ -1630,7 +1634,7 @@ void Dimr::scanGlobalSettings(XmlTree* rootXml) XmlTree* loggerNcFormat = globalSettings->Lookup("logger_ncFormat"); if (loggerNcFormat != NULL) { - int intRead = sscanf(loggerNcFormat->charData, "%d", &(nc_mode)); + int intRead = sscanf(loggerNcFormat->charData.c_str(), "%d", &(nc_mode)); if (intRead != 1) throw Exception(Exception::ERR_INVALID_INPUT, "logger_ncFormat must contain the value 3 or 4"); if (nc_mode == 3) @@ -1659,7 +1663,7 @@ void Dimr::scanUnits(XmlTree* rootXml) // Scan for (int i = 0; i < rootXml->children.size(); i++) { - if (strcmp(rootXml->children[i]->name, "component") == 0) + if (rootXml->children[i]->name == "component") { componentsList.numComponents++; if (componentsList.components == NULL) @@ -1678,7 +1682,7 @@ void Dimr::scanUnits(XmlTree* rootXml) } scanComponent(rootXml->children[i], &(componentsList.components[componentsList.numComponents - 1])); } - if (strcmp(rootXml->children[i]->name, "coupler") == 0) + if (rootXml->children[i]->name == "coupler") { couplersList.numCouplers++; if (couplersList.couplers == NULL) @@ -1714,7 +1718,7 @@ void Dimr::scanComponent(XmlTree* xmlComponent, dimr_component* newComp) if (libraryElement == NULL) throw Exception(Exception::ERR_INVALID_INPUT, "Component \"%s\" does not contain a library element", newComp->name); - newComp->library = libraryElement->charData; + newComp->library = libraryElement->charData.c_str(); int libLen = strlen(newComp->library); char* libNameLowercase = new char[libLen + 1]; strncpy(libNameLowercase, newComp->library, libLen); @@ -1784,7 +1788,7 @@ void Dimr::scanComponent(XmlTree* xmlComponent, dimr_component* newComp) if (processElement != NULL) { // Store process rank numbers in component's processes array. - char_to_ints(processElement->charData, &(newComp->processes), &(newComp->numProcesses)); + char_to_ints(processElement->charData.c_str(), &(newComp->processes), &(newComp->numProcesses)); // Check whether this process' rank is also in components configured processes array. newComp->onThisRank = false; // Not found (yet): only active on other ranks. @@ -1826,7 +1830,7 @@ void Dimr::scanComponent(XmlTree* xmlComponent, dimr_component* newComp) if (commElement != NULL) { // Store communicator var name in component. - newComp->mpiCommVar = commElement->charData; + newComp->mpiCommVar = commElement->charData.c_str(); } else { @@ -1841,37 +1845,39 @@ void Dimr::scanComponent(XmlTree* xmlComponent, dimr_component* newComp) } else { - newComp->inputFile = inputFileElement->charData; + newComp->inputFile = inputFileElement->charData.c_str(); } // Element workingDir XmlTree* workingDirElement = xmlComponent->Lookup("workingDir"); + const char* rawWorkingDir; if (workingDirElement == NULL) { - newComp->workingDir = (char*)&curPath; + rawWorkingDir = curPath; log->Write(INFO, my_rank, "WARNING: No workingDir specified for component %s.", newComp->name); - log->Write(INFO, my_rank, " workingDir is set to %s", newComp->workingDir); + log->Write(INFO, my_rank, " workingDir is set to %s", rawWorkingDir); } else { - newComp->workingDir = workingDirElement->charData; + rawWorkingDir = workingDirElement->charData.c_str(); } // Is workingDir a valid relative path? char* combinedPath = new char[MAXSTRING]; - sprintf(combinedPath, "%s%s%s", curPath, dirSeparator, newComp->workingDir); + sprintf(combinedPath, "%s%s%s", curPath, dirSeparator, rawWorkingDir); if (chdir(combinedPath)) { // CombinedPath is not correct. May be just workingDir? delete[] combinedPath; // Is workingDir a valid absolute path? - if (chdir(newComp->workingDir)) + if (chdir(rawWorkingDir)) { throw Exception(Exception::ERR_INVALID_INPUT, "Component \"%s\" has an invalid workingDir \"%s\"", - newComp->name, newComp->workingDir); + newComp->name, rawWorkingDir); } + newComp->workingDir = new char[strlen(rawWorkingDir) + 1]; + strcpy(newComp->workingDir, rawWorkingDir); } else { - // newComp->workingDir was a pointer to workingDirElement->charData; now it will point to the new combinedPath newComp->workingDir = combinedPath; } chdir(curPath); @@ -1895,7 +1901,7 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) if (sourceComponent == NULL) throw Exception(Exception::ERR_INVALID_INPUT, "The coupler \"%s\" does not contain a sourceComponent element", newCoup->name); - newCoup->sourceComponentName = sourceComponent->charData; + newCoup->sourceComponentName = sourceComponent->charData.c_str(); // Add reference to the actual component acting as source newCoup->sourceComponent = getComponent(newCoup->sourceComponentName); // Element targetComponent @@ -1903,7 +1909,7 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) if (targetComponent == NULL) throw Exception(Exception::ERR_INVALID_INPUT, "The coupler \"%s\" does not contain a targetComponent element", newCoup->name); - newCoup->targetComponentName = targetComponent->charData; + newCoup->targetComponentName = targetComponent->charData.c_str(); // Add reference to the actual component acting as target newCoup->targetComponent = getComponent(newCoup->targetComponentName); // Items @@ -1911,7 +1917,7 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) newCoup->items = NULL; for (int j = 0; j < xmlCoupler->children.size(); j++) { - if (strcmp(xmlCoupler->children[j]->name, "item") == 0) + if (xmlCoupler->children[j]->name == "item") { // Create the item newCoup->numItems++; @@ -1949,8 +1955,8 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) throw Exception(Exception::ERR_INVALID_INPUT, "The coupler \"%s\", item %d, does not contain a sourceName element", newCoup->name, newCoup->numItems); - newItem->sourceName = xmlSource->charData; - if (newItem->sourceName == NULL) + newItem->sourceName = xmlSource->charData.c_str(); + if (xmlSource->charData.empty()) throw Exception(Exception::ERR_INVALID_INPUT, "Item %d of coupler \"%s\" does not contain a source::name element", newCoup->numItems, newCoup->name); @@ -1961,8 +1967,8 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) throw Exception(Exception::ERR_INVALID_INPUT, "The coupler \"%s\", item %d, does not contain a targetName element", newCoup->name, newCoup->numItems); - newItem->targetName = xmlTarget->charData; - if (newItem->targetName == NULL) + newItem->targetName = xmlTarget->charData.c_str(); + if (xmlTarget->charData.empty()) throw Exception(Exception::ERR_INVALID_INPUT, "Item %d of coupler \"%s\" does not contain a target::name element", newCoup->numItems, newCoup->name); @@ -2001,45 +2007,46 @@ void Dimr::scanCoupler(XmlTree* xmlCoupler, dimr_coupler* newCoup) //------------------------------------------------------------------------------ void Dimr::scanControl(XmlTree* controlBlockXml, dimr_control_block* controlBlock) { - if (strcmp(controlBlockXml->name, "control") == 0) + if (controlBlockXml->name == "control") { controlBlock->type = CT_SEQUENTIAL; } - else if (strcmp(controlBlockXml->name, "parallel") == 0) + else if (controlBlockXml->name == "parallel") { controlBlock->type = CT_PARALLEL; } - else if (strcmp(controlBlockXml->name, "start") == 0) + else if (controlBlockXml->name == "start") { controlBlock->type = CT_START; controlBlock->unit.component = getComponent(controlBlockXml->GetAttrib("name")); controlBlock->unit.coupler = NULL; } - else if (strcmp(controlBlockXml->name, "startGroup") == 0) + else if (controlBlockXml->name == "startGroup") { controlBlock->type = CT_STARTGROUP; XmlTree* timeElt = controlBlockXml->Lookup("time"); if (timeElt == NULL) throw Exception(Exception::ERR_INVALID_INPUT, - "The startGroup component \"%s\" does not contain a time element", controlBlockXml->name); + "The startGroup component \"%s\" does not contain a time element", + controlBlockXml->name.c_str()); // The time field either contains: // - tStart, tStep, tStop , e.g. // - name of a file containing computation time points, e.g. // First check whether it's the name of a file: - if (!readComputeTimesFile(timeElt->charData, controlBlock)) + if (!readComputeTimesFile(timeElt->charData.c_str(), controlBlock)) { // No, it's not the name of a file. Assume that it contains tStart, tStep, tStop - int intRead = sscanf(timeElt->charData, "%lf %lf %lf", &(controlBlock->tStart), &(controlBlock->tStep), - &(controlBlock->tEnd)); + int intRead = sscanf(timeElt->charData.c_str(), "%lf %lf %lf", &(controlBlock->tStart), + &(controlBlock->tStep), &(controlBlock->tEnd)); if (intRead != 3) throw Exception(Exception::ERR_INVALID_INPUT, "'%s' must either contain 'tStart, tStep, tEnd' or the name of a time series file", - timeElt->charData); + timeElt->charData.c_str()); // computeTimesCurrent>0 indicates a time series read from file controlBlock->computeTimesCurrent = -1; } } - else if (strcmp(controlBlockXml->name, "coupler") == 0) + else if (controlBlockXml->name == "coupler") { controlBlock->type = CT_COUPLER; controlBlock->unit.component = NULL; @@ -2050,10 +2057,8 @@ void Dimr::scanControl(XmlTree* controlBlockXml, dimr_control_block* controlBloc controlBlock->masterSubBlockId = -1; for (int i = 0; i < controlBlockXml->children.size(); i++) { - if (strcmp(controlBlockXml->children[i]->name, "parallel") == 0 || - strcmp(controlBlockXml->children[i]->name, "start") == 0 || - strcmp(controlBlockXml->children[i]->name, "startGroup") == 0 || - strcmp(controlBlockXml->children[i]->name, "coupler") == 0) + if (controlBlockXml->children[i]->name == "parallel" || controlBlockXml->children[i]->name == "start" || + controlBlockXml->children[i]->name == "startGroup" || controlBlockXml->children[i]->name == "coupler") { controlBlock->numSubBlocks++; if (controlBlock->subBlocks == NULL) diff --git a/src/engines_gpl/dimr/packages/dimr_lib/src/xmltree.cpp b/src/engines_gpl/dimr/packages/dimr_lib/src/xmltree.cpp index a1e0aec866e..31923409977 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/src/xmltree.cpp +++ b/src/engines_gpl/dimr/packages/dimr_lib/src/xmltree.cpp @@ -84,22 +84,26 @@ #define strdup _strdup #endif +struct ParseState +{ + XmlTree** curnode; + std::string charDataStr; +}; + static void starttag(void*, const XML_Char*, const XML_Char**); static void endtag(void*, const XML_Char*); static void chardata(void*, const XML_Char*, int); -static char CharDataBuffer[XmlTree::maxCharData]; -static int CharDataLen = 0; - //------------------------------------------------------------------------------ XmlTree::XmlTree(FILE* input) { this->init(); XmlTree* currentNode = this; + ParseState state{¤tNode, {}}; XML_Parser parser = XML_ParserCreate(NULL); - XML_SetUserData(parser, (void*)¤tNode); + XML_SetUserData(parser, (void*)&state); XML_SetElementHandler(parser, &starttag, &endtag); XML_SetCharacterDataHandler(parser, &chardata); @@ -117,7 +121,7 @@ XmlTree::XmlTree(FILE* input) static void starttag(void* userdata, const XML_Char* name, const XML_Char* attr[]) { - XmlTree** curnode = (XmlTree**)userdata; + XmlTree** curnode = static_cast(userdata)->curnode; XmlTree* node = new XmlTree(*curnode, name); (*curnode)->AddChild(node); *curnode = node; @@ -125,27 +129,25 @@ static void starttag(void* userdata, const XML_Char* name, const XML_Char* attr[ for (int i = 0; attr[i] != NULL && attr[i + 1] != NULL; i += 2) node->AddAttrib(attr[i], attr[i + 1]); } -#define IS_WHITESPACE(X) ((X) == ' ' || (X) == '\t' || (X) == '\n' || (X) == '\r') - static void endtag(void* userdata, const XML_Char* name) { - XmlTree** curnode = (XmlTree**)userdata; + ParseState* state = static_cast(userdata); + XmlTree** curnode = state->curnode; - if (CharDataLen > 0) + if (!state->charDataStr.empty()) { - char* begin = CharDataBuffer; - while (IS_WHITESPACE(*begin)) begin++; - - char* end = CharDataBuffer + CharDataLen - 1; - while (IS_WHITESPACE(*end)) end--; - *++end = '\0'; - - int len = strlen(begin); - (*curnode)->charData = new char[len + 1]; - memcpy((*curnode)->charData, begin, len); - (*curnode)->charData[len] = '\0'; - (*curnode)->charDataLen = len + 1; - CharDataLen = 0; + // Trim leading whitespace + const size_t first = state->charDataStr.find_first_not_of(" \t\n\r"); + if (first == std::string::npos) + { + state->charDataStr.clear(); + } + else + { + const size_t last = state->charDataStr.find_last_not_of(" \t\n\r"); + (*curnode)->charData = state->charDataStr.substr(first, last - first + 1); + state->charDataStr.clear(); + } } *curnode = (*curnode)->parent; @@ -154,17 +156,10 @@ static void endtag(void* userdata, const XML_Char* name) static void chardata(void* userdata, const XML_Char* data, int len) { // Chardata is stuff between tags, including "comments". - // Add it to the end of a static buffer. When the end tag is reached + // Add it to the end of the buffer. When the end tag is reached // the data will be added to the node. - XmlTree** curnode = (XmlTree**)userdata; - - if (len + CharDataLen >= sizeof CharDataBuffer) - throw Exception(Exception::ERR_XML_PARSING, "XML charcter data block exceeds buffer size (%d bytes)", - sizeof CharDataBuffer); - - memcpy(CharDataBuffer + CharDataLen, data, len); - CharDataLen += len; + static_cast(userdata)->charDataStr.append(data, len); } //------------------------------------------------------------------------------ @@ -173,54 +168,30 @@ XmlTree::XmlTree(XmlTree* parent, const char* name) { this->init(); - const char* ppn; - if (parent == NULL) - ppn = ""; - else - ppn = parent->pathname; - - int pathlen = strlen(ppn) + strlen(name) + 2; - if (pathlen > this->maxPathname) - throw Exception(Exception::ERR_XML_PARSING, " XML pathname for node \"%s\" is too long", name); - - this->name = new char[strlen(name) + 1]; - strcpy(this->name, name); + const std::string& ppn = (parent == NULL) ? std::string() : parent->pathname; - this->pathname = new char[pathlen]; - sprintf(this->pathname, "%s/%s", ppn, name); + this->name = name; + this->pathname = ppn + "/" + name; this->parent = parent; } -void XmlTree::init(void) -{ - this->name = (char*)""; - this->pathname = (char*)""; - this->parent = NULL; - this->charData = NULL; - this->charDataLen = 0; -} +void XmlTree::init(void) { this->parent = NULL; } XmlTree::~XmlTree(void) { - if (this->parent != NULL && this->name != NULL) + for (XmlTree* child : children) { - delete[] this->name; - delete[] this->pathname; + delete child; } - - if (this->charData != NULL) delete[] this->charData; } //------------------------------------------------------------------------------ void XmlTree::AddAttrib(const char* name, const char* value) { - this->attribNames.push_back(new char[strlen(name) + 1]); - this->attribValues.push_back(new char[strlen(value) + 1]); - - strcpy(this->attribNames.back(), name); - strcpy(this->attribValues.back(), value); + this->attribNames.push_back(std::string(name)); + this->attribValues.push_back(std::string(value)); } void XmlTree::AddChild(XmlTree* child) { this->children.push_back(child); } @@ -235,7 +206,7 @@ XmlTree* XmlTree::Lookup(const char* pathname, int instance) if (pathname[0] == '/') { - if (this->name[0] != '\0') return NULL; + if (!this->name.empty()) return NULL; pathname++; // skip leading slash } @@ -243,31 +214,31 @@ XmlTree* XmlTree::Lookup(const char* pathname, int instance) // Copy pathname and split first component and the remainder // (think of a backwards dirname/basename) - char* path = new char[strlen(pathname) + 1]; - strcpy(path, pathname); - char* remainder = strchr(path, '/'); - if (remainder == NULL) - remainder = (char*)""; - else - *remainder++ = '\0'; + std::string path = pathname; + std::string remainder; + auto slash = path.find('/'); + if (slash != std::string::npos) + { + remainder = path.substr(slash + 1); + path.resize(slash); + } XmlTree* node = NULL; for (int i = 0; i < children.size(); i++) { - if (strcmp(path, this->children[i]->name) == 0) + if (this->children[i]->name == path) { - if (remainder[0] == '\0') + if (remainder.empty()) { if (instance-- > 0) continue; node = this->children[i]; } else - node = this->children[i]->Lookup(remainder, instance); + node = this->children[i]->Lookup(remainder.c_str(), instance); break; } } - delete[] path; return node; } @@ -282,7 +253,7 @@ int XmlTree::Lookup(const char* pathname, int instance, { if (pathname[0] == '/') { - if (this->name[0] != '\0') return (NULL); + if (!this->name.empty()) return (NULL); pathname++; // skip leading slash } @@ -290,22 +261,23 @@ int XmlTree::Lookup(const char* pathname, int instance, // Copy pathname and split first component and the remainder // (think of a backwards dirname/basename) - char* path = new char[strlen(pathname) + 1]; - strcpy(path, pathname); - char* remainder = strchr(path, '/'); - if (remainder == NULL) - remainder = (char*)""; - else - *remainder++ = '\0'; + std::string path = pathname; + std::string remainder; + auto slash = path.find('/'); + if (slash != std::string::npos) + { + remainder = path.substr(slash + 1); + path.resize(slash); + } XmlTree* node = NULL; int ncount = 0; kvlist = NULL; for (int i = 0; i < children.size(); i++) { - if (strcmp(path, children[i]->name) == 0) + if (children[i]->name == path) { - if (remainder[0] == '\0') + if (remainder.empty()) { if (instance-- > 0) continue; node = this->children[i]; // found a node @@ -324,11 +296,10 @@ int XmlTree::Lookup(const char* pathname, int instance, ncount++; } else - this->children[i]->Lookup(remainder, instance); // found a path to descend + this->children[i]->Lookup(remainder.c_str(), instance); // found a path to descend } } - delete[] path; return (ncount); } @@ -350,7 +321,7 @@ const char* XmlTree::GetAttrib(const char* name) } for (int i = 0; i < attribNames.size(); i++) - if (strcmp(name, this->attribNames[i]) == 0) return this->attribValues[i]; + if (this->attribNames[i] == name) return this->attribValues[i].c_str(); return NULL; } @@ -393,7 +364,7 @@ const char* XmlTree::GetElement(const char* name) if (node == NULL) return NULL; else - return node->charData; + return node->charData.empty() ? nullptr : node->charData.c_str(); } bool XmlTree::GetBoolElement(const char* name, bool defaultValue) @@ -426,9 +397,10 @@ void XmlTree::print(int level) if (this->parent == NULL) printf("/ [ "); else - printf("%s [ ", this->pathname); + printf("%s [ ", this->pathname.c_str()); - for (int i = 0; i < attribNames.size(); i++) printf("%s=%s ", this->attribNames[i], this->attribValues[i]); + for (int i = 0; i < attribNames.size(); i++) + printf("%s=%s ", this->attribNames[i].c_str(), this->attribValues[i].c_str()); printf("]\n"); @@ -472,26 +444,13 @@ void XmlTree::ExpandEnvironmentVariables() { return this->ExpandEnvironmentVaria void XmlTree::ExpandEnvironmentVariables(int instance) { - XmlTree* node = NULL; - char* orgstr; - std::string instr, outstr; for (int iattrib = 0; iattrib < attribValues.size(); iattrib++) { - orgstr = this->attribValues[iattrib]; - instr = orgstr; - outstr = SubstEnvVar(instr) + '\0'; - delete[] this->attribValues[iattrib]; - this->attribValues[iattrib] = new char[outstr.length()]; - outstr.copy(this->attribValues[iattrib], outstr.length()); + this->attribValues[iattrib] = SubstEnvVar(this->attribValues[iattrib]); } - if (this->charData != NULL) + if (!this->charData.empty()) { - orgstr = this->charData; - instr = orgstr; - outstr = SubstEnvVar(instr) + '\0'; - delete[] this->charData; - this->charData = new char[outstr.length()]; - outstr.copy(this->charData, outstr.length()); + this->charData = SubstEnvVar(this->charData); } for (int i = 0; i < children.size(); i++) From cfd93f86441c2da55df56d1701df859901bade38 Mon Sep 17 00:00:00 2001 From: Harmen Wierenga Date: Fri, 17 Apr 2026 14:39:38 +0200 Subject: [PATCH 2/2] UNST-9861 Properly deallocate Dimr instance at the end. Ensure that logger is only deleted when owned by Dimr instance --- .../dflowfm_kernel/prepost/cutcell_list.f90 | 2 +- .../dimr/packages/dimr_lib/include/dimr.h | 10 ++---- .../dimr/packages/dimr_lib/src/bmi.cpp | 5 +++ .../dimr/packages/dimr_lib/src/dimr.cpp | 33 ++++++++++++++++--- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/engines_gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/prepost/cutcell_list.f90 b/src/engines_gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/prepost/cutcell_list.f90 index 2c611a1f17d..4644103ecd0 100644 --- a/src/engines_gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/prepost/cutcell_list.f90 +++ b/src/engines_gpl/dflowfm/packages/dflowfm_kernel/src/dflowfm_kernel/prepost/cutcell_list.f90 @@ -85,7 +85,7 @@ subroutine cutcell_list(n12, jamasks) jastored = 0 - inquire (FILE=md_cutcelllist, EXIST=JAWEL) + inquire (FILE=trim(md_cutcelllist), EXIST=JAWEL) NUMFIL = 0 if (JAWEL) then call OLDFIL(MLIST, md_cutcelllist) diff --git a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr.h b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr.h index d7275ec5777..cf14bb76c36 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/include/dimr.h +++ b/src/engines_gpl/dimr/packages/dimr_lib/include/dimr.h @@ -94,11 +94,7 @@ class Log; class Dimr { public: - static Dimr* GetInstance() - { - if (instance == NULL) instance = new Dimr(); - return instance; - } + static Dimr* GetInstance(); void scanConfigFile(void); void connectLibs(void); @@ -144,6 +140,7 @@ class Dimr Level feedbackLevel; const char* configfile; // name of configuration file bool done; // set to true when it's time to stop + bool logIsOwned; // false when log was injected via set_dimr_logger (owned by the caller) char* redirectFile; // Name of file to redirect stdout/stderr to // Default: Off when started via dimr-exe, On otherwise @@ -151,9 +148,6 @@ class Dimr const char* dirSeparator; // String constants; initialized below, outside class definition private: - // static Dimr *m_pInstance; - static Dimr* instance; - Dimr(); ~Dimr(); Dimr(Dimr const&) = delete; // Don't Implement. diff --git a/src/engines_gpl/dimr/packages/dimr_lib/src/bmi.cpp b/src/engines_gpl/dimr/packages/dimr_lib/src/bmi.cpp index d871ab60185..202009ea014 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/src/bmi.cpp +++ b/src/engines_gpl/dimr/packages/dimr_lib/src/bmi.cpp @@ -44,6 +44,11 @@ BMI_API void set_dimr_logger(Log* loggerFromDimrExe) { thisDimr = Dimr::GetInstance(); } + if (thisDimr->logIsOwned) + { + delete thisDimr->log; // owned by Dimr; replaced by the caller's logger + thisDimr->logIsOwned = false; + } thisDimr->log = loggerFromDimrExe; } diff --git a/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp b/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp index d241bb4cdfa..8a0959f64ef 100644 --- a/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp +++ b/src/engines_gpl/dimr/packages/dimr_lib/src/dimr.cpp @@ -83,7 +83,11 @@ #include #endif -Dimr* Dimr::instance = NULL; +Dimr* Dimr::GetInstance() +{ + static Dimr instance; + return &instance; +} Dimr::Dimr(void) { @@ -95,6 +99,7 @@ Dimr::Dimr(void) logLevel = WARNING; feedbackLevel = INFO; log = new Log(logFile, clock, logLevel, feedbackLevel); + logIsOwned = true; config = NULL; mainArgs = NULL; slaveArg = NULL; @@ -184,7 +189,10 @@ Dimr::~Dimr(void) // to do: (void) FreeLibrary(handle); freeLibs(); - log->Write(DEBUG, my_rank, "dimr shutting down normally"); + if (logIsOwned) + { + log->Write(DEBUG, my_rank, "dimr shutting down normally"); + } #ifndef _WIN32 free(exeName); @@ -192,15 +200,19 @@ Dimr::~Dimr(void) delete[] exeName; #endif + if (logIsOwned) + { + delete log; + } delete clock; delete config; free(exePath); - delete log; delete[] mainArgs; // componentsList for (int i = 0; i < componentsList.numComponents; i++) { delete[] componentsList.components[i].workingDir; + free(componentsList.components[i].processes); } free(componentsList.components); // couplersList @@ -227,7 +239,11 @@ void Dimr::deleteControlBlock(dimr_control_block cb) { for (int i = 0; i < cb.numSubBlocks; i++) { - if (cb.computeTimes->empty()) cb.computeTimes->clear(); + if (cb.computeTimes != nullptr) + { + delete cb.computeTimes; + cb.computeTimes = nullptr; + } // Recursively delete all subBlocks deleteControlBlock(cb.subBlocks[i]); } @@ -2010,16 +2026,19 @@ void Dimr::scanControl(XmlTree* controlBlockXml, dimr_control_block* controlBloc if (controlBlockXml->name == "control") { controlBlock->type = CT_SEQUENTIAL; + controlBlock->computeTimes = nullptr; } else if (controlBlockXml->name == "parallel") { controlBlock->type = CT_PARALLEL; + controlBlock->computeTimes = nullptr; } else if (controlBlockXml->name == "start") { controlBlock->type = CT_START; controlBlock->unit.component = getComponent(controlBlockXml->GetAttrib("name")); controlBlock->unit.coupler = NULL; + controlBlock->computeTimes = nullptr; } else if (controlBlockXml->name == "startGroup") { @@ -2051,6 +2070,7 @@ void Dimr::scanControl(XmlTree* controlBlockXml, dimr_control_block* controlBloc controlBlock->type = CT_COUPLER; controlBlock->unit.component = NULL; controlBlock->unit.coupler = getCoupler(controlBlockXml->GetAttrib("name")); + controlBlock->computeTimes = nullptr; } controlBlock->numSubBlocks = 0; controlBlock->subBlocks = NULL; @@ -2420,7 +2440,10 @@ void Dimr::freeLibs(void) continue; } - log->Write(ALL, my_rank, "Freeing library \"%s\"", componentsList.components[i].library); + if (logIsOwned) + { + log->Write(ALL, my_rank, "Freeing library \"%s\"", componentsList.components[i].library); + } #ifndef _WIN32 dlerror(); /* clear error code */ int ierr = dlclose(componentsList.components[i].libHandle);