Cleaner handling of errors in '__close' metamethods

Instead of protecting each individual metamethod call, protect the
entire call to 'luaF_close'.

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 */
@@ -770,6 +768,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
 ** its stack level in case of errors.
@@ -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'");
-  }
-  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 */
+  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 */
   }
-  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);
         }