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

Proposed by Martin Konrad on 2019-01-08
Status: Merged
Merged at revision: 59cb5ba6a0f66461e1b0efea28699046d44a7a8b
Proposed branch: ~info-martin-konrad/epics-base:fix-substitution-file-expansion
Merge into: epics-base:3.15
Diff against target: 531 lines (+109/-92)
9 files modified
src/ioc/dbtemplate/Makefile (+1/-1)
src/ioc/dbtemplate/msi.cpp (+71/-90)
src/ioc/dbtemplate/test/msi.plt (+7/-1)
src/ioc/dbtemplate/test/t10-result.txt (+4/-0)
src/ioc/dbtemplate/test/t10-substitute.txt (+8/-0)
src/ioc/dbtemplate/test/t10-template.txt (+2/-0)
src/ioc/dbtemplate/test/t11-result.txt (+4/-0)
src/ioc/dbtemplate/test/t11-substitute.txt (+10/-0)
src/ioc/dbtemplate/test/t11-template.txt (+2/-0)
Reviewer Review Type Date Requested Status
Andrew Johnson 2019-01-08 Pending
Review via email: mp+361500@code.launchpad.net

Description of the change

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).

To post a comment you must log in.
mdavidsaver (mdavidsaver) wrote :

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.

Andrew Johnson (anj) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/ioc/dbtemplate/Makefile b/src/ioc/dbtemplate/Makefile
2index 2aa5a0c..13b4f11 100644
3--- a/src/ioc/dbtemplate/Makefile
4+++ b/src/ioc/dbtemplate/Makefile
5@@ -13,7 +13,7 @@ SRC_DIRS += $(IOCDIR)/dbtemplate
6
7 PROD_HOST += msi
8
9-msi_SRCS = msi.c
10+msi_SRCS = msi.cpp
11 msi_LIBS += Com
12 HTMLS += msi.html
13
14diff --git a/src/ioc/dbtemplate/msi.c b/src/ioc/dbtemplate/msi.cpp
15similarity index 80%
16rename from src/ioc/dbtemplate/msi.c
17rename to src/ioc/dbtemplate/msi.cpp
18index 69680e1..9a370e8 100644
19--- a/src/ioc/dbtemplate/msi.c
20+++ b/src/ioc/dbtemplate/msi.cpp
21@@ -9,6 +9,8 @@
22
23 /* msi - macro substitutions and include */
24
25+#include <string>
26+
27 #include <stdlib.h>
28 #include <stddef.h>
29 #include <stdio.h>
30@@ -56,28 +58,31 @@ int din = 0;
31 typedef struct inputData inputData;
32
33 static void inputConstruct(inputData **ppvt);
34-static void inputDestruct(inputData *pvt);
35-static void inputAddPath(inputData *pvt, char *pval);
36-static void inputBegin(inputData *pvt, char *fileName);
37-static char *inputNextLine(inputData *pvt);
38-static void inputNewIncludeFile(inputData *pvt, char *name);
39-static void inputErrPrint(inputData *pvt);
40+static void inputDestruct(inputData * const pvt);
41+static void inputAddPath(inputData * const pvt, const char * const pval);
42+static void inputBegin(inputData * const pvt, const char * const fileName);
43+static char *inputNextLine(inputData * const pvt);
44+static void inputNewIncludeFile(inputData * const pvt, const char * const name);
45+static void inputErrPrint(const inputData * const pvt);
46
47 /* Module to read the substitution file */
48 typedef struct subInfo subInfo;
49
50-static void substituteOpen(subInfo **ppvt, char *substitutionName);
51-static void substituteDestruct(subInfo *pvt);
52-static int substituteGetNextSet(subInfo *pvt, char **filename);
53-static int substituteGetGlobalSet(subInfo *pvt);
54-static char *substituteGetReplacements(subInfo *pvt);
55-static char *substituteGetGlobalReplacements(subInfo *pvt);
56+static void substituteOpen(subInfo **ppvt, char * const substitutionName);
57+static void substituteDestruct(subInfo * const pvt);
58+static int substituteGetNextSet(subInfo * const pvt, char **filename);
59+static int substituteGetGlobalSet(subInfo * const pvt);
60+static const char *substituteGetReplacements(subInfo * const pvt);
61+static const char *substituteGetGlobalReplacements(subInfo * const pvt);
62
63 /* Forward references to local routines */
64-static void usageExit(int status);
65-static void abortExit(int status);
66-static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval);
67-static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *templateName);
68+static void usageExit(const int status);
69+static void abortExit(const int status);
70+static void addMacroReplacements(MAC_HANDLE * const macPvt,
71+ const char * const pval);
72+static void makeSubstitutions(inputData * const inputPvt,
73+ MAC_HANDLE * const macPvt,
74+ const char * const templateName);
75
76 /*Global variables */
77 static int opt_V = 0;
78@@ -180,9 +185,9 @@ int main(int argc,char **argv)
79 isGlobal = substituteGetGlobalSet(substitutePvt);
80 if (isGlobal) {
81 STEP("Handling global macros");
82- pval = substituteGetGlobalReplacements(substitutePvt);
83- if (pval)
84- addMacroReplacements(macPvt, pval);
85+ const char *macStr = substituteGetGlobalReplacements(substitutePvt);
86+ if (macStr)
87+ addMacroReplacements(macPvt, macStr);
88 }
89 else if ((isFile = substituteGetNextSet(substitutePvt, &filename))) {
90 if (templateName)
91@@ -193,11 +198,12 @@ int main(int argc,char **argv)
92 }
93
94 STEPS("Handling template file", filename);
95- while ((pval = substituteGetReplacements(substitutePvt))) {
96+ const char *macStr;
97+ while ((macStr = substituteGetReplacements(substitutePvt))) {
98 if (localScope)
99 macPushScope(macPvt);
100
101- addMacroReplacements(macPvt, pval);
102+ addMacroReplacements(macPvt, macStr);
103 makeSubstitutions(inputPvt, macPvt, filename);
104
105 if (localScope)
106@@ -218,7 +224,7 @@ int main(int argc,char **argv)
107 return opt_V & 2;
108 }
109
110
111-void usageExit(int status)
112+void usageExit(const int status)
113 {
114 fprintf(stderr,
115 "Usage: msi [options] [template]\n"
116@@ -236,7 +242,7 @@ void usageExit(int status)
117 exit(status);
118 }
119
120-void abortExit(int status)
121+void abortExit(const int status)
122 {
123 if (outFile) {
124 fclose(stdout);
125@@ -245,7 +251,8 @@ void abortExit(int status)
126 exit(status);
127 }
128
129-static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval)
130+static void addMacroReplacements(MAC_HANDLE * const macPvt,
131+ const char * const pval)
132 {
133 char **pairs;
134 long status;
135@@ -268,7 +275,9 @@ static void addMacroReplacements(MAC_HANDLE *macPvt, char *pval)
136 typedef enum {cmdInclude,cmdSubstitute} cmdType;
137 static const char *cmdNames[] = {"include","substitute"};
138
139-static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *templateName)
140+static void makeSubstitutions(inputData * const inputPvt,
141+ MAC_HANDLE * const macPvt,
142+ const char * const templateName)
143 {
144 char *input;
145 static char buffer[MAX_BUFFER_SIZE];
146@@ -325,7 +334,7 @@ static void makeSubstitutions(inputData *inputPvt, MAC_HANDLE *macPvt, char *tem
147 /*skip quote and any trailing blanks*/
148 while (*++p == ' ') ;
149 if (*p != '\n' && *p != 0) goto endcmd;
150- copy = calloc(pend-pstart + 1, sizeof(char));
151+ copy = static_cast<char *>(calloc(pend-pstart + 1, sizeof(char)));
152 strncpy(copy, pstart, pend-pstart);
153
154 switch(cmdind) {
155@@ -378,7 +387,7 @@ struct inputData {
156 char inputBuffer[MAX_BUFFER_SIZE];
157 };
158
159-static void inputOpenFile(inputData *pinputData, char *filename);
160+static void inputOpenFile(inputData *pinputData, const char * const filename);
161 static void inputCloseFile(inputData *pinputData);
162 static void inputCloseAllFiles(inputData *pinputData);
163
164@@ -386,13 +395,13 @@ static void inputConstruct(inputData **ppvt)
165 {
166 inputData *pinputData;
167
168- pinputData = calloc(1, sizeof(inputData));
169+ pinputData = static_cast<inputData *>(calloc(1, sizeof(inputData)));
170 ellInit(&pinputData->inputFileList);
171 ellInit(&pinputData->pathList);
172 *ppvt = pinputData;
173 }
174
175-static void inputDestruct(inputData *pinputData)
176+static void inputDestruct(inputData * const pinputData)
177 {
178 pathNode *ppathNode;
179
180@@ -405,7 +414,7 @@ static void inputDestruct(inputData *pinputData)
181 free(pinputData);
182 }
183
184
185-static void inputAddPath(inputData *pinputData, char *path)
186+static void inputAddPath(inputData * const pinputData, const char * const path)
187 {
188 ELLLIST *ppathList = &pinputData->pathList;
189 pathNode *ppathNode;
190@@ -448,7 +457,7 @@ static void inputAddPath(inputData *pinputData, char *path)
191 EXIT;
192 }
193
194
195-static void inputBegin(inputData *pinputData, char *fileName)
196+static void inputBegin(inputData * const pinputData, const char * const fileName)
197 {
198 ENTER;
199 inputCloseAllFiles(pinputData);
200@@ -456,7 +465,7 @@ static void inputBegin(inputData *pinputData, char *fileName)
201 EXIT;
202 }
203
204-static char *inputNextLine(inputData *pinputData)
205+static char *inputNextLine(inputData * const pinputData)
206 {
207 inputFile *pinputFile;
208 char *pline;
209@@ -475,14 +484,15 @@ static char *inputNextLine(inputData *pinputData)
210 return 0;
211 }
212
213-static void inputNewIncludeFile(inputData *pinputData, char *name)
214+static void inputNewIncludeFile(inputData * const pinputData,
215+ const char * const name)
216 {
217 ENTER;
218 inputOpenFile(pinputData,name);
219 EXIT;
220 }
221
222-static void inputErrPrint(inputData *pinputData)
223+static void inputErrPrint(const inputData *const pinputData)
224 {
225 inputFile *pinputFile;
226
227@@ -511,7 +521,7 @@ static void inputErrPrint(inputData *pinputData)
228 EXIT;
229 }
230
231
232-static void inputOpenFile(inputData *pinputData,char *filename)
233+static void inputOpenFile(inputData *pinputData, const char * const filename)
234 {
235 ELLLIST *ppathList = &pinputData->pathList;
236 pathNode *ppathNode = 0;
237@@ -531,8 +541,8 @@ static void inputOpenFile(inputData *pinputData,char *filename)
238 else {
239 ppathNode = (pathNode *) ellFirst(ppathList);
240 while (ppathNode) {
241- fullname = calloc(strlen(filename) + strlen(ppathNode->directory) + 2,
242- sizeof(char));
243+ fullname = static_cast<char *>(calloc(strlen(filename) + strlen(ppathNode->directory) + 2,
244+ sizeof(char)));
245 strcpy(fullname, ppathNode->directory);
246 strcat(fullname, "/");
247 strcat(fullname, filename);
248@@ -552,7 +562,7 @@ static void inputOpenFile(inputData *pinputData,char *filename)
249 }
250
251 STEP("File opened");
252- pinputFile = calloc(1, sizeof(inputFile));
253+ pinputFile = static_cast<inputFile *>(calloc(1, sizeof(inputFile)));
254
255 if (ppathNode) {
256 pinputFile->filename = fullname;
257@@ -647,14 +657,13 @@ struct subInfo {
258 char *filename;
259 int isPattern;
260 ELLLIST patternList;
261- size_t size;
262- size_t curLength;
263- char *macroReplacements;
264+ std::string macroReplacements;
265+ subInfo() : psubFile(NULL), isFile(0), filename(NULL), isPattern(0) {};
266 };
267
268 static char *subGetNextLine(subFile *psubFile);
269 static tokenType subGetNextToken(subFile *psubFile);
270-static void subFileErrPrint(subFile *psubFile,char * message);
271+static void subFileErrPrint(subFile *psubFile, const char * message);
272 static void freeSubFile(subInfo *psubInfo);
273 static void freePattern(subInfo *psubInfo);
274 static void catMacroReplacements(subInfo *psubInfo,const char *value);
275@@ -688,25 +697,25 @@ void freePattern(subInfo *psubInfo)
276 EXIT;
277 }
278
279
280-static void substituteDestruct(subInfo *psubInfo)
281+static void substituteDestruct(subInfo * const psubInfo)
282 {
283 ENTER;
284 freeSubFile(psubInfo);
285 freePattern(psubInfo);
286- free(psubInfo);
287+ delete(psubInfo);
288 EXIT;
289 }
290
291-static void substituteOpen(subInfo **ppvt, char *substitutionName)
292+static void substituteOpen(subInfo **ppvt, char * const substitutionName)
293 {
294 subInfo *psubInfo;
295 subFile *psubFile;
296 FILE *fp;
297
298 ENTER;
299- psubInfo = calloc(1, sizeof(subInfo));
300+ psubInfo = new subInfo;
301 *ppvt = psubInfo;
302- psubFile = calloc(1, sizeof(subFile));
303+ psubFile = static_cast<subFile *>(calloc(1, sizeof(subFile)));
304 psubInfo->psubFile = psubFile;
305 ellInit(&psubInfo->patternList);
306
307@@ -725,7 +734,7 @@ static void substituteOpen(subInfo **ppvt, char *substitutionName)
308 EXIT;
309 }
310
311-static int substituteGetGlobalSet(subInfo *psubInfo)
312+static int substituteGetGlobalSet(subInfo * const psubInfo)
313 {
314 subFile *psubFile = psubInfo->psubFile;
315
316@@ -744,7 +753,7 @@ static int substituteGetGlobalSet(subInfo *psubInfo)
317 return 0;
318 }
319
320-static int substituteGetNextSet(subInfo *psubInfo,char **filename)
321+static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
322 {
323 subFile *psubFile = psubInfo->psubFile;
324 patternNode *ppatternNode;
325@@ -831,7 +840,7 @@ static int substituteGetNextSet(subInfo *psubInfo,char **filename)
326 if (psubFile->token != tokenString)
327 break;
328
329- ppatternNode = calloc(1, sizeof(patternNode));
330+ ppatternNode = static_cast<patternNode *>(calloc(1, sizeof(patternNode)));
331 ellAdd(&psubInfo->patternList, &ppatternNode->node);
332 ppatternNode->var = epicsStrDup(psubFile->string);
333 }
334@@ -846,14 +855,12 @@ static int substituteGetNextSet(subInfo *psubInfo,char **filename)
335 return 1;
336 }
337
338
339-static char *substituteGetGlobalReplacements(subInfo *psubInfo)
340+static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
341 {
342 subFile *psubFile = psubInfo->psubFile;
343
344 ENTER;
345- if (psubInfo->macroReplacements)
346- psubInfo->macroReplacements[0] = 0;
347- psubInfo->curLength = 0;
348+ psubInfo->macroReplacements.clear();
349
350 while (psubFile->token == tokenSeparator)
351 subGetNextToken(psubFile);
352@@ -881,8 +888,8 @@ static char *substituteGetGlobalReplacements(subInfo *psubInfo)
353 switch(subGetNextToken(psubFile)) {
354 case tokenRBrace:
355 subGetNextToken(psubFile);
356- EXITS(psubInfo->macroReplacements);
357- return psubInfo->macroReplacements;
358+ EXITS(psubInfo->macroReplacements.c_str());
359+ return psubInfo->macroReplacements.c_str();
360
361 case tokenSeparator:
362 catMacroReplacements(psubInfo, ",");
363@@ -902,15 +909,13 @@ static char *substituteGetGlobalReplacements(subInfo *psubInfo)
364 }
365 }
366
367-static char *substituteGetReplacements(subInfo *psubInfo)
368+static const char *substituteGetReplacements(subInfo * const psubInfo)
369 {
370 subFile *psubFile = psubInfo->psubFile;
371 patternNode *ppatternNode;
372
373 ENTER;
374- if (psubInfo->macroReplacements)
375- psubInfo->macroReplacements[0] = 0;
376- psubInfo->curLength = 0;
377+ psubInfo->macroReplacements.clear();
378
379 while (psubFile->token == tokenSeparator)
380 subGetNextToken(psubFile);
381@@ -943,8 +948,8 @@ static char *substituteGetReplacements(subInfo *psubInfo)
382 while (1) {
383 if (psubFile->token == tokenRBrace) {
384 subGetNextToken(psubFile);
385- EXITS(psubInfo->macroReplacements);
386- return psubInfo->macroReplacements;
387+ EXITS(psubInfo->macroReplacements.c_str());
388+ return psubInfo->macroReplacements.c_str();
389 }
390
391 if (psubFile->token != tokenString) {
392@@ -973,8 +978,8 @@ static char *substituteGetReplacements(subInfo *psubInfo)
393 switch(subGetNextToken(psubFile)) {
394 case tokenRBrace:
395 subGetNextToken(psubFile);
396- EXITS(psubInfo->macroReplacements);
397- return psubInfo->macroReplacements;
398+ EXITS(psubInfo->macroReplacements.c_str());
399+ return psubInfo->macroReplacements.c_str();
400
401 case tokenSeparator:
402 catMacroReplacements(psubInfo, ",");
403@@ -1017,7 +1022,7 @@ static char *subGetNextLine(subFile *psubFile)
404 return &psubFile->inputBuffer[0];
405 }
406
407-static void subFileErrPrint(subFile *psubFile,char * message)
408+static void subFileErrPrint(subFile *psubFile, const char * message)
409 {
410 fprintf(stderr, "msi: %s\n",message);
411 fprintf(stderr, " in substitution file '%s' at line %d:\n %s",
412@@ -1107,32 +1112,8 @@ done:
413
414 static void catMacroReplacements(subInfo *psubInfo, const char *value)
415 {
416- size_t len = strlen(value);
417-
418 ENTER;
419- if (psubInfo->size <= (psubInfo->curLength + len)) {
420- size_t newsize = psubInfo->size + MAX_BUFFER_SIZE;
421- char *newbuf;
422-
423- STEP("Enlarging buffer");
424- if (newsize <= psubInfo->curLength + len)
425- newsize = psubInfo->curLength + len + 1;
426- newbuf = calloc(1, newsize);
427- if (!newbuf) {
428- fprintf(stderr, "calloc failed for size %lu\n",
429- (unsigned long) newsize);
430- abortExit(1);
431- }
432- if (psubInfo->macroReplacements) {
433- memcpy(newbuf, psubInfo->macroReplacements, psubInfo->curLength);
434- free(psubInfo->macroReplacements);
435- }
436- psubInfo->size = newsize;
437- psubInfo->macroReplacements = newbuf;
438- }
439-
440 STEPS("Appending", value);
441- strcat(psubInfo->macroReplacements, value);
442- psubInfo->curLength += len;
443+ psubInfo->macroReplacements += value;
444 EXIT;
445 }
446diff --git a/src/ioc/dbtemplate/test/msi.plt b/src/ioc/dbtemplate/test/msi.plt
447index 4149125..332defb 100644
448--- a/src/ioc/dbtemplate/test/msi.plt
449+++ b/src/ioc/dbtemplate/test/msi.plt
450@@ -11,7 +11,7 @@
451 use strict;
452 use Test;
453
454-BEGIN {plan tests => 9}
455+BEGIN {plan tests => 11}
456
457 # Check include/substitute command model
458 ok(msi('-I .. ../t1-template.txt'), slurp('../t1-result.txt'));
459@@ -50,6 +50,12 @@ ok(msi('-I.. -D -o t8.txt ../t1-template.txt'), slurp('../t8-result.txt'));
460 # Dependency generation, dbLoadTemplate format
461 ok(msi('-I.. -D -ot9.txt -S ../t2-substitution.txt'), slurp('../t9-result.txt'));
462
463+# Substitution file, variable format, with 0 variable definitions
464+ok(msi('-I. -I.. -S ../t10-substitute.txt'), slurp('../t10-result.txt'));
465+
466+# Substitution file, pattern format, with 0 pattern definitions
467+ok(msi('-I. -I.. -S ../t11-substitute.txt'), slurp('../t11-result.txt'));
468+
469
470 # Test support routines
471
472diff --git a/src/ioc/dbtemplate/test/t10-result.txt b/src/ioc/dbtemplate/test/t10-result.txt
473new file mode 100644
474index 0000000..47b594e
475--- /dev/null
476+++ b/src/ioc/dbtemplate/test/t10-result.txt
477@@ -0,0 +1,4 @@
478+# comment line
479+a=$(a)
480+# comment line
481+a=gbl
482diff --git a/src/ioc/dbtemplate/test/t10-substitute.txt b/src/ioc/dbtemplate/test/t10-substitute.txt
483new file mode 100644
484index 0000000..aec88bb
485--- /dev/null
486+++ b/src/ioc/dbtemplate/test/t10-substitute.txt
487@@ -0,0 +1,8 @@
488+file t10-template.txt {
489+ {}
490+}
491+
492+global { a=gbl }
493+file t10-template.txt {
494+ {}
495+}
496diff --git a/src/ioc/dbtemplate/test/t10-template.txt b/src/ioc/dbtemplate/test/t10-template.txt
497new file mode 100644
498index 0000000..7958885
499--- /dev/null
500+++ b/src/ioc/dbtemplate/test/t10-template.txt
501@@ -0,0 +1,2 @@
502+# comment line
503+a=$(a)
504diff --git a/src/ioc/dbtemplate/test/t11-result.txt b/src/ioc/dbtemplate/test/t11-result.txt
505new file mode 100644
506index 0000000..47b594e
507--- /dev/null
508+++ b/src/ioc/dbtemplate/test/t11-result.txt
509@@ -0,0 +1,4 @@
510+# comment line
511+a=$(a)
512+# comment line
513+a=gbl
514diff --git a/src/ioc/dbtemplate/test/t11-substitute.txt b/src/ioc/dbtemplate/test/t11-substitute.txt
515new file mode 100644
516index 0000000..94dcdbc
517--- /dev/null
518+++ b/src/ioc/dbtemplate/test/t11-substitute.txt
519@@ -0,0 +1,10 @@
520+file t11-template.txt {
521+ pattern {}
522+ {}
523+}
524+
525+global { a=gbl }
526+file t11-template.txt {
527+ pattern {}
528+ {}
529+}
530diff --git a/src/ioc/dbtemplate/test/t11-template.txt b/src/ioc/dbtemplate/test/t11-template.txt
531new file mode 100644
532index 0000000..7958885
533--- /dev/null
534+++ b/src/ioc/dbtemplate/test/t11-template.txt
535@@ -0,0 +1,2 @@
536+# comment line
537+a=$(a)

Subscribers

People subscribed via source and target branches