Merge ~info-martin-konrad/epics-base:fix-substitution-file-expansion into ~epics-core/epics-base/+git/epics-base:3.15

[Martin Konrad]

Fix https://bugs.launchpad.net/epics-base/+bug/1810946 and https://bugs.launchpad.net/epics-base/+bug/1810949.

It's way too easy to goof these kind of things up when doing manual memory management. By converting MSI to C++ I'm trying to start some improvements with regards to this. I have a bunch of other clean up changes we can look at once this issue is fixed (see https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+ref/clean-up-msi).

[mdavidsaver (mdavidsaver)]

Looking at the change in this MR I see a good start with a lot of work remaining. Looking at your clean-up-msi branch, I see that a lot of the remaining work is done there. If you don't want to fix the leak in the C code first, then I'd rather review all of the c++ conversion at once. The main points which make this c++ code (eg. ellLib -> std::list) are on the clean-up-msi branch.

[Martin Konrad (info-martin-konrad)]

I have created a MP for my clean-up-msi branch: https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+merge/361502.

[Andrew Johnson (anj)]

Martin, please clarify which of your two MRs should be merged, we think we're okay with fixing these bugs.

[Martin Konrad (info-martin-konrad)]

This one only fixes the immediate problem I ran into. https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+ref/clean-up-msi contains the same commits plus some additional clean up that should make similar problems less likely. I would recommend merging the latter.

diff --git a/src/ioc/dbtemplate/Makefile b/src/ioc/dbtemplate/Makefile
index 2aa5a0c..13b4f11 100644
--- a/src/ioc/dbtemplate/Makefile
+++ b/src/ioc/dbtemplate/Makefile
@@ -13,7 +13,7 @@ SRC_DIRS += $(IOCDIR)/dbtemplate
 
 PROD_HOST += msi
 
-msi_SRCS = msi.c
+msi_SRCS = msi.cpp
 msi_LIBS += Com
 HTMLS += msi.html
 
diff --git a/src/ioc/dbtemplate/msi.c b/src/ioc/dbtemplate/msi.cpp
 similarity index 80%
 rename from src/ioc/dbtemplate/msi.c
 rename to src/ioc/dbtemplate/msi.cpp
index 69680e1..9a370e8 100644
--- a/src/ioc/dbtemplate/msi.c
+++ b/src/ioc/dbtemplate/msi.cpp
@@ -9,6 +9,8 @@
 
 /* msi - macro substitutions and include */
 
+#include <string>
+
 #include <stdlib.h>
 #include <stddef.h>
 #include <stdio.h>
@@ -56,28 +58,31 @@ int din = 0;
 typedef struct inputData inputData;
 
 static void inputConstruct(inputData **ppvt);
-static void inputDestruct(inputData *pvt);
-static void inputAddPath(inputData *pvt, char *pval);
-static void inputBegin(inputData *pvt, char *fileName);
-static char *inputNextLine(inputData *pvt);
-static void inputNewIncludeFile(inputData *pvt, char *name);
-static void inputErrPrint(inputData *pvt);
+static void inputDestruct(inputData * const pvt);
+static void inputAddPath(inputData * const pvt, const char * const pval);
+static void inputBegin(inputData * const pvt, const char * const fileName);
+static char *inputNextLine(inputData * const pvt);
+static void inputNewIncludeFile(inputData * const pvt, const char * const name);
+static void inputErrPrint(const inputData * const pvt);
 
 /* Module to read the substitution file */
 typedef struct subInfo subInfo;
 
-static void substituteOpen(subInfo **ppvt, char *substitutionName);
-static void substituteDestruct(subInfo *pvt);
-static int substituteGetNextSet(subInfo *pvt, char **filename);
-static int substituteGetGlobalSet(subInfo *pvt);
-static char *substituteGetReplacements(subInfo *pvt);
-static char *substituteGetGlobalReplacements(subInfo *pvt);
+static void substituteOpen(subInfo **ppvt, char * const substitutionName);
+static void substituteDestruct(subInfo * const pvt);
+static int substituteGetNextSet(subInfo * const pvt, char **filename);
+static int substituteGetGlobalSet(subInfo * const pvt);
+static const char *substituteGetReplacements(subInfo * const pvt);
+static const char *substituteGetGlobalReplacements(subInfo * const pvt);
 
 /* Forward references to local routines */
-static void usageExit(int status);
-static void abortExit(int status);
-static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval);
-static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *templateName);
+static void usageExit(const int status);
+static void abortExit(const int status);
+static void addMacroReplacements(MAC_HANDLE * const macPvt,
+                                 const char * const pval);
+static void makeSubstitutions(inputData * const inputPvt,
+                              MAC_HANDLE * const macPvt,
+                              const char * const templateName);
 
 /*Global variables */
 static int opt_V = 0;
@@ -180,9 +185,9 @@ int main(int argc,char **argv)
             isGlobal = substituteGetGlobalSet(substitutePvt);
             if (isGlobal) {
                 STEP("Handling global macros");
-                pval = substituteGetGlobalReplacements(substitutePvt);
-                if (pval)
-                    addMacroReplacements(macPvt, pval);
+                const char *macStr = substituteGetGlobalReplacements(substitutePvt);
+                if (macStr)
+                    addMacroReplacements(macPvt, macStr);
             }
             else if ((isFile = substituteGetNextSet(substitutePvt, &filename))) {
                 if (templateName)
@@ -193,11 +198,12 @@ int main(int argc,char **argv)
                 }
 
                 STEPS("Handling template file", filename);
-                while ((pval = substituteGetReplacements(substitutePvt))) {
+                const char *macStr;
+                while ((macStr = substituteGetReplacements(substitutePvt))) {
                     if (localScope)
                         macPushScope(macPvt);
 
-                    addMacroReplacements(macPvt, pval);
+                    addMacroReplacements(macPvt, macStr);
                     makeSubstitutions(inputPvt, macPvt, filename);
 
                     if (localScope)
@@ -218,7 +224,7 @@ int main(int argc,char **argv)
     return opt_V & 2;
 }
 
 
-void usageExit(int status)
+void usageExit(const int status)
 {
     fprintf(stderr,
         "Usage: msi [options] [template]\n"
@@ -236,7 +242,7 @@ void usageExit(int status)
     exit(status);
 }
 
-void abortExit(int status)
+void abortExit(const int status)
 {
     if (outFile) {
         fclose(stdout);
@@ -245,7 +251,8 @@ void abortExit(int status)
     exit(status);
 }
 
-static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval)
+static void addMacroReplacements(MAC_HANDLE * const macPvt,
+                                 const char * const pval)
 {
     char **pairs;
     long status;
@@ -268,7 +275,9 @@ static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval)
 typedef enum {cmdInclude,cmdSubstitute} cmdType;
 static const char *cmdNames[] = {"include","substitute"};
 
-static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *templateName)
+static void makeSubstitutions(inputData * const inputPvt,
+                              MAC_HANDLE * const macPvt,
+                              const char * const templateName)
 {
     char *input;
     static char buffer[MAX_BUFFER_SIZE];
@@ -325,7 +334,7 @@ static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *tem
             /*skip quote and any trailing blanks*/
             while (*++p == ' ') ;
             if (*p != '\n' && *p != 0) goto endcmd;
-            copy = calloc(pend-pstart + 1, sizeof(char));
+            copy = static_cast<char *>(calloc(pend-pstart + 1, sizeof(char)));
             strncpy(copy, pstart, pend-pstart);
 
             switch(cmdind) {
@@ -378,7 +387,7 @@ struct inputData {
     char        inputBuffer[MAX_BUFFER_SIZE];
 };
 
-static void inputOpenFile(inputData *pinputData, char *filename);
+static void inputOpenFile(inputData *pinputData, const char * const filename);
 static void inputCloseFile(inputData *pinputData);
 static void inputCloseAllFiles(inputData *pinputData);
 
@@ -386,13 +395,13 @@ static void inputConstruct(inputData **ppvt)
 {
     inputData *pinputData;
 
-    pinputData = calloc(1, sizeof(inputData));
+    pinputData = static_cast<inputData *>(calloc(1, sizeof(inputData)));
     ellInit(&pinputData->inputFileList);
     ellInit(&pinputData->pathList);
     *ppvt = pinputData;
 }
 
-static void inputDestruct(inputData *pinputData)
+static void inputDestruct(inputData * const pinputData)
 {
     pathNode *ppathNode;
 
@@ -405,7 +414,7 @@ static void inputDestruct(inputData *pinputData)
     free(pinputData);
 }
 
 
-static void inputAddPath(inputData *pinputData, char *path)
+static void inputAddPath(inputData * const pinputData, const char * const path)
 {
     ELLLIST     *ppathList = &pinputData->pathList;
     pathNode    *ppathNode;
@@ -448,7 +457,7 @@ static void inputAddPath(inputData *pinputData, char *path)
     EXIT;
 }
 
 
-static void inputBegin(inputData *pinputData, char *fileName)
+static void inputBegin(inputData * const pinputData, const char * const fileName)
 {
     ENTER;
     inputCloseAllFiles(pinputData);
@@ -456,7 +465,7 @@ static void inputBegin(inputData *pinputData, char *fileName)
     EXIT;
 }
 
-static char *inputNextLine(inputData *pinputData)
+static char *inputNextLine(inputData * const pinputData)
 {
     inputFile   *pinputFile;
     char        *pline;
@@ -475,14 +484,15 @@ static char *inputNextLine(inputData *pinputData)
     return 0;
 }
 
-static void inputNewIncludeFile(inputData *pinputData, char *name)
+static void inputNewIncludeFile(inputData * const pinputData,
+                                const char * const name)
 {
     ENTER;
     inputOpenFile(pinputData,name);
     EXIT;
 }
 
-static void inputErrPrint(inputData *pinputData)
+static void inputErrPrint(const inputData *const pinputData)
 {
     inputFile   *pinputFile;
 
@@ -511,7 +521,7 @@ static void inputErrPrint(inputData *pinputData)
     EXIT;
 }
 
 
-static void inputOpenFile(inputData *pinputData,char *filename)
+static void inputOpenFile(inputData *pinputData, const char * const filename)
 {
     ELLLIST     *ppathList = &pinputData->pathList;
     pathNode    *ppathNode = 0;
@@ -531,8 +541,8 @@ static void inputOpenFile(inputData *pinputData,char *filename)
     else {
         ppathNode = (pathNode *) ellFirst(ppathList);
         while (ppathNode) {
-            fullname = calloc(strlen(filename) + strlen(ppathNode->directory) + 2,
-                sizeof(char));
+            fullname = static_cast<char *>(calloc(strlen(filename) + strlen(ppathNode->directory) + 2,
+                sizeof(char)));
             strcpy(fullname, ppathNode->directory);
             strcat(fullname, "/");
             strcat(fullname, filename);
@@ -552,7 +562,7 @@ static void inputOpenFile(inputData *pinputData,char *filename)
     }
 
     STEP("File opened");
-    pinputFile = calloc(1, sizeof(inputFile));
+    pinputFile = static_cast<inputFile *>(calloc(1, sizeof(inputFile)));
 
     if (ppathNode) {
         pinputFile->filename = fullname;
@@ -647,14 +657,13 @@ struct subInfo {
     char        *filename;
     int         isPattern;
     ELLLIST     patternList;
-    size_t      size;
-    size_t      curLength;
-    char        *macroReplacements;
+    std::string macroReplacements;
+    subInfo() : psubFile(NULL), isFile(0), filename(NULL), isPattern(0) {};
 };
 
 static char *subGetNextLine(subFile *psubFile);
 static tokenType subGetNextToken(subFile *psubFile);
-static void subFileErrPrint(subFile *psubFile,char * message);
+static void subFileErrPrint(subFile *psubFile, const char * message);
 static void freeSubFile(subInfo *psubInfo);
 static void freePattern(subInfo *psubInfo);
 static void catMacroReplacements(subInfo *psubInfo,const char *value);
@@ -688,25 +697,25 @@ void freePattern(subInfo *psubInfo)
     EXIT;
 }
 
 
-static void substituteDestruct(subInfo *psubInfo)
+static void substituteDestruct(subInfo * const psubInfo)
 {
     ENTER;
     freeSubFile(psubInfo);
     freePattern(psubInfo);
-    free(psubInfo);
+    delete(psubInfo);
     EXIT;
 }
 
-static void substituteOpen(subInfo **ppvt, char *substitutionName)
+static void substituteOpen(subInfo **ppvt, char * const substitutionName)
 {
     subInfo     *psubInfo;
     subFile     *psubFile;
     FILE        *fp;
 
     ENTER;
-    psubInfo = calloc(1, sizeof(subInfo));
+    psubInfo = new subInfo;
     *ppvt = psubInfo;
-    psubFile = calloc(1, sizeof(subFile));
+    psubFile = static_cast<subFile *>(calloc(1, sizeof(subFile)));
     psubInfo->psubFile = psubFile;
     ellInit(&psubInfo->patternList);
 
@@ -725,7 +734,7 @@ static void substituteOpen(subInfo **ppvt, char *substitutionName)
     EXIT;
 }
 
-static int substituteGetGlobalSet(subInfo *psubInfo)
+static int substituteGetGlobalSet(subInfo * const psubInfo)
 {
     subFile *psubFile = psubInfo->psubFile;
 
@@ -744,7 +753,7 @@ static int substituteGetGlobalSet(subInfo *psubInfo)
     return 0;
 }
 
-static int substituteGetNextSet(subInfo *psubInfo,char **filename)
+static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
 {
     subFile     *psubFile = psubInfo->psubFile;
     patternNode *ppatternNode;
@@ -831,7 +840,7 @@ static int substituteGetNextSet(subInfo *psubInfo,char **filename)
         if (psubFile->token != tokenString)
             break;
 
-        ppatternNode = calloc(1, sizeof(patternNode));
+        ppatternNode = static_cast<patternNode *>(calloc(1, sizeof(patternNode)));
         ellAdd(&psubInfo->patternList, &ppatternNode->node);
         ppatternNode->var = epicsStrDup(psubFile->string);
     }
@@ -846,14 +855,12 @@ static int substituteGetNextSet(subInfo *psubInfo,char **filename)
     return 1;
 }
 
 
-static char *substituteGetGlobalReplacements(subInfo *psubInfo)
+static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
 {
     subFile     *psubFile = psubInfo->psubFile;
 
     ENTER;
-    if (psubInfo->macroReplacements)
-        psubInfo->macroReplacements[0] = 0;
-    psubInfo->curLength = 0;
+    psubInfo->macroReplacements.clear();
 
     while (psubFile->token == tokenSeparator)
         subGetNextToken(psubFile);
@@ -881,8 +888,8 @@ static char *substituteGetGlobalReplacements(subInfo *psubInfo)
         switch(subGetNextToken(psubFile)) {
             case tokenRBrace:
                 subGetNextToken(psubFile);
-                EXITS(psubInfo->macroReplacements);
-                return psubInfo->macroReplacements;
+                EXITS(psubInfo->macroReplacements.c_str());
+                return psubInfo->macroReplacements.c_str();
 
             case tokenSeparator:
                 catMacroReplacements(psubInfo, ",");
@@ -902,15 +909,13 @@ static char *substituteGetGlobalReplacements(subInfo *psubInfo)
     }
 }
 
-static char *substituteGetReplacements(subInfo *psubInfo)
+static const char *substituteGetReplacements(subInfo * const psubInfo)
 {
     subFile     *psubFile = psubInfo->psubFile;
     patternNode *ppatternNode;
 
     ENTER;
-    if (psubInfo->macroReplacements)
-        psubInfo->macroReplacements[0] = 0;
-    psubInfo->curLength = 0;
+    psubInfo->macroReplacements.clear();
 
     while (psubFile->token == tokenSeparator)
         subGetNextToken(psubFile);
@@ -943,8 +948,8 @@ static char *substituteGetReplacements(subInfo *psubInfo)
         while (1) {
             if (psubFile->token == tokenRBrace) {
                 subGetNextToken(psubFile);
-                EXITS(psubInfo->macroReplacements);
-                return psubInfo->macroReplacements;
+                EXITS(psubInfo->macroReplacements.c_str());
+                return psubInfo->macroReplacements.c_str();
             }
 
             if (psubFile->token != tokenString) {
@@ -973,8 +978,8 @@ static char *substituteGetReplacements(subInfo *psubInfo)
         switch(subGetNextToken(psubFile)) {
             case tokenRBrace:
                 subGetNextToken(psubFile);
-                EXITS(psubInfo->macroReplacements);
-                return psubInfo->macroReplacements;
+                EXITS(psubInfo->macroReplacements.c_str());
+                return psubInfo->macroReplacements.c_str();
 
             case tokenSeparator:
                 catMacroReplacements(psubInfo, ",");
@@ -1017,7 +1022,7 @@ static char *subGetNextLine(subFile *psubFile)
     return &psubFile->inputBuffer[0];
 }
 
-static void subFileErrPrint(subFile *psubFile,char * message)
+static void subFileErrPrint(subFile *psubFile, const char * message)
 {
     fprintf(stderr, "msi: %s\n",message);
     fprintf(stderr, "  in substitution file '%s' at line %d:\n  %s",
@@ -1107,32 +1112,8 @@ done:
 
 static void catMacroReplacements(subInfo *psubInfo, const char *value)
 {
-    size_t len = strlen(value);
-
     ENTER;
-    if (psubInfo->size <= (psubInfo->curLength + len)) {
-        size_t newsize = psubInfo->size + MAX_BUFFER_SIZE;
-        char *newbuf;
-
-        STEP("Enlarging buffer");
-        if (newsize <= psubInfo->curLength + len)
-            newsize = psubInfo->curLength + len + 1;
-        newbuf = calloc(1, newsize);
-        if (!newbuf) {
-            fprintf(stderr, "calloc failed for size %lu\n",
-                (unsigned long) newsize);
-            abortExit(1);
-        }
-        if (psubInfo->macroReplacements) {
-            memcpy(newbuf, psubInfo->macroReplacements, psubInfo->curLength);
-            free(psubInfo->macroReplacements);
-        }
-        psubInfo->size = newsize;
-        psubInfo->macroReplacements = newbuf;
-    }
-
     STEPS("Appending", value);
-    strcat(psubInfo->macroReplacements, value);
-    psubInfo->curLength += len;
+    psubInfo->macroReplacements += value;
     EXIT;
 }
diff --git a/src/ioc/dbtemplate/test/msi.plt b/src/ioc/dbtemplate/test/msi.plt
index 4149125..332defb 100644
--- a/src/ioc/dbtemplate/test/msi.plt
+++ b/src/ioc/dbtemplate/test/msi.plt
@@ -11,7 +11,7 @@
 use strict;
 use Test;
 
-BEGIN {plan tests => 9}
+BEGIN {plan tests => 11}
 
 # Check include/substitute command model
 ok(msi('-I .. ../t1-template.txt'),             slurp('../t1-result.txt'));
@@ -50,6 +50,12 @@ ok(msi('-I.. -D -o t8.txt ../t1-template.txt'), slurp('../t8-result.txt'));
 # Dependency generation, dbLoadTemplate format
 ok(msi('-I.. -D -ot9.txt -S ../t2-substitution.txt'), slurp('../t9-result.txt'));
 
+# Substitution file, variable format, with 0 variable definitions
+ok(msi('-I. -I.. -S ../t10-substitute.txt'), slurp('../t10-result.txt'));
+
+# Substitution file, pattern format, with 0 pattern definitions
+ok(msi('-I. -I.. -S ../t11-substitute.txt'), slurp('../t11-result.txt'));
+
 
 # Test support routines
 
diff --git a/src/ioc/dbtemplate/test/t10-result.txt b/src/ioc/dbtemplate/test/t10-result.txt
 new file mode 100644
index 0000000..47b594e
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t10-result.txt
@@ -0,0 +1,4 @@
+# comment line
+a=$(a)
+# comment line
+a=gbl
diff --git a/src/ioc/dbtemplate/test/t10-substitute.txt b/src/ioc/dbtemplate/test/t10-substitute.txt
 new file mode 100644
index 0000000..aec88bb
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t10-substitute.txt
@@ -0,0 +1,8 @@
+file t10-template.txt {
+    {}
+}
+
+global { a=gbl }
+file t10-template.txt {
+    {}
+}
diff --git a/src/ioc/dbtemplate/test/t10-template.txt b/src/ioc/dbtemplate/test/t10-template.txt
 new file mode 100644
index 0000000..7958885
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t10-template.txt
@@ -0,0 +1,2 @@
+# comment line
+a=$(a)
diff --git a/src/ioc/dbtemplate/test/t11-result.txt b/src/ioc/dbtemplate/test/t11-result.txt
 new file mode 100644
index 0000000..47b594e
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t11-result.txt
@@ -0,0 +1,4 @@
+# comment line
+a=$(a)
+# comment line
+a=gbl
diff --git a/src/ioc/dbtemplate/test/t11-substitute.txt b/src/ioc/dbtemplate/test/t11-substitute.txt
 new file mode 100644
index 0000000..94dcdbc
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t11-substitute.txt
@@ -0,0 +1,10 @@
+file t11-template.txt {
+    pattern {}
+    {}
+}
+
+global { a=gbl }
+file t11-template.txt {
+    pattern {}
+    {}
+}
diff --git a/src/ioc/dbtemplate/test/t11-template.txt b/src/ioc/dbtemplate/test/t11-template.txt
 new file mode 100644
index 0000000..7958885
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t11-template.txt
@@ -0,0 +1,2 @@
+# comment line
+a=$(a)