| blob | 6f69ecb1f5880751a08a8c6a335681cab6591223 |
1 Subject: [PATCH v4] Program: fix memleaks and free while in use
3 This is my proposal for fixing several issues with the handling of compiled
4 macro programs.
6 The first is a memory leak in source/macro.c::readCheckMacroString(), if we
7 the caller wants to parse only the macro, i.e. passing runWindow == NULL.
8 This was reported as SF BUG#2078867.
10 The second is more subtile: if a macro function definition overwrites a
11 previously one we unconditionaly free the old program. But this old program
12 is may be currently in use. To solve this I have introduced a reference count
13 for the program and FreeProgram() really releases the program only if this
14 drops to zero. To track references to the program it is needed to put the
15 program pointer into the stack frame and increment the reference count.
17 There are currently 3 places were we build up a stacke frame, each needs
18 different handling of the program pointer:
20 1) ExecuteMacro():
21 The caller is already responsible for freeing the prog (its runMacro()).
22 Therefore we increment the ref to allow a double free.
24 2) RunMacroAsSubrCall():
25 Here the caller passes the reference count to this function, so we don't
26 need to increment the reference.
27 The callers are runMacro() and readCheckMacroString().
29 3) callSubroutine():
30 This is the usual usage: increment the reference and put it into the stack
31 frame.
33 All three places are now consolidated into the function setupFrame().
35 In case of an error, all frames need to be rewinded. This is handled
36 with the new function rewindFrame(). Which is also used in returnValOrNone(),
37 therefore consolidates this too.
39 Before we can rewind the whole stack we need to update the execution context,
40 therefore these new calls to saveContext().
42 This was reported as SF BUG#2113904. Tony reported a second case where we hit
43 the 'free while in use' case, but I haven't done a test case for this yet. I
44 assume that this is also catched by this patch.
46 ---
48 Changes in v4:
49 * remove the special case of ExecuteMacro(), just increment the ref
51 Changes in v3:
52 * fix pushing of old frame pointer
54 If there are no objections, I commit this by the end of the week, plus my
55 proposal B for SF: BUG#2074318, from here:
57 http://www.nedit.org/pipermail/develop/2008-August/014628.html
59 source/interpret.c | 197 +++++++++++++++++++++++++++++------------------------
60 source/interpret.h | 1
61 source/macro.c | 6 +
62 3 files changed, 117 insertions(+), 87 deletions(-)
64 diff --quilt old/source/macro.c new/source/macro.c
65 --- old/source/macro.c
66 +++ new/source/macro.c
67 @@ -923,6 +923,7 @@ static int readCheckMacroString(Widget d
68 if (runWindow != NULL) {
69 XEvent nextEvent;
70 if (runWindow->macroCmdData == NULL) {
71 + /* runMacro() is responsable for freeing prog */
72 runMacro(runWindow, prog);
73 while (runWindow->macroCmdData != NULL) {
74 XtAppNextEvent(XtWidgetToApplicationContext(
75 @@ -945,6 +946,11 @@ static int readCheckMacroString(Widget d
76 }
77 inPtr = stoppedAt;
78 }
79 +
80 + /* we are in 'parse only' mode, therefore release the prog */
81 + if (runWindow == NULL) {
82 + FreeProgram(prog);
83 + }
84 }
86 /* Unroll reversal stack for macros loaded from macros. */
87 diff --quilt old/source/interpret.c new/source/interpret.c
88 --- old/source/interpret.c
89 +++ new/source/interpret.c
90 @@ -215,12 +215,14 @@ static int (*OpFns[N_OPS])() = {returnNo
91 #define FP_ARG_COUNT_INDEX (-2)
92 #define FP_OLD_FP_INDEX (-3)
93 #define FP_RET_PC_INDEX (-4)
94 -#define FP_TO_ARGS_DIST (4) /* should be 0 - (above index) */
95 +#define FP_PROG_INDEX (-5)
96 +#define FP_TO_ARGS_DIST (5) /* should be 0 - (above index) */
97 #define FP_GET_ITEM(xFrameP,xIndex) (*(xFrameP + xIndex))
98 #define FP_GET_ARG_ARRAY_CACHE(xFrameP) (FP_GET_ITEM(xFrameP, FP_ARG_ARRAY_CACHE_INDEX))
99 #define FP_GET_ARG_COUNT(xFrameP) (FP_GET_ITEM(xFrameP, FP_ARG_COUNT_INDEX).val.n)
100 #define FP_GET_OLD_FP(xFrameP) ((FP_GET_ITEM(xFrameP, FP_OLD_FP_INDEX)).val.dataval)
101 #define FP_GET_RET_PC(xFrameP) ((FP_GET_ITEM(xFrameP, FP_RET_PC_INDEX)).val.inst)
102 +#define FP_GET_PROG(xFrameP) ((FP_GET_ITEM(xFrameP, FP_PROG_INDEX)).val.prog)
103 #define FP_ARG_START_INDEX(xFrameP) (-(FP_GET_ARG_COUNT(xFrameP) + FP_TO_ARGS_DIST))
104 #define FP_GET_ARG_N(xFrameP,xN) (FP_GET_ITEM(xFrameP, xN + FP_ARG_START_INDEX(xFrameP)))
105 #define FP_GET_SYM_N(xFrameP,xN) (FP_GET_ITEM(xFrameP, xN))
106 @@ -297,6 +299,7 @@ Program *FinishCreatingProgram(void)
107 newProg->code = (Inst *)XtMalloc(progLen);
108 memcpy(newProg->code, Prog, progLen);
109 newProg->localSymList = LocalSymList;
110 + newProg->refcount = 1;
111 LocalSymList = NULL;
113 /* Local variables' values are stored on the stack. Here we assign
114 @@ -311,9 +314,11 @@ Program *FinishCreatingProgram(void)
116 void FreeProgram(Program *prog)
117 {
118 - freeSymbolTable(prog->localSymList);
119 - XtFree((char *)prog->code);
120 - XtFree((char *)prog);
121 + if (--prog->refcount == 0) {
122 + freeSymbolTable(prog->localSymList);
123 + XtFree((char *)prog->code);
124 + XtFree((char *)prog);
125 + }
126 }
128 /*
129 @@ -459,6 +464,90 @@ void FillLoopAddrs(Inst *breakAddr, Inst
130 }
132 /*
133 +** helper function to setup the next frame
134 +*/
135 +static void setupFrame(DataValue **frameP, DataValue **stackP,
136 + Inst **pc, Program *prog, int nArgs, DataValue *args)
137 +{
138 + static DataValue noValue = {NO_TAG, {0}};
139 + int i;
140 + Symbol *s;
141 +
142 + /* Push arguments and caller information onto the stack */
143 + if (nArgs >= 0) {
144 + for (i = 0; i < nArgs; i++) {
145 + *((*stackP)++) = args[i];
146 + }
147 + }
148 + else {
149 + /* a negative nArgs means, that the arguments are on the stack
150 + ** already
151 + */
152 + nArgs = -nArgs;
153 + }
154 +
155 + /* prog to free, if requested */
156 + (*stackP)->tag = NO_TAG;
157 + (*stackP)->val.prog = prog;
158 + (*stackP)++;
159 +
160 + /* return PC */
161 + (*stackP)->tag = NO_TAG;
162 + (*stackP)->val.inst = *pc;
163 + (*stackP)++;
164 +
165 + /* old FrameP */
166 + (*stackP)->tag = NO_TAG;
167 + (*stackP)->val.dataval = *frameP;
168 + (*stackP)++;
169 +
170 + /* nArgs */
171 + (*stackP)->tag = NO_TAG;
172 + (*stackP)->val.n = nArgs;
173 + (*stackP)++;
174 +
175 + /* cached arg array */
176 + *((*stackP)++) = noValue;
177 +
178 + *frameP = *stackP;
179 +
180 + /* Initialize and make room on the stack for local variables */
181 + for (s = prog->localSymList; s != NULL; s = s->next) {
182 + FP_GET_SYM_VAL(*frameP, s) = noValue;
183 + (*stackP)++;
184 + }
185 +
186 + *pc = prog->code;
187 +}
188 +
189 +static Inst *rewindFrame(DataValue **frameP, DataValue **stackP)
190 +{
191 + /* get stored return information */
192 + int nArgs = FP_GET_ARG_COUNT(*frameP);
193 + DataValue *newFrameP = FP_GET_OLD_FP(*frameP);
194 + Inst *newPC = FP_GET_RET_PC(*frameP);
195 + Program *prog = FP_GET_PROG(*frameP);
196 +
197 + /* pop past local variables */
198 + *stackP = *frameP;
199 + /* pop past arguments */
200 + *stackP -= (FP_TO_ARGS_DIST + nArgs);
201 +
202 + *frameP = newFrameP;
203 +
204 + FreeProgram(prog);
205 +
206 + return newPC;
207 +}
208 +
209 +static void rewindStack(Inst *pc, DataValue *frameP, DataValue *stackP)
210 +{
211 + while (pc) {
212 + pc = rewindFrame(&frameP, &stackP);
213 + }
214 +}
215 +
216 +/*
217 ** Execute a compiled macro, "prog", using the arguments in the array
218 ** "args". Returns one of MACRO_DONE, MACRO_PREEMPT, or MACRO_ERROR.
219 ** if MACRO_DONE is returned, the macro completed, and the returned value
220 @@ -480,33 +569,15 @@ int ExecuteMacro(WindowInfo *window, Pro
221 context->stack = (DataValue *)XtMalloc(sizeof(DataValue) * STACK_SIZE);
222 *continuation = context;
223 context->stackP = context->stack;
224 - context->pc = prog->code;
225 context->runWindow = window;
226 context->focusWindow = window;
228 - /* Push arguments and call information onto the stack */
229 - for (i=0; i<nArgs; i++)
230 - *(context->stackP++) = args[i];
231 -
232 - context->stackP->val.subr = NULL; /* return PC */
233 - context->stackP->tag = NO_TAG;
234 - context->stackP++;
235 -
236 - *(context->stackP++) = noValue; /* old FrameP */
237 -
238 - context->stackP->tag = NO_TAG; /* nArgs */
239 - context->stackP->val.n = nArgs;
240 - context->stackP++;
241 -
242 - *(context->stackP++) = noValue; /* cached arg array */
243 -
244 - context->frameP = context->stackP;
245 -
246 - /* Initialize and make room on the stack for local variables */
247 - for (s = prog->localSymList; s != NULL; s = s->next) {
248 - FP_GET_SYM_VAL(context->frameP, s) = noValue;
249 - context->stackP++;
250 - }
251 + /* pre-set the return PC to NULL */
252 + context->pc = NULL;
253 + /* prog will be freed by cller, but by stack also, so inc refcount */
254 + prog->refcount++;
255 + setupFrame(&context->frameP, &context->stackP, &context->pc,
256 + prog, nArgs, args);
258 /* Begin execution, return on error or preemption */
259 return ContinueMacro(context, result, msg);
260 @@ -547,12 +618,14 @@ int ContinueMacro(RestartData *continuat
261 return MACRO_PREEMPT;
262 } else if (status == STAT_ERROR) {
263 *msg = ErrMsg;
264 + saveContext(continuation);
265 FreeRestartData(continuation);
266 restoreContext(&oldContext);
267 return MACRO_ERROR;
268 } else if (status == STAT_DONE) {
269 *msg = "";
270 *result = *--StackP;
271 + saveContext(continuation);
272 FreeRestartData(continuation);
273 restoreContext(&oldContext);
274 return MACRO_DONE;
275 @@ -580,35 +653,12 @@ int ContinueMacro(RestartData *continuat
276 */
277 void RunMacroAsSubrCall(Program *prog)
278 {
279 - Symbol *s;
280 - static DataValue noValue = {NO_TAG, {0}};
281 -
282 - /* See subroutine "callSubroutine" for a description of the stack frame
283 - for a subroutine call */
284 - StackP->tag = NO_TAG;
285 - StackP->val.inst = PC; /* return PC */
286 - StackP++;
287 -
288 - StackP->tag = NO_TAG;
289 - StackP->val.dataval = FrameP; /* old FrameP */
290 - StackP++;
291 -
292 - StackP->tag = NO_TAG; /* nArgs */
293 - StackP->val.n = 0;
294 - StackP++;
295 -
296 - *(StackP++) = noValue; /* cached arg array */
297 -
298 - FrameP = StackP;
299 - PC = prog->code;
300 - for (s = prog->localSymList; s != NULL; s = s->next) {
301 - FP_GET_SYM_VAL(FrameP, s) = noValue;
302 - StackP++;
303 - }
304 + setupFrame(&FrameP, &StackP, &PC, prog, 0, NULL);
305 }
307 void FreeRestartData(RestartData *context)
308 {
309 + rewindStack(context->pc, context->frameP, context->stackP);
310 XtFree((char *)context->stack);
311 XtFree((char *)context);
312 }
313 @@ -1949,27 +1999,10 @@ static int callSubroutine(void)
314 ** values which are already there.
315 */
316 if (sym->type == MACRO_FUNCTION_SYM) {
317 - StackP->tag = NO_TAG; /* return PC */
318 - StackP->val.inst = PC;
319 - StackP++;
320 -
321 - StackP->tag = NO_TAG; /* old FrameP */
322 - StackP->val.dataval = FrameP;
323 - StackP++;
324 -
325 - StackP->tag = NO_TAG; /* nArgs */
326 - StackP->val.n = nArgs;
327 - StackP++;
328 -
329 - *(StackP++) = noValue; /* cached arg array */
330 -
331 - FrameP = StackP;
332 - prog = sym->value.val.prog;
333 - PC = prog->code;
334 - for (s = prog->localSymList; s != NULL; s = s->next) {
335 - FP_GET_SYM_VAL(FrameP, s) = noValue;
336 - StackP++;
337 - }
338 + prog = sym->value.val.prog;
339 + prog->refcount++;
340 + /* -nArgs means 'arguments are on stack' */
341 + setupFrame(&FrameP, &StackP, &PC, prog, -nArgs, NULL);
342 return STAT_OK;
343 }
345 @@ -2051,8 +2084,6 @@ static int returnValOrNone(int valOnStac
346 {
347 DataValue retVal;
348 static DataValue noValue = {NO_TAG, {0}};
349 - DataValue *newFrameP;
350 - int nArgs;
352 DISASM_RT(PC-1, 1);
353 STACKDUMP(StackP - FrameP + FP_GET_ARG_COUNT(FrameP) + FP_TO_ARGS_DIST, 3);
354 @@ -2062,17 +2093,8 @@ static int returnValOrNone(int valOnStac
355 POP(retVal);
356 }
358 - /* get stored return information */
359 - nArgs = FP_GET_ARG_COUNT(FrameP);
360 - newFrameP = FP_GET_OLD_FP(FrameP);
361 - PC = FP_GET_RET_PC(FrameP);
362 -
363 - /* pop past local variables */
364 - StackP = FrameP;
365 - /* pop past function arguments */
366 - StackP -= (FP_TO_ARGS_DIST + nArgs);
367 - FrameP = newFrameP;
368 -
369 + PC = rewindFrame(&FrameP, &StackP);
370 +
371 /* push returned value, if requsted */
372 if (PC == NULL) {
373 if (valOnStack) {
374 @@ -3051,6 +3073,7 @@ static void stackdump(int n, int extra)
375 case FP_ARG_COUNT_INDEX: pos = "NArgs"; break; /* number of arguments */
376 case FP_OLD_FP_INDEX: pos = "OldFP"; break;
377 case FP_RET_PC_INDEX: pos = "RetPC"; break;
378 + case FP_PROG_INDEX: pos = "Prog"; break;
379 default:
380 if (offset < -FP_TO_ARGS_DIST && offset >= -FP_TO_ARGS_DIST - nArgs) {
381 sprintf(pos = buffer, STACK_DUMP_ARG_PREFIX "%d",
382 diff --quilt old/source/interpret.h new/source/interpret.h
383 --- old/source/interpret.h
384 +++ new/source/interpret.h
385 @@ -105,6 +105,7 @@ typedef struct SymbolRec {
386 typedef struct ProgramTag {
387 Symbol *localSymList;
388 Inst *code;
389 + unsigned refcount;
390 } Program;
392 /* Information needed to re-start a preempted macro */