From 7af27ef59da4051914d93d8b63efac663b64765a Mon Sep 17 00:00:00 2001 From: Roberto Ierusalimschy Date: Mon, 28 Dec 2020 11:40:30 -0300 Subject: [PATCH] Cleaner handling of errors in '__close' metamethods Instead of protecting each individual metamethod call, protect the entire call to 'luaF_close'. --- lapi.c | 2 +- ldo.c | 59 +++++++++++++++++++++++++++++++++++--------- ldo.h | 1 + lfunc.c | 75 +++++++++++++++++--------------------------------------- lfunc.h | 6 ++--- lstate.c | 10 ++++---- lvm.c | 2 +- 7 files changed, 80 insertions(+), 75 deletions(-) diff --git a/lapi.c b/lapi.c index 03e756d6..00e95a11 100644 --- a/lapi.c +++ b/lapi.c @@ -188,7 +188,7 @@ LUA_API void lua_settop (lua_State *L, int idx) { diff = idx + 1; /* will "subtract" index (as it is negative) */ } if (diff < 0 && hastocloseCfunc(ci->nresults)) - luaF_close(L, L->top + diff, LUA_OK); + luaF_close(L, L->top + diff, CLOSEKTOP); L->top += diff; /* correct top only after closing any upvalue */ lua_unlock(L); } diff --git a/ldo.c b/ldo.c index 4b55c31c..59391f7b 100644 --- a/ldo.c +++ b/ldo.c @@ -98,11 +98,12 @@ void luaD_seterrorobj (lua_State *L, int errcode, StkId oldtop) { setsvalue2s(L, oldtop, luaS_newliteral(L, "error in error handling")); break; } - case CLOSEPROTECT: { + case LUA_OK: { /* special case only for closing upvalues */ setnilvalue(s2v(oldtop)); /* no error message */ break; } default: { + lua_assert(errcode >= LUA_ERRRUN); /* real error */ setobjs2s(L, oldtop, L->top - 1); /* error message on current top */ break; } @@ -118,7 +119,7 @@ l_noret luaD_throw (lua_State *L, int errcode) { } else { /* thread has no error handler */ global_State *g = G(L); - errcode = luaF_close(L, L->stack, errcode); /* close all upvalues */ + errcode = luaD_closeprotected(L, 0, errcode); /* close all upvalues */ L->status = cast_byte(errcode); /* mark it as dead */ if (g->mainthread->errorJmp) { /* main thread has a handler? */ setobjs2s(L, g->mainthread->top++, L->top - 1); /* copy error obj. */ @@ -409,7 +410,7 @@ static void moveresults (lua_State *L, StkId res, int nres, int wanted) { default: /* multiple results (or to-be-closed variables) */ if (hastocloseCfunc(wanted)) { /* to-be-closed variables? */ ptrdiff_t savedres = savestack(L, res); - luaF_close(L, res, LUA_OK); /* may change the stack */ + luaF_close(L, res, CLOSEKTOP); /* may change the stack */ res = restorestack(L, savedres); wanted = codeNresults(wanted); /* correct value */ if (wanted == LUA_MULTRET) @@ -636,16 +637,13 @@ static CallInfo *findpcall (lua_State *L) { ** 'luaD_pcall'. If there is no recover point, returns zero. */ static int recover (lua_State *L, int status) { - StkId oldtop; CallInfo *ci = findpcall(L); if (ci == NULL) return 0; /* no recovery point */ /* "finish" luaD_pcall */ - oldtop = restorestack(L, ci->u2.funcidx); L->ci = ci; L->allowhook = getoah(ci->callstatus); /* restore original 'allowhook' */ - status = luaF_close(L, oldtop, status); /* may change the stack */ - oldtop = restorestack(L, ci->u2.funcidx); - luaD_seterrorobj(L, status, oldtop); + status = luaD_closeprotected(L, ci->u2.funcidx, status); + luaD_seterrorobj(L, status, restorestack(L, ci->u2.funcidx)); luaD_shrinkstack(L); /* restore stack size in case of overflow */ L->errfunc = ci->u.c.old_errfunc; return 1; /* continue running the coroutine */ @@ -769,6 +767,45 @@ LUA_API int lua_yieldk (lua_State *L, int nresults, lua_KContext ctx, } +/* +** Auxiliary structure to call 'luaF_close' in protected mode. +*/ +struct CloseP { + StkId level; + int status; +}; + + +/* +** Auxiliary function to call 'luaF_close' in protected mode. +*/ +static void closepaux (lua_State *L, void *ud) { + struct CloseP *pcl = cast(struct CloseP *, ud); + luaF_close(L, pcl->level, pcl->status); +} + + +/* +** Calls 'luaF_close' in protected mode. Return the original status +** or, in case of errors, the new status. +*/ +int luaD_closeprotected (lua_State *L, ptrdiff_t level, int status) { + CallInfo *old_ci = L->ci; + lu_byte old_allowhooks = L->allowhook; + for (;;) { /* keep closing upvalues until no more errors */ + struct CloseP pcl; + pcl.level = restorestack(L, level); pcl.status = status; + status = luaD_rawrunprotected(L, &closepaux, &pcl); + if (likely(status == LUA_OK)) /* no more errors? */ + return pcl.status; + else { /* an error occurred; restore saved state and repeat */ + L->ci = old_ci; + L->allowhook = old_allowhooks; + } + } +} + + /* ** Call the C function 'func' in protected mode, restoring basic ** thread information ('allowhook', etc.) and in particular @@ -783,12 +820,10 @@ int luaD_pcall (lua_State *L, Pfunc func, void *u, L->errfunc = ef; status = luaD_rawrunprotected(L, func, u); if (unlikely(status != LUA_OK)) { /* an error occurred? */ - StkId oldtop = restorestack(L, old_top); L->ci = old_ci; L->allowhook = old_allowhooks; - status = luaF_close(L, oldtop, status); - oldtop = restorestack(L, old_top); /* previous call may change stack */ - luaD_seterrorobj(L, status, oldtop); + status = luaD_closeprotected(L, old_top, status); + luaD_seterrorobj(L, status, restorestack(L, old_top)); luaD_shrinkstack(L); /* restore stack size in case of overflow */ } L->errfunc = old_errfunc; diff --git a/ldo.h b/ldo.h index 4d30d072..c7721d62 100644 --- a/ldo.h +++ b/ldo.h @@ -63,6 +63,7 @@ LUAI_FUNC CallInfo *luaD_precall (lua_State *L, StkId func, int nResults); LUAI_FUNC void luaD_call (lua_State *L, StkId func, int nResults); LUAI_FUNC void luaD_callnoyield (lua_State *L, StkId func, int nResults); LUAI_FUNC void luaD_tryfuncTM (lua_State *L, StkId func); +LUAI_FUNC int luaD_closeprotected (lua_State *L, ptrdiff_t level, int status); LUAI_FUNC int luaD_pcall (lua_State *L, Pfunc func, void *u, ptrdiff_t oldtop, ptrdiff_t ef); LUAI_FUNC void luaD_poscall (lua_State *L, CallInfo *ci, int nres); diff --git a/lfunc.c b/lfunc.c index bfbf270b..ae68487c 100644 --- a/lfunc.c +++ b/lfunc.c @@ -100,12 +100,6 @@ UpVal *luaF_findupval (lua_State *L, StkId level) { } -static void callclose (lua_State *L, void *ud) { - UNUSED(ud); - luaD_callnoyield(L, L->top - 3, 0); -} - - /* ** Prepare closing method plus its arguments for object 'obj' with ** error message 'err'. (This function assumes EXTRA_STACK.) @@ -136,40 +130,25 @@ static void varerror (lua_State *L, StkId level, const char *msg) { /* -** Prepare and call a closing method. If status is OK, code is still -** inside the original protected call, and so any error will be handled -** there. Otherwise, a previous error already activated the original -** protected call, and so the call to the closing method must be -** protected here. (A status == CLOSEPROTECT behaves like a previous -** error, to also run the closing method in protected mode). -** If status is OK, the call to the closing method will be pushed -** at the top of the stack. Otherwise, values are pushed after -** the 'level' of the upvalue being closed, as everything after -** that won't be used again. +** Prepare and call a closing method. +** If status is CLOSEKTOP, the call to the closing method will be pushed +** at the top of the stack. Otherwise, values can be pushed right after +** the 'level' of the upvalue being closed, as everything after that +** won't be used again. */ -static int callclosemth (lua_State *L, StkId level, int status) { +static void callclosemth (lua_State *L, StkId level, int status) { TValue *uv = s2v(level); /* value being closed */ - if (likely(status == LUA_OK)) { - if (prepclosingmethod(L, uv, &G(L)->nilvalue)) /* something to call? */ - callclose(L, NULL); /* call closing method */ - else if (!l_isfalse(uv)) /* non-closable non-false value? */ - varerror(L, level, "attempt to close non-closable variable '%s'"); + TValue *errobj; + if (status == CLOSEKTOP) + errobj = &G(L)->nilvalue; /* error object is nil */ + else { /* 'luaD_seterrorobj' will set top to level + 2 */ + errobj = s2v(level + 1); /* error object goes after 'uv' */ + luaD_seterrorobj(L, status, level + 1); /* set error object */ } - else { /* must close the object in protected mode */ - ptrdiff_t oldtop; - level++; /* space for error message */ - oldtop = savestack(L, level + 1); /* top will be after that */ - luaD_seterrorobj(L, status, level); /* set error message */ - if (prepclosingmethod(L, uv, s2v(level))) { /* something to call? */ - int newstatus = luaD_pcall(L, callclose, NULL, oldtop, 0); - if (newstatus != LUA_OK) /* new error? */ - status = newstatus; /* this will be the error now */ - else /* leave original error (or nil) on top */ - L->top = restorestack(L, oldtop); - } - /* else no metamethod; ignore this case and keep original error */ - } - return status; + if (prepclosingmethod(L, uv, errobj)) /* something to call? */ + luaD_callnoyield(L, L->top - 3, 0); /* call method */ + else if (!l_isfalse(uv)) /* non-closable non-false value? */ + varerror(L, level, "attempt to close non-closable variable '%s'"); } @@ -201,7 +180,7 @@ void luaF_newtbcupval (lua_State *L, StkId level) { luaD_seterrorobj(L, LUA_ERRMEM, level + 1); /* save error message */ /* next call must succeed, as object is closable */ prepclosingmethod(L, s2v(level), s2v(level + 1)); - callclose(L, NULL); /* call closing method */ + luaD_callnoyield(L, L->top - 3, 0); /* call method */ luaD_throw(L, LUA_ERRMEM); /* throw memory error */ } } @@ -217,19 +196,11 @@ void luaF_unlinkupval (UpVal *uv) { /* -** Close all upvalues up to the given stack level. 'status' indicates -** how/why the function was called: -** - LUA_OK: regular code exiting the scope of a variable; may raise -** an error due to errors in __close metamethods; -** - CLOSEPROTECT: finishing a thread; run all metamethods in protected -** mode; -** - NOCLOSINGMETH: close upvalues without running __close metamethods; -** - other values: error status from previous errors, to be propagated. -** -** Returns the resulting status, either the original status or an error -** in a closing method. +** Close all upvalues up to the given stack level. A 'status' equal +** to NOCLOSINGMETH closes upvalues without running any __close +** metamethods. */ -int luaF_close (lua_State *L, StkId level, int status) { +void luaF_close (lua_State *L, StkId level, int status) { UpVal *uv; StkId upl; /* stack index pointed by 'uv' */ while ((uv = L->openupval) != NULL && (upl = uplevel(uv)) >= level) { @@ -243,13 +214,11 @@ int luaF_close (lua_State *L, StkId level, int status) { luaC_barrier(L, uv, slot); } if (uv->tbc && status != NOCLOSINGMETH) { - /* must run closing method, which may change the stack */ ptrdiff_t levelrel = savestack(L, level); - status = callclosemth(L, upl, status); + callclosemth(L, upl, status); /* may change the stack */ level = restorestack(L, levelrel); } } - return status; } diff --git a/lfunc.h b/lfunc.h index 8d6f965c..40de4636 100644 --- a/lfunc.h +++ b/lfunc.h @@ -49,8 +49,8 @@ /* close upvalues without running their closing methods */ #define NOCLOSINGMETH (-1) -/* close upvalues running all closing methods in protected mode */ -#define CLOSEPROTECT (-2) +/* special status to close upvalues preserving the top of the stack */ +#define CLOSEKTOP (-2) LUAI_FUNC Proto *luaF_newproto (lua_State *L); @@ -59,7 +59,7 @@ LUAI_FUNC LClosure *luaF_newLclosure (lua_State *L, int nupvals); LUAI_FUNC void luaF_initupvals (lua_State *L, LClosure *cl); LUAI_FUNC UpVal *luaF_findupval (lua_State *L, StkId level); LUAI_FUNC void luaF_newtbcupval (lua_State *L, StkId level); -LUAI_FUNC int luaF_close (lua_State *L, StkId level, int status); +LUAI_FUNC void luaF_close (lua_State *L, StkId level, int status); LUAI_FUNC void luaF_unlinkupval (UpVal *uv); LUAI_FUNC void luaF_freeproto (lua_State *L, Proto *f); LUAI_FUNC const char *luaF_getlocalname (const Proto *func, int local_number, diff --git a/lstate.c b/lstate.c index 96187c66..e01a7e7b 100644 --- a/lstate.c +++ b/lstate.c @@ -268,7 +268,7 @@ static void preinit_thread (lua_State *L, global_State *g) { static void close_state (lua_State *L) { global_State *g = G(L); - luaF_close(L, L->stack, CLOSEPROTECT); /* close all upvalues */ + luaD_closeprotected(L, 0, LUA_OK); /* close all upvalues */ luaC_freeallobjects(L); /* collect all objects */ if (ttisnil(&g->nilvalue)) /* closing a fully built state? */ luai_userstateclose(L); @@ -329,10 +329,10 @@ int lua_resetthread (lua_State *L) { setnilvalue(s2v(L->stack)); /* 'function' entry for basic 'ci' */ ci->func = L->stack; ci->callstatus = CIST_C; - if (status == LUA_OK || status == LUA_YIELD) - status = CLOSEPROTECT; /* run closing methods in protected mode */ - status = luaF_close(L, L->stack, status); - if (status != CLOSEPROTECT) /* errors? */ + if (status == LUA_YIELD) + status = LUA_OK; + status = luaD_closeprotected(L, 0, status); + if (status != LUA_OK) /* errors? */ luaD_seterrorobj(L, status, L->stack + 1); else { status = LUA_OK; diff --git a/lvm.c b/lvm.c index ccebdbe0..a6f04606 100644 --- a/lvm.c +++ b/lvm.c @@ -1662,7 +1662,7 @@ void luaV_execute (lua_State *L, CallInfo *ci) { if (TESTARG_k(i)) { /* may there be open upvalues? */ if (L->top < ci->top) L->top = ci->top; - luaF_close(L, base, LUA_OK); + luaF_close(L, base, CLOSEKTOP); updatetrap(ci); updatestack(ci); }