Bug: UB when applying GC parameter

When computing whether Lua should return from gen-major to gen-minor,
the difference between the total memory and the previous total memory
can be negative, which can result in that negative value being
left-shifted (UB).

ldebug.c

@@ -150,7 +150,7 @@ LUA_API lua_Hook lua_gethook (lua_State *L) {
 
 
 LUA_API int lua_gethookmask (lua_State *L) {
-  return L->hookmask;
+  return cast_int(L->hookmask);
 }
 
 

lgc.c

@@ -1472,7 +1472,8 @@ static int checkmajorminor (lua_State *L, global_State *g) {
   if (g->gckind == KGC_GENMAJOR) {  /* generational mode? */
     l_mem numbytes = gettotalbytes(g);
     l_mem addedbytes = numbytes - g->GCmajorminor;
-    l_mem limit = applygcparam(g, MAJORMINOR, addedbytes);
+    l_mem limit = (addedbytes < 0) ? 0
+                                   : applygcparam(g, MAJORMINOR, addedbytes);
     l_mem tobecollected = numbytes - g->GCmarked;
     if (tobecollected > limit) {
       atomic2gen(L, g);  /* return to generational mode */

lgc.h

@@ -231,8 +231,9 @@
 #endif
 
 #define luaC_condGC(L,pre,pos) \
-	{ if (G(L)->GCdebt <= 0) { pre; luaC_step(L); pos;}; \
-	  condchangemem(L,pre,pos,0); }
+	{ if (G(L)->GCdebt <= 0)  \
+	  { pre; luaC_step(L); condchangemem(L,{},{},0); pos;} \
+	  else condchangemem(L,pre,pos,0); }
 
 /* more often than not, 'pre'/'pos' are empty */
 #define luaC_checkGC(L)		luaC_condGC(L,(void)0,(void)0)

lobject.c

@@ -89,6 +89,7 @@ lu_byte luaO_codeparam (unsigned int p) {
 l_mem luaO_applyparam (lu_byte p, l_mem x) {
   int m = p & 0xF;  /* mantissa */
   int e = (p >> 4);  /* exponent */
+  lua_assert(x >= 0);
   if (e > 0) {  /* normalized? */
     e--;  /* correct exponent */
     m += 0x10;  /* correct mantissa; maximum value is 0x1F */

lobject.h

@@ -827,7 +827,7 @@ typedef struct Table {
 
 
 #define twoto(x)	(1u<<(x))
-#define sizenode(t)	(twoto((t)->lsizenode))
+#define sizenode(t)	cast_uint(twoto((t)->lsizenode))
 
 
 /* size of buffer for 'luaO_utf8esc' function */

loslib.c

@@ -345,11 +345,22 @@ static int os_date (lua_State *L) {
 }
 
 
+static int os_time_aux (lua_State *L, time_t t, int err) {
+  if (err || t != (time_t)(l_timet)t)
+    return luaL_error(L,
+                  "time result cannot be represented in this installation");
+  l_pushtime(L, t);
+  return 1;
+}
+
+
 static int os_time (lua_State *L) {
-  time_t t;
-  if (lua_isnoneornil(L, 1))  /* called without args? */
-    t = time(NULL);  /* get current time */
+  if (lua_isnoneornil(L, 1)) {  /* called without args? */
+    time_t t = time(NULL);  /* get current time; error if it is -1 */
+    return os_time_aux(L, t, (t == (time_t)(-1)));
+  }
   else {
+    time_t t;
     struct tm ts;
     luaL_checktype(L, 1, LUA_TTABLE);
     lua_settop(L, 1);  /* make sure table is at the top */
@@ -360,14 +371,12 @@ static int os_time (lua_State *L) {
     ts.tm_min = getfield(L, "min", 0, 0);
     ts.tm_sec = getfield(L, "sec", 0, 0);
     ts.tm_isdst = getboolfield(L, "isdst");
+    ts.tm_wday = -1;  /* if call succeeds, it will set this value */
     t = mktime(&ts);
     setallfields(L, &ts);  /* update fields with normalized values */
+    /* error if tm_wday was not set */
+    return os_time_aux(L, t, ts.tm_wday == -1);
   }
-  if (t != (time_t)(l_timet)t || t == (time_t)(-1))
-    return luaL_error(L,
-                  "time result cannot be represented in this installation");
-  l_pushtime(L, t);
-  return 1;
 }
 
 

lvm.c

@@ -1117,7 +1117,7 @@ void luaV_finishOp (lua_State *L) {
 
 
 
-#define updatetrap(ci)  (trap = ci->u.l.trap)
+#define updatetrap(ci)  (trap = cast_int(ci->u.l.trap))
 
 #define updatebase(ci)	(base = ci->func.p + 1)
 
@@ -1211,7 +1211,7 @@ void luaV_execute (lua_State *L, CallInfo *ci) {
 #include "ljumptab.h"
 #endif
  startfunc:
-  trap = L->hookmask;
+  trap = cast_int(L->hookmask);
  returning:  /* trap already set */
   cl = ci_func(ci);
   k = cl->p->k;

manual/manual.of

@@ -6071,8 +6071,8 @@ In both cases,
 the function pushes onto the stack the final value associated
 with @id{tname} in the registry.
 
-Usage note: Beware the use of the return value of this function to
-conditionally initializes the new metatable
+Usage note: Beware the use of the return value of this function
+to conditionally initialize the new metatable
 (e.g., by adding metamethods to it).
 If the initialization raises an error,
 the metatable will not be properly initialized,

testes/files.lua

@@ -854,6 +854,13 @@ local function checkDateTable (t)
   _G.D = nil
 end
 
+
+do   -- testing mktime returning -1
+  local t = os.date("*t", -1)
+  assert(os.time(t) == -1)
+end
+
+
 checkDateTable(os.time())
 if not _port then
   -- assume that time_t can represent these values

testes/gengc.lua

@@ -162,6 +162,51 @@ end
 assert(collectgarbage'isrunning')
 
 
+do
+  -- bug in 5.0: when computing whether it should return from gen-major
+  -- to gen-minor, the difference between the total memory and the
+  -- previous total memory can be negative, which results in that
+  -- negative value being left-shifted (UB)
+
+  local lim = 1e6
+
+  -- make major collections non-incremental
+  local oldsm = collectgarbage("param", "stepmul", 0)
+
+  -- make "majorminor" large enough to force a left-shift
+  -- when applying the parameter (internal details)
+  local oldmm = collectgarbage("param", "majorminor", 2000)
+
+  collectgarbage(); collectgarbage()
+  assert(not T or T.gcquery() == "genminor")
+
+  local M = collectgarbage"count" * 1024
+
+  -- create a large table
+  local t = {}
+  for i = 1, lim do t[i] = true end
+  assert(collectgarbage"count" * 1024 > M + lim * string.packsize"j")
+
+  -- force collector to "generational major" mode, doing several
+  -- minor collections that recover no memory
+  collectgarbage"step"; collectgarbage"step"; collectgarbage"step"
+  assert(not T or T.gcquery() == "genmajor")
+
+  -- shrink the table
+  for i = 1, lim do t[i] = nil end
+  t[2 * lim] = true
+  assert(collectgarbage"count" < M * 5/4)
+
+  -- bug was here, an assert violation when checking whether to
+  -- return to 'genminor'
+  collectgarbage"step"
+
+  -- restore previous parameters
+  collectgarbage("param", "stepmul", oldsm)
+  collectgarbage("param", "majorminor", oldmm)
+end
+
+
 do  print"testing stop-the-world collection"
   local step = collectgarbage("param", "stepsize", 0);
   collectgarbage("incremental")