CVE-2020-24371: Release of Invalid Pointer or Reference (CWE-763)
value: lgc.c in Lua 5.4.0 mishandles the interaction between barriers and the sweep phase, leading to a memory access violation involving collectgarbage.
CWE description: The product attempts to return a memory resource to the system, but it calls the wrong release function or calls the appropriate release function incorrectly.
CVSS v3 base score: 5.3
Published: 2020-08-17T17:15Z
Repository: https://github.com/lua/lua
Language: C
Files changed: lgc.c, testes/gengc.lua
Commit date: 2020-07-27 10:26:20 -0300
Commit message: Fixed bug: barriers cannot be active during sweep

Barriers cannot be active during sweep, even in generational mode.
(Although gen. mode is not incremental, it can hit a barrier when
deleting a thread and closing its upvalues.)  The colors of objects are
being changed during sweep and, therefore, cannot be trusted.

Diff: diff --git a/lgc.c b/lgc.c
index f7fd7a59a..64d1334b4 100644
--- a/lgc.c
+++ b/lgc.c
@@ -179,18 +179,21 @@ static int iscleared (global_State *g, const GCObject *o) {
   else return iswhite(o);
 }
 
 
 /*
-** barrier that moves collector forward, that is, mark the white object
-** 'v' being pointed by the black object 'o'. (If in sweep phase, clear
-** the black object to white [sweep it] to avoid other barrier calls for
-** this same object.) In the generational mode, 'v' must also become
-** old, if 'o' is old; however, it cannot be changed directly to OLD,
-** because it may still point to non-old objects. So, it is marked as
-** OLD0. In the next cycle it will become OLD1, and in the next it
-** will finally become OLD (regular old).
+** Barrier that moves collector forward, that is, marks the white object
+** 'v' being pointed by the black object 'o'.  In the generational
+** mode, 'v' must also become old, if 'o' is old; however, it cannot
+** be changed directly to OLD, because it may still point to non-old
+** objects. So, it is marked as OLD0. In the next cycle it will become
+** OLD1, and in the next it will finally become OLD (regular old). By
+** then, any object it points to will also be old.  If called in the
+** incremental sweep phase, it clears the black object to white (sweep
+** it) to avoid other barrier calls for this same object. (That cannot
+** be done is generational mode, as its sweep does not distinguish
+** whites from deads.)
 */
 void luaC_barrier_ (lua_State *L, GCObject *o, GCObject *v) {
   global_State *g = G(L);
   lua_assert(isblack(o) && iswhite(v) && !isdead(g, v) && !isdead(g, o));
   if (keepinvariant(g)) {  /* must keep invariant? */
@@ -200,11 +203,12 @@ void luaC_barrier_ (lua_State *L, GCObject *o, GCObject *v) {
       setage(v, G_OLD0);  /* restore generational invariant */
     }
   }
   else {  /* sweep phase */
     lua_assert(issweepphase(g));
-    makewhite(g, o);  /* mark main obj. as white to avoid other barriers */
+    if (g->gckind == KGC_INC)  /* incremental mode? */
+      makewhite(g, o);  /* mark 'o' as white to avoid other barriers */
   }
 }
 
 
 /*
@@ -322,14 +326,19 @@ static lu_mem markbeingfnz (global_State *g) {
   return count;
 }
 
 
 /*
-** Mark all values stored in marked open upvalues from non-marked threads.
-** (Values from marked threads were already marked when traversing the
-** thread.) Remove from the list threads that no longer have upvalues and
-** not-marked threads.
+** For each non-marked thread, simulates a barrier between each open
+** upvalue and its value. (If the thread is collected, the value will be
+** assigned to the upvalue, but then it can be too late for the barrier
+** to act. The "barrier" does not need to check colors: A non-marked
+** thread must be young; upvalues cannot be older than their threads; so
+** any visited upvalue must be young too.) Also removes the thread from
+** the list, as it was already visited. Removes also threads with no
+** upvalues, as they have nothing to be checked. (If the thread gets an
+** upvalue later, it will be linked in the list again.)
 */
 static int remarkupvals (global_State *g) {
   lua_State *thread;
   lua_State **p = &g->twups;
   int work = 0;
@@ -338,13 +347,15 @@ static int remarkupvals (global_State *g) {
     lua_assert(!isblack(thread));  /* threads are never black */
     if (isgray(thread) && thread->openupval != NULL)
       p = &thread->twups;  /* keep marked thread with upvalues in the list */
     else {  /* thread is not marked or without upvalues */
       UpVal *uv;
+      lua_assert(!isold(thread) || thread->openupval == NULL);
       *p = thread->twups;  /* remove thread from the list */
       thread->twups = thread;  /* mark that it is out of list */
       for (uv = thread->openupval; uv != NULL; uv = uv->u.open.next) {
+        lua_assert(getage(uv) <= getage(thread));
         work++;
         if (!iswhite(uv))  /* upvalue already visited? */
           markvalue(g, uv->v);  /* mark its value */
       }
     }
@@ -993,10 +1004,13 @@ static void sweep2old (lua_State *L, GCObject **p) {
 ** Sweep for generational mode. Delete dead objects. (Because the
 ** collection is not incremental, there are no "new white" objects
 ** during the sweep. So, any white object must be dead.) For
 ** non-dead objects, advance their ages and clear the color of
 ** new objects. (Old objects keep their colors.)
+** The ages of G_TOUCHED1 and G_TOUCHED2 objects will advance
+** in 'correctgraylist'. (That function will also remove objects
+** turned white here from any gray list.)
 */
 static GCObject **sweepgen (lua_State *L, global_State *g, GCObject **p,
                             GCObject *limit) {
   static const lu_byte nextage[] = {
     G_SURVIVAL,  /* from G_NEW */
@@ -1053,20 +1067,20 @@ static GCObject **correctgraylist (GCObject **p) {
         GCObject **next = getgclist(curr);
         if (getage(curr) == G_TOUCHED1) {  /* touched in this cycle? */
           lua_assert(isgray(curr));
           gray2black(curr);  /* make it black, for next barrier */
           changeage(curr, G_TOUCHED1, G_TOUCHED2);
-          p = next;  /* go to next element */
+          p = next;  /* keep it in the list and go to next element */
         }
-        else {  /* not touched in this cycle */
+        else {  /* everything else is removed */
+          /* white objects are simply removed */
           if (!iswhite(curr)) {  /* not white? */
             lua_assert(isold(curr));
             if (getage(curr) == G_TOUCHED2)  /* advance from G_TOUCHED2... */
               changeage(curr, G_TOUCHED2, G_OLD);  /* ... to G_OLD */
             gray2black(curr);  /* make it black */
           }
-          /* else, object is white: just remove it from this list */
           *p = *next;  /* remove 'curr' from gray list */
         }
         break;
       }
       case LUA_VTHREAD: {
@@ -1141,10 +1155,11 @@ static void youngcollection (lua_State *L, global_State *g) {
   markold(g, g->allgc, g->reallyold);
   markold(g, g->finobj, g->finobjrold);
   atomic(L);
 
   /* sweep nursery and get a pointer to its last live element */
+  g->gcstate = GCSswpallgc;
   psurvival = sweepgen(L, g, &g->allgc, g->survival);
   /* sweep 'survival' and 'old' */
   sweepgen(L, g, psurvival, g->reallyold);
   g->reallyold = g->old;
   g->old = *psurvival;  /* 'survival' survivals are old now */
@@ -1164,10 +1179,11 @@ static void youngcollection (lua_State *L, global_State *g) {
 }
 
 
 static void atomic2gen (lua_State *L, global_State *g) {
   /* sweep all elements making them old */
+  g->gcstate = GCSswpallgc;
   sweep2old(L, &g->allgc);
   /* everything alive now is old */
   g->reallyold = g->old = g->survival = g->allgc;
 
   /* repeat for 'finobj' lists */
diff --git a/testes/gengc.lua b/testes/gengc.lua
index 4e80dd7ea..7a7dabdd4 100644
--- a/testes/gengc.lua
+++ b/testes/gengc.lua
@@ -55,17 +55,43 @@ do   -- bug in 5.4.0
 
   collectgarbage()   -- make A old
   local obj = {}     -- create a new object
   collectgarbage("step", 0)   -- make it a survival
   assert(not T or T.gcage(obj) == "survival")
-  setmetatable(obj, {__gc = gcf, x = "ok"})   -- create its metatable
+  setmetatable(obj, {__gc = gcf, x = "+"})   -- create its metatable
   assert(not T or T.gcage(getmetatable(obj)) == "new")
   obj = nil   -- clear object
   collectgarbage("step", 0)   -- will call obj's finalizer
 end
 
 
+do   -- another bug in 5.4.0
+  local old = {10}
+  collectgarbage()   -- make 'old' old
+  local co = coroutine.create(
+    function ()
+      local x = nil
+      local f = function ()
+                  return x[1]
+                end
+      x = coroutine.yield(f)
+      coroutine.yield()
+    end
+  )
+  local _, f = coroutine.resume(co)   -- create closure over 'x' in coroutine
+  collectgarbage("step", 0)   -- make upvalue a survival
+  old[1] = {"hello"}    -- 'old' go to grayagain as 'touched1'
+  coroutine.resume(co, {123})     -- its value will be new
+  co = nil
+  collectgarbage("step", 0)   -- hit the barrier
+  assert(f() == 123 and old[1][1] == "hello")
+  collectgarbage("step", 0)   -- run the collector once more
+  -- make sure old[1] was not collected
+  assert(f() == 123 and old[1][1] == "hello")
+end
+
+
 if T == nil then
   (Message or print)('\n >>> testC not active: \z
                              skipping some generational tests <<<\n')
   print 'OK'
   return

Vulnerable code: ** barrier that moves collector forward, that is, mark the white object
** 'v' being pointed by the black object 'o'. (If in sweep phase, clear
** the black object to white [sweep it] to avoid other barrier calls for
** this same object.) In the generational mode, 'v' must also become
** old, if 'o' is old; however, it cannot be changed directly to OLD,
** because it may still point to non-old objects. So, it is marked as
** OLD0. In the next cycle it will become OLD1, and in the next it
** will finally become OLD (regular old).
    makewhite(g, o);  /* mark main obj. as white to avoid other barriers */
** Mark all values stored in marked open upvalues from non-marked threads.
** (Values from marked threads were already marked when traversing the
** thread.) Remove from the list threads that no longer have upvalues and
** not-marked threads.
          p = next;  /* go to next element */
        else {  /* not touched in this cycle */
          /* else, object is white: just remove it from this list */
  setmetatable(obj, {__gc = gcf, x = "ok"})   -- create its metatable
Fixed code: ** Barrier that moves collector forward, that is, marks the white object
** 'v' being pointed by the black object 'o'.  In the generational
** mode, 'v' must also become old, if 'o' is old; however, it cannot
** be changed directly to OLD, because it may still point to non-old
** objects. So, it is marked as OLD0. In the next cycle it will become
** OLD1, and in the next it will finally become OLD (regular old). By
** then, any object it points to will also be old.  If called in the
** incremental sweep phase, it clears the black object to white (sweep
** it) to avoid other barrier calls for this same object. (That cannot
** be done is generational mode, as its sweep does not distinguish
** whites from deads.)
    if (g->gckind == KGC_INC)  /* incremental mode? */
      makewhite(g, o);  /* mark 'o' as white to avoid other barriers */
** For each non-marked thread, simulates a barrier between each open
** upvalue and its value. (If the thread is collected, the value will be
** assigned to the upvalue, but then it can be too late for the barrier
** to act. The "barrier" does not need to check colors: A non-marked
** thread must be young; upvalues cannot be older than their threads; so
** any visited upvalue must be young too.) Also removes the thread from
** the list, as it was already visited. Removes also threads with no
** upvalues, as they have nothing to be checked. (If the thread gets an
** upvalue later, it will be linked in the list again.)
      lua_assert(!isold(thread) || thread->openupval == NULL);
        lua_assert(getage(uv) <= getage(thread));
** The ages of G_TOUCHED1 and G_TOUCHED2 objects will advance
** in 'correctgraylist'. (That function will also remove objects
** turned white here from any gray list.)
          p = next;  /* keep it in the list and go to next element */
        else {  /* everything else is removed */
          /* white objects are simply removed */
  g->gcstate = GCSswpallgc;
  g->gcstate = GCSswpallgc;
  setmetatable(obj, {__gc = gcf, x = "+"})   -- create its metatable
do   -- another bug in 5.4.0
  local old = {10}
  collectgarbage()   -- make 'old' old
  local co = coroutine.create(
    function ()
      local x = nil
      local f = function ()
                  return x[1]
                end
      x = coroutine.yield(f)
      coroutine.yield()
    end
  )
  local _, f = coroutine.resume(co)   -- create closure over 'x' in coroutine
  collectgarbage("step", 0)   -- make upvalue a survival
  old[1] = {"hello"}    -- 'old' go to grayagain as 'touched1'
  coroutine.resume(co, {123})     -- its value will be new
  co = nil
  collectgarbage("step", 0)   -- hit the barrier
  assert(f() == 123 and old[1][1] == "hello")
  collectgarbage("step", 0)   -- run the collector once more
  -- make sure old[1] was not collected
  assert(f() == 123 and old[1][1] == "hello")
end