Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Add NaN check to IR_NEWREF.
@ 2023-11-16  8:49 Sergey Kaplun via Tarantool-patches
  2023-11-17 10:23 ` Maxim Kokryashkin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-11-16  8:49 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry-picked from commit 7f9907b4ed0870ba64342bcc4b26cff0a94540da)

When emitting IR NEWREF, there is no check for a non-NaN stored key
value. Thus, when the NaN number value is given to trace, it may be
stored as a key. This patch adds the corresponding check. If fold
optimization is enabled, this IR EQ check is dropped if it references
CONV IR from any (unsigned) integer type since NaN can be created via
conversion from an integer.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1069-newref-nan-key
Tarantool PR: https://github.com/tarantool/tarantool/pull/9374
Fuzzer link: https://oss-fuzz.com/testcase-detail/5251574662037504
Relate issues:
* https://github.com/LuaJIT/LuaJIT/issues/1069
* https://github.com/tarantool/tarantool/issues/9145

 src/lj_opt_fold.c                             |   5 +-
 src/lj_record.c                               |  12 +-
 .../lj-1069-newref-nan-key.test.lua           | 151 ++++++++++++++++++
 3 files changed, 164 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1069-newref-nan-key.test.lua

diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index 944a9ecc..61169397 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -1936,7 +1936,10 @@ LJFOLD(NE any any)
 LJFOLDF(comm_equal)
 {
   /* For non-numbers only: x == x ==> drop; x ~= x ==> fail */
-  if (fins->op1 == fins->op2 && !irt_isnum(fins->t))
+  if (fins->op1 == fins->op2 &&
+      (!irt_isnum(fins->t) ||
+       (fleft->o == IR_CONV &&  /* Converted integers cannot be NaN. */
+	(uint32_t)(fleft->op2 & IRCONV_SRCMASK) - (uint32_t)IRT_I8 <= (uint32_t)(IRT_U64 - IRT_U8))))
     return CONDFOLD(fins->o == IR_EQ);
   return fold_comm_swap(J);
 }
diff --git a/src/lj_record.c b/src/lj_record.c
index 3189a7c3..3e4914e1 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -1509,10 +1509,16 @@ TRef lj_record_idx(jit_State *J, RecordIndex *ix)
       lj_assertJ(!hasmm, "inconsistent metamethod handling");
       if (oldv == niltvg(J2G(J))) {  /* Need to insert a new key. */
 	TRef key = ix->key;
-	if (tref_isinteger(key))  /* NEWREF needs a TValue as a key. */
+	if (tref_isinteger(key)) {  /* NEWREF needs a TValue as a key. */
 	  key = emitir(IRTN(IR_CONV), key, IRCONV_NUM_INT);
-	else if (tref_isnumber(key) && tref_isk(key) && tvismzero(&ix->keyv))
-	  key = lj_ir_knum_zero(J);  /* Canonicalize -0.0 to +0.0. */
+	} else if (tref_isnum(key)) {
+	  if (tref_isk(key)) {
+	    if (tvismzero(&ix->keyv))
+	      key = lj_ir_knum_zero(J);  /* Canonicalize -0.0 to +0.0. */
+	  } else {
+	    emitir(IRTG(IR_EQ, IRT_NUM), key, key);  /* Check for !NaN. */
+	  }
+	}
 	xref = emitir(IRT(IR_NEWREF, IRT_PGC), ix->tab, key);
 	keybarrier = 0;  /* NEWREF already takes care of the key barrier. */
 #ifdef LUAJIT_ENABLE_TABLE_BUMP
diff --git a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
new file mode 100644
index 00000000..ec28b274
--- /dev/null
+++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua
@@ -0,0 +1,151 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour for NEWREF IR
+-- taken NaN value.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/1069.
+
+local test = tap.test('lj-1069-newref-nan-key'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+local table_new = require('table.new')
+local ffi = require('ffi')
+
+local NaN = tonumber('nan')
+
+test:plan(4)
+
+test:test('NaN on trace in the non-constant IR', function(subtest)
+  local NKEYS = 3
+
+  -- XXX: NaN isn't stored, so the number of tests is:
+  -- (NKEYS - 1) + test status + test error message + test keys
+  -- amount.
+  subtest:plan(NKEYS + 2)
+
+  local tset_table = table_new(0, NKEYS)
+
+  local function tset(t, k)
+    -- Value doesn't matter.
+    t[k] = true
+  end
+
+  -- Compile the function.
+  jit.opt.start('hotloop=1')
+
+  -- Use number keys to emit NEWREF.
+  tset(tset_table, 0.1)
+  tset(tset_table, 0.2)
+
+  -- Insert NaN on the trace.
+  local ok, err = pcall(tset, tset_table, NaN)
+
+  subtest:ok(not ok, 'function returns an error')
+  subtest:like(err, 'table index is NaN', 'correct error message')
+
+  local nkeys = 0
+  for k in pairs(tset_table) do
+    nkeys = nkeys + 1
+    subtest:ok(k == k, ('not NaN key by number %d'):format(nkeys))
+  end
+  subtest:is(nkeys, NKEYS - 1, 'correct amount of keys')
+end)
+
+test:test('NaN on trace in the non-constant IR CONV', function(subtest)
+  local tonumber = tonumber
+  local NKEYS = 3
+
+  -- XXX: NaN isn't stored, so the number of tests is:
+  -- (NKEYS - 1) + test status + test error message + test keys
+  -- amount.
+  subtest:plan(NKEYS + 2)
+
+  local tset_table = table_new(0, NKEYS)
+
+  local function tset(t, k)
+    -- XXX: Emit CONV to number type. Value doesn't matter.
+    t[tonumber(k)] = true
+  end
+
+  -- Compile the function.
+  jit.on()
+  jit.opt.start('hotloop=1')
+
+  -- Use number keys to emit NEWREF.
+  tset(tset_table, ffi.new('float', 0.1))
+  tset(tset_table, ffi.new('float', 0.2))
+
+  -- Insert NaN on the trace.
+  local ok, err = pcall(tset, tset_table, ffi.new('float', NaN))
+
+  subtest:ok(not ok, 'function returns an error')
+  subtest:like(err, 'table index is NaN', 'correct error message')
+
+  local nkeys = 0
+  for k in pairs(tset_table) do
+    nkeys = nkeys + 1
+    subtest:ok(k == k, ('not NaN key by number %d'):format(nkeys))
+  end
+  subtest:is(nkeys, NKEYS - 1, 'correct amount of keys')
+end)
+
+-- Test the constant IR NaN value on the trace.
+
+test:test('constant NaN on the trace', function(subtest)
+  -- Test the status and the error message.
+  subtest:plan(2)
+  local function protected()
+    local counter = 0
+    -- Use a number key to emit NEWREF.
+    local key = 0.1
+
+    jit.opt.start('hotloop=1')
+
+    while counter < 2 do
+      counter = counter + 1
+      -- luacheck: ignore
+      local tab = {_ = 'unused'}
+      tab[key] = 'NaN'
+      -- XXX: Use the constant NaN value on the trace.
+      key = 0/0
+    end
+  end
+
+  local ok, err = pcall(protected)
+  subtest:ok(not ok, 'function returns an error')
+  subtest:like(err, 'table index is NaN', 'NaN index error message')
+end)
+
+test:test('constant NaN on the trace and rehash of the table', function(subtest)
+  -- A little bit different test case: after rehashing the table,
+  -- the node is lost, and a hash part of the table isn't freed.
+  -- This leads to the assertion failure, which checks memory
+  -- leaks on shutdown.
+  -- XXX: The test didn't fail even before the patch. Check the
+  -- same values as in the previous test for consistency.
+  subtest:plan(2)
+  local function protected()
+    local counter = 0
+    -- Use a number key to emit NEWREF.
+    local key = 0.1
+
+    jit.opt.start('hotloop=1')
+
+    while counter < 2 do
+      counter = counter + 1
+      -- luacheck: ignore
+      local tab = {_ = 'unused'}
+      tab[key] = 'NaN'
+      -- Rehash the table.
+      tab[counter] = 'unused'
+      -- XXX: Use the constant NaN value on the trace.
+      key = 0/0
+    end
+  end
+
+  local ok, err = pcall(protected)
+  subtest:ok(not ok, 'function returns an error')
+  subtest:like(err, 'table index is NaN', 'NaN index error message')
+end)
+
+test:done(true)
-- 
2.42.0


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-23  6:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-16  8:49 [Tarantool-patches] [PATCH luajit] Add NaN check to IR_NEWREF Sergey Kaplun via Tarantool-patches
2023-11-17 10:23 ` Maxim Kokryashkin via Tarantool-patches
2023-11-20  8:48   ` Sergey Kaplun via Tarantool-patches
2023-11-18 15:13 ` Sergey Bronnikov via Tarantool-patches
2023-11-20  8:51   ` Sergey Kaplun via Tarantool-patches
2023-11-20  8:57     ` Sergey Bronnikov via Tarantool-patches
2023-11-23  6:31 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox