Skip to content

Commit 4868db6

Browse files
runrevmarkrunrevmark
committed
[[ Bug 16947 ]] Fix memory leak when making recursive calls with refs
This patch fixes a memory leak which can occur when a handler calls itself and it uses reference parameters. In order to fix the issue, calling of handlers has been refactored in two ways. First of all the MCComref and MCFuncref syntax node classes have been moved to keywords.h and are now unified by both being implemented via aggregating an MCHandref class (this means that they can share almost all their implementation, and still retain distinct MCStatement and MCExpression syntax node base classes). Secondly, the calling of handlers has now been split into three parts: setup, teardown and exec. The setup phase evaluates parameters, using a pre-allocated vector of containers provided by the caller; the teardown phase frees any resources allocated by setup. Critically this has reduced the remit of the main handler exec function which means that MCComref, MCFuncref *and* MCDispatchCmd can now share most of their code. The actual memory leak is fixed by moving ownership of any containers required to invoke a handler to the caller of setup and teardown phases. The number of containers required is precalculated after parsing the expression list (as it is just the number of expressions which have a root variable ref), and space for those that are required when the call is invoked is allocated before setup is called. The only difference between MCComref/MCFuncref and MCDispatchCmd is now that the former use MCKeywordsExecCommandOrFunction to execute the call whereas MCDispatchCmd uses MCEngineExecDispatch, directly reflecting their key difference. Additionally, MCParameter::eval or MCParameter::eval_ctxt have been changed to only ever evaluate the expression bound to the parameter rather than evaluating any set argument if present. Places where these methods were being used to evaluate the set argument value have been updated to use eval_argument and eval_argument_ctxt respectively.
1 parent a57c886 commit 4868db6

File tree

20 files changed

+541
-298
lines changed

20 files changed

+541
-298
lines changed

‎docs/notes/bugfix-16947.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
# Fix memory leak when calling self-recursive handlers having reference parameters
2+

‎engine/src/cmds.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -997,7 +997,13 @@ class MCDispatchCmd: public MCStatement
997997
MCExpression *message;
998998
MCChunk *target;
999999
MCParameter *params;
1000-
bool is_function;
1000+
struct
1001+
{
1002+
/* The container count is the number of containers needed to execute
1003+
* the command. It is calculated after parsing the node. */
1004+
unsigned container_count : 16;
1005+
bool is_function : 1;
1006+
};
10011007

10021008
public:
10031009
MCDispatchCmd(void)
@@ -1006,6 +1012,7 @@ class MCDispatchCmd: public MCStatement
10061012
target = NULL;
10071013
params = NULL;
10081014
is_function = false;
1015+
container_count = 0;
10091016
}
10101017
~MCDispatchCmd(void);
10111018

‎engine/src/cmdse.cpp‎

Lines changed: 36 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,14 @@ Parse_stat MCDispatchCmd::parse(MCScriptPoint& sp)
463463
return PS_ERROR;
464464
}
465465
}
466-
466+
467+
/* If there are any parameters then compute the number of containers needed
468+
* to execute the command. */
469+
if (params != nullptr)
470+
{
471+
container_count = params->count_containers();
472+
}
473+
467474
return PS_NORMAL;
468475
}
469476

@@ -488,51 +495,36 @@ void MCDispatchCmd::exec_ctxt(MCExecContext &ctxt)
488495
}
489496
else
490497
t_target_ptr = nil;
491-
492-
// Evaluate the parameter list
493-
bool t_success, t_can_debug;
494-
MCParameter *tptr = params;
495-
while (tptr != NULL)
496-
{
497-
// AL-2014-08-20: [[ ArrayElementRefParams ]] Use containers for potential reference parameters
498-
MCAutoPointer<MCContainer> t_container = new (nothrow) MCContainer;
499-
if (tptr -> evalcontainer(ctxt, **t_container))
500-
tptr -> set_argument_container(t_container.Release());
501-
else
502-
{
503-
MCExecValue t_value;
504-
tptr -> clear_argument();
505-
506-
do
507-
{
508-
if (!(t_success = tptr->eval_ctxt(ctxt, t_value)))
509-
t_can_debug = MCB_error(ctxt, line, pos, EE_STATEMENT_BADPARAM);
510-
ctxt.IgnoreLastError();
511-
}
512-
while (!t_success && t_can_debug && (MCtrace || MCnbreakpoints) && !MCtrylock && !MClockerrors);
513-
514-
if (!t_success)
515-
{
516-
ctxt . LegacyThrow(EE_STATEMENT_BADPARAM);
517-
return;
518-
}
519-
520-
tptr->give_exec_argument(t_value);
521-
}
522-
523-
tptr = tptr->getnext();
524-
}
525-
526-
ctxt . SetLineAndPos(line, pos);
527-
MCEngineExecDispatch(ctxt, is_function ? HT_FUNCTION : HT_MESSAGE, *t_message, t_target_ptr, params);
528498

529-
// AL-2014-09-17: [[ Bug 13465 ]] Clear parameters after executing dispatch
530-
tptr = params;
531-
while (tptr != NULL)
532-
{
533-
tptr -> clear_argument();
534-
tptr = tptr->getnext();
499+
/* Attempt to allocate the number of containers needed for the call. */
500+
MCAutoPointer<MCContainer[]> t_containers = new MCContainer[container_count];
501+
if (!t_containers)
502+
{
503+
ctxt.LegacyThrow(EE_NO_MEMORY);
504+
return;
535505
}
506+
507+
/* If the argument list is successfully evaluated, then do the dispatch. */
508+
if (MCKeywordsExecSetupCommandOrFunction(ctxt,
509+
params,
510+
*t_containers,
511+
line,
512+
pos,
513+
is_function))
514+
{
515+
if (!ctxt.HasError())
516+
{
517+
ctxt.SetLineAndPos(line, pos);
518+
MCEngineExecDispatch(ctxt,
519+
is_function ? HT_FUNCTION : HT_MESSAGE,
520+
*t_message,
521+
t_target_ptr,
522+
params);
523+
}
524+
}
525+
526+
/* Clean up the evaluated argument list */
527+
MCKeywordsExecTeardownCommandOrFunction(params);
536528
}
537529

538530
Parse_stat MCMessage::parse(MCScriptPoint &sp)

‎engine/src/exec-engine.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1308,7 +1308,7 @@ static void MCEngineSendOrCall(MCExecContext& ctxt, MCStringRef p_script, MCObje
13081308
MCAutoValueRef t_value;
13091309
MCAutoStringRef t_value_string;
13101310

1311-
if (!t_param_ptr->eval(ctxt, &t_value) ||
1311+
if (!t_param_ptr->eval_argument(ctxt, &t_value) ||
13121312
!ctxt . ConvertToString(*t_value, &t_value_string) ||
13131313
!MCListAppend(*t_param_list, *t_value_string))
13141314
goto cleanup;

‎engine/src/exec-extension.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ Exec_stat MCEngineHandleLibraryMessage(MCNameRef p_message, MCParameter *p_param
448448
if (MCHandlerTypeInfoGetParameterMode(t_signature, i) != kMCHandlerTypeFieldModeOut)
449449
{
450450
MCValueRef t_value;
451-
if (!t_param -> eval(*MCECptr, t_value))
451+
if (!t_param -> eval_argument(*MCECptr, t_value))
452452
{
453453
t_success = false;
454454
break;
@@ -467,6 +467,7 @@ Exec_stat MCEngineHandleLibraryMessage(MCNameRef p_message, MCParameter *p_param
467467

468468
if (!t_arguments . Push(t_value))
469469
{
470+
MCValueRelease(t_value);
470471
t_success = false;
471472
break;
472473
}

‎engine/src/exec-keywords.cpp‎

Lines changed: 67 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -132,43 +132,70 @@ void MCKeywordsExecResolveCommandOrFunction(MCExecContext& ctxt, MCNameRef p_nam
132132
r_handler = t_resolved_handler;
133133
}
134134

135+
bool MCKeywordsExecSetupCommandOrFunction(MCExecContext& ctxt, MCParameter *params, MCContainer *containers, uint2 line, uint2 pos, bool is_function)
136+
{
137+
MCParameter *tptr = params;
138+
uindex_t t_container_index = 0;
139+
while (tptr != NULL)
140+
{
141+
/* If the parameter evaluates as a container, then place the result
142+
* into the next available container slot and bump the index; otherwise
143+
* evaluate as an expression. */
144+
if (tptr -> evalcontainer(ctxt, containers[t_container_index]))
145+
{
146+
tptr -> set_argument_container(&containers[t_container_index]);
147+
t_container_index += 1;
148+
}
149+
else
150+
{
151+
MCExecValue t_value;
152+
153+
if (!ctxt.TryToEvaluateParameter(tptr,
154+
line,
155+
pos,
156+
is_function ? EE_FUNCTION_BADSOURCE : EE_STATEMENT_BADPARAM,
157+
t_value))
158+
{
159+
return false;
160+
}
161+
162+
tptr -> clear_argument();
163+
tptr->give_exec_argument(t_value);
164+
}
165+
166+
tptr = tptr->getnext();
167+
}
168+
169+
return true;
170+
}
171+
172+
void MCKeywordsExecTeardownCommandOrFunction(MCParameter *params)
173+
{
174+
// AL-2014-09-17: [[ Bug 13465 ]] Clear parameters after executing dispatch
175+
MCParameter *tptr = params;
176+
while (tptr != NULL)
177+
{
178+
tptr -> clear_argument();
179+
tptr = tptr->getnext();
180+
}
181+
}
182+
135183
void MCKeywordsExecCommandOrFunction(MCExecContext& ctxt, MCHandler *handler, MCParameter *params, MCNameRef name, uint2 line, uint2 pos, bool global_handler, bool is_function)
136184
{
137185
if (MCscreen->abortkey())
138186
{
139187
ctxt . LegacyThrow(EE_HANDLER_ABORT);
140188
return;
141189
}
142-
190+
143191
if (is_function)
144192
MCexitall = False;
145-
146-
// Go through all the parameters to the function, if they are not variables, clear their current value. Each parameter stores an expression
147-
// which allows its value to be re-evaluated in a given context. Re-evaluate each in the context of ep and set it to the new value.
148-
// As the ep should contain the context of the caller at this point, the expression should be evaluated in that context.
193+
149194
Exec_stat stat;
150-
MCParameter *tptr = params;
151-
while (tptr != NULL)
152-
{
153-
// AL-2014-08-20: [[ ArrayElementRefParams ]] Use containers for potential reference parameters
154-
MCAutoPointer<MCContainer> t_container = new (nothrow) MCContainer;
155-
if (tptr -> evalcontainer(ctxt, **t_container))
156-
tptr -> set_argument_container(t_container.Release());
157-
else
158-
{
159-
tptr -> clear_argument();
160-
MCExecValue t_value;
161-
if (!ctxt . TryToEvaluateParameter(tptr, line, pos, is_function ? EE_FUNCTION_BADSOURCE : EE_STATEMENT_BADPARAM, t_value))
162-
return;
163-
tptr->give_exec_argument(t_value);
164-
}
165-
166-
tptr = tptr->getnext();
167-
}
168-
MCObject *p = ctxt . GetObject();
195+
stat = ES_NOT_HANDLED;
196+
MCObject *p = ctxt . GetObject();
169197
MCExecContext *oldctxt = MCECptr;
170198
MCECptr = &ctxt;
171-
stat = ES_NOT_HANDLED;
172199
Boolean added = False;
173200
if (MCnexecutioncontexts < MAX_CONTEXTS)
174201
{
@@ -218,6 +245,21 @@ void MCKeywordsExecCommandOrFunction(MCExecContext& ctxt, MCHandler *handler, MC
218245
stat = p->handle(HT_FUNCTION, name, params, p);
219246
if (oldstat == ES_PASS && stat == ES_NOT_HANDLED)
220247
stat = ES_PASS;
248+
249+
/* The following clause was pulled in from MCFuncref::eval_ctxt
250+
* it is not quite clear why this code path is different from
251+
* commands; however without the clause a test failure occurs
252+
* in 'TestExtensionLibraryHandlerCallErrors'. */
253+
// MW-2007-08-09: [[ Bug 5705 ]] Throws inside private functions don't trigger an
254+
// exception.
255+
if (!global_handler &&
256+
stat != ES_NORMAL &&
257+
stat != ES_PASS &&
258+
stat != ES_EXIT_HANDLER)
259+
{
260+
MCeerror->add(EE_FUNCTION_BADFUNCTION, line, pos, name);
261+
stat = ES_ERROR;
262+
}
221263
}
222264
else
223265
{
@@ -265,14 +307,6 @@ void MCKeywordsExecCommandOrFunction(MCExecContext& ctxt, MCHandler *handler, MC
265307
ctxt . SetExecStat(stat);
266308
else
267309
ctxt . SetExecStat(ES_NORMAL);
268-
269-
// AL-2014-09-17: [[ Bug 13465 ]] Clear parameters after executing command/function
270-
tptr = params;
271-
while (tptr != NULL)
272-
{
273-
tptr -> clear_argument();
274-
tptr = tptr->getnext();
275-
}
276310
}
277311

278312
////////////////////////////////////////////////////////////////////////////////

‎engine/src/exec.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ along with LiveCode. If not see <http://www.gnu.org/licenses/>. */
3232
#include "variable.h"
3333
#include "handler.h"
3434
#include "hndlrlst.h"
35+
#include "keywords.h"
3536

3637
#include "osspec.h"
3738

‎engine/src/exec.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1802,6 +1802,8 @@ void MCKeywordsExecPass(MCExecContext& ctxt);
18021802
void MCKeywordsExecPassAll(MCExecContext& ctxt);
18031803
void MCKeywordsExecThrow(MCExecContext& ctxt, MCStringRef string);
18041804
void MCKeywordsExecResolveCommandOrFunction(MCExecContext& ctxt, MCNameRef p_name, bool is_function, MCHandler*& r_handler);
1805+
bool MCKeywordsExecSetupCommandOrFunction(MCExecContext& ctxt, MCParameter *params, MCContainer *containers, uint2 line, uint2 pos, bool is_function);
1806+
void MCKeywordsExecTeardownCommandOrFunction(MCParameter *params);
18051807
void MCKeywordsExecCommandOrFunction(MCExecContext& ctxt, MCHandler *handler, MCParameter *params, MCNameRef name, uint2 line, uint2 pos, bool platform_message, bool is_function);
18061808

18071809
////////////////////////////////////////////////////////////////////////////////

‎engine/src/express.cpp‎

Lines changed: 0 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -440,93 +440,3 @@ void MCExpression::initpoint(MCScriptPoint &sp)
440440
}
441441

442442
////////////////////////////////////////////////////////////////////////////////
443-
444-
MCFuncref::MCFuncref(MCNameRef inname)
445-
: name(inname)
446-
{
447-
handler = nil;
448-
params = NULL;
449-
resolved = false;
450-
global_handler = false;
451-
}
452-
453-
MCFuncref::~MCFuncref()
454-
{
455-
while (params != NULL)
456-
{
457-
MCParameter *tmp = params;
458-
params = params->getnext();
459-
delete tmp;
460-
}
461-
}
462-
463-
Parse_stat MCFuncref::parse(MCScriptPoint &sp, Boolean the)
464-
{
465-
initpoint(sp);
466-
if (getparams(sp, &params) != PS_NORMAL)
467-
{
468-
MCperror->add(PE_FUNCTION_BADPARAMS, sp);
469-
return PS_ERROR;
470-
}
471-
472-
if (MCIsGlobalHandler(*name))
473-
{
474-
global_handler = true;
475-
resolved = true;
476-
}
477-
478-
return PS_NORMAL;
479-
}
480-
481-
void MCFuncref::eval_ctxt(MCExecContext& ctxt, MCExecValue& r_value)
482-
{
483-
if (!resolved)
484-
{
485-
MCKeywordsExecResolveCommandOrFunction(ctxt, *name, true, handler);
486-
resolved = true;
487-
}
488-
489-
MCKeywordsExecCommandOrFunction(ctxt, handler, params, *name, line, pos, global_handler, true);
490-
491-
Exec_stat stat = ctxt . GetExecStat();
492-
493-
// MW-2007-08-09: [[ Bug 5705 ]] Throws inside private functions don't trigger an
494-
// exception.
495-
if (stat != ES_NORMAL && stat != ES_PASS && stat != ES_EXIT_HANDLER)
496-
{
497-
ctxt . LegacyThrow(EE_FUNCTION_BADFUNCTION, *name);
498-
return;
499-
}
500-
501-
if (MCresultmode == kMCExecResultModeReturn)
502-
{
503-
if (MCresult->eval(ctxt, r_value . valueref_value))
504-
{
505-
r_value . type = kMCExecValueTypeValueRef;
506-
return;
507-
}
508-
}
509-
else if (MCresultmode == kMCExecResultModeReturnValue)
510-
{
511-
// Our return value is MCresult, and 'the result' gets set to empty.
512-
if (MCresult->eval(ctxt, r_value . valueref_value))
513-
{
514-
r_value . type = kMCExecValueTypeValueRef;
515-
ctxt.SetTheResultToEmpty();
516-
return;
517-
}
518-
}
519-
else if (MCresultmode == kMCExecResultModeReturnError)
520-
{
521-
// Our return value is empty, and 'the result' remains as it is.
522-
MCExecTypeSetValueRef(r_value, MCValueRetain(kMCEmptyString));
523-
524-
// Make sure we reset the 'return mode' to default.
525-
MCresultmode = kMCExecResultModeReturn;
526-
return;
527-
}
528-
529-
ctxt . Throw();
530-
}
531-
532-
////////////////////////////////////////////////////////////////////////////////

0 commit comments

Comments
 (0)