fix(rpc): "grid_line" event parsing crashes (#25581)

refactor: use a more idiomatic loop to iterate over the cells

There are two cases in which the following assertion would fail:
```c
assert(g->icell < g->ncells);
```

1. If `g->ncells = 0`. Update this to be legal.
2. If an EOF is reached while parsing `wrap`. In this case, the unpacker
   attempts to resume from `cells`, which is a bug. Create a new state
   for parsing `wrap`.

Reference: https://neovim.io/doc/user/ui.html#ui-event-grid_line

4 files changed, 128 insertions, 49 deletions

diff --git a/src/nvim/msgpack_rpc/unpacker.c b/src/nvim/msgpack_rpc/unpacker.c
index 5a65e6c5df..9b7fc3fbb7 100644
--- a/src/nvim/msgpack_rpc/unpacker.c
+++ b/src/nvim/msgpack_rpc/unpacker.c
@@ -291,13 +291,13 @@ error:
 // objects. For the moment "redraw/grid_line" uses a hand-rolled decoder,
 // to avoid a blizzard of small objects for each screen cell.
 //
-// <0>[2, "redraw", <10>[{11}["method", <12>[args], <12>[args], ...], <11>[...], ...]]
+// <0>[2, "redraw", <10>[<11>["method", <12>[args], <12>[args], ...], <11>[...], ...]]
 //
 // Where [args] gets unpacked as an Array. Note: first {11} is not saved as a state.
 //
 // When method is "grid_line", we furthermore decode a cell at a time like:
 //
-// <0>[2, "redraw", <10>[{11}["grid_line", <14>[g, r, c, [<15>[cell], <15>[cell], ...]], ...], <11>[...], ...]]
+// <0>[2, "redraw", <10>[<11>["grid_line", <14>[g, r, c, [<15>[cell], <15>[cell], ...], <16>wrap]], <11>[...], ...]]
 //
 // where [cell] is [char, repeat, attr], where 'repeat' and 'attr' is optional
 
@@ -322,7 +322,7 @@ bool unpacker_advance(Unpacker *p)
       return false;
     }
 
-    if (p->state == 15) {
+    if (p->state == 16) {
       // grid_line event already unpacked
       goto done;
     } else {
@@ -357,10 +357,10 @@ done:
     p->state = 0;
     return true;
   case 13:
-  case 15:
+  case 16:
     p->ncalls--;
     if (p->ncalls > 0) {
-      p->state = (p->state == 15) ? 14 : 12;
+      p->state = (p->state == 16) ? 14 : 12;
     } else if (p->nevents > 0) {
       p->state = 11;
     } else {
@@ -393,7 +393,6 @@ bool unpacker_parse_redraw(Unpacker *p)
     return false; \
   }
 
-redo:
   switch (p->state) {
   case 10:
     NEXT_TYPE(tok, MPACK_TOKEN_ARRAY);
@@ -461,63 +460,64 @@ redo:
     FALLTHROUGH;
 
   case 15:
-    assert(g->icell < g->ncells);
-
-    NEXT_TYPE(tok, MPACK_TOKEN_ARRAY);
-    int cellarrsize = (int)tok.length;
-    if (cellarrsize < 1 || cellarrsize > 3) {
-      p->state = -1;
-      return false;
-    }
+    for (; g->icell != g->ncells; g->icell++) {
+      assert(g->icell < g->ncells);
+
+      NEXT_TYPE(tok, MPACK_TOKEN_ARRAY);
+      int cellarrsize = (int)tok.length;
+      if (cellarrsize < 1 || cellarrsize > 3) {
+        p->state = -1;
+        return false;
+      }
 
-    NEXT_TYPE(tok, MPACK_TOKEN_STR);
-    if (tok.length > size) {
-      return false;
-    }
+      NEXT_TYPE(tok, MPACK_TOKEN_STR);
+      if (tok.length > size) {
+        return false;
+      }
 
-    const char *cellbuf = data;
-    size_t cellsize = tok.length;
-    data += cellsize;
-    size -= cellsize;
+      const char *cellbuf = data;
+      size_t cellsize = tok.length;
+      data += cellsize;
+      size -= cellsize;
 
-    if (cellarrsize >= 2) {
-      NEXT_TYPE(tok, MPACK_TOKEN_SINT);
-      g->cur_attr = (int)tok.data.value.lo;
-    }
+      if (cellarrsize >= 2) {
+        NEXT_TYPE(tok, MPACK_TOKEN_SINT);
+        g->cur_attr = (int)tok.data.value.lo;
+      }
 
-    int repeat = 1;
-    if (cellarrsize >= 3) {
-      NEXT_TYPE(tok, MPACK_TOKEN_UINT);
-      repeat = (int)tok.data.value.lo;
-    }
+      int repeat = 1;
+      if (cellarrsize >= 3) {
+        NEXT_TYPE(tok, MPACK_TOKEN_UINT);
+        repeat = (int)tok.data.value.lo;
+      }
 
-    g->clear_width = 0;
-    if (g->icell == g->ncells - 1 && cellsize == 1 && cellbuf[0] == ' ' && repeat > 1) {
-      g->clear_width = repeat;
-    } else {
-      schar_T sc = schar_from_buf(cellbuf, cellsize);
-      for (int r = 0; r < repeat; r++) {
-        if (g->coloff >= (int)grid_line_buf_size) {
-          p->state = -1;
-          return false;
+      g->clear_width = 0;
+      if (g->icell == g->ncells - 1 && cellsize == 1 && cellbuf[0] == ' ' && repeat > 1) {
+        g->clear_width = repeat;
+      } else {
+        schar_T sc = schar_from_buf(cellbuf, cellsize);
+        for (int r = 0; r < repeat; r++) {
+          if (g->coloff >= (int)grid_line_buf_size) {
+            p->state = -1;
+            return false;
+          }
+          grid_line_buf_char[g->coloff] = sc;
+          grid_line_buf_attr[g->coloff++] = g->cur_attr;
         }
-        grid_line_buf_char[g->coloff] = sc;
-        grid_line_buf_attr[g->coloff++] = g->cur_attr;
       }
-    }
 
-    g->icell++;
-    if (g->icell == g->ncells) {
-      NEXT_TYPE(tok, MPACK_TOKEN_BOOLEAN);
-      g->wrap = mpack_unpack_boolean(tok);
       p->read_ptr = data;
       p->read_size = size;
-      return true;
     }
+    p->state = 16;
+    FALLTHROUGH;
 
+  case 16:
+    NEXT_TYPE(tok, MPACK_TOKEN_BOOLEAN);
+    g->wrap = mpack_unpack_boolean(tok);
     p->read_ptr = data;
     p->read_size = size;
-    goto redo;
+    return true;
 
   case 12:
     return true;
diff --git a/test/helpers.lua b/test/helpers.lua
index 02192e4924..f9405c011d 100644
--- a/test/helpers.lua
+++ b/test/helpers.lua
@@ -15,6 +15,7 @@ local function shell_quote(str)
   end
 end
 
+--- @class test.helpers
 local module = {
   REMOVE_THIS = {},
 }
diff --git a/test/unit/helpers.lua b/test/unit/helpers.lua
index 448209fe0b..8d581dac49 100644
--- a/test/unit/helpers.lua
+++ b/test/unit/helpers.lua
@@ -861,6 +861,7 @@ local function is_asan()
   end
 end
 
+--- @class test.unit.helpers.module
 local module = {
   cimport = cimport,
   cppimport = cppimport,
@@ -890,7 +891,9 @@ local module = {
   debug_log = debug_log,
   is_asan = is_asan,
 }
+--- @class test.unit.helpers: test.unit.helpers.module, test.helpers
 module = global_helpers.tbl_extend('error', module, global_helpers)
+
 return function()
   return module
 end
diff --git a/test/unit/msgpack_spec.lua b/test/unit/msgpack_spec.lua
new file mode 100644
index 0000000000..c573714714
--- /dev/null
+++ b/test/unit/msgpack_spec.lua
@@ -0,0 +1,75 @@
+local helpers = require('test.unit.helpers')(after_each)
+local cimport = helpers.cimport
+local itp = helpers.gen_itp(it)
+local lib = cimport('./src/nvim/msgpack_rpc/unpacker.h', './src/nvim/memory.h')
+local ffi = helpers.ffi
+local eq = helpers.eq
+local to_cstr = helpers.to_cstr
+
+--- @class Unpacker
+--- @field read_ptr ffi.cdata*
+--- @field read_size number
+
+--- @alias Unpacker* table<number, Unpacker>
+--- @return Unpacker* unpacker `unpacker[0]` to dereference
+local function make_unpacker()
+  return ffi.gc(ffi.cast('Unpacker*', lib.xcalloc(1, ffi.sizeof('Unpacker'))), function(unpacker)
+    lib.unpacker_teardown(unpacker, nil, nil)
+    lib.xfree(unpacker)
+  end)
+end
+
+--- @param unpacker Unpacker*
+--- @param data string
+--- @param size number? *default: data:len()*
+local function unpacker_goto(unpacker, data, size)
+  unpacker[0].read_ptr = to_cstr(data)
+  unpacker[0].read_size = size or data:len()
+end
+
+--- @param unpacker Unpacker*
+--- @return boolean
+local function unpacker_advance(unpacker)
+  return lib.unpacker_advance(unpacker)
+end
+
+describe('msgpack', function()
+  describe('unpacker', function()
+    itp('does not crash when paused between `cells` and `wrap` params of `grid_line` #25184', function()
+      -- [kMessageTypeNotification, "redraw", [
+      --   ["grid_line",
+      --     [2, 0, 0, [[" " , 0, 77]], false]
+      --   ]
+      -- ]]
+      local payload =
+        '\x93\x02\xa6\x72\x65\x64\x72\x61\x77\x91\x92\xa9\x67\x72\x69\x64\x5f\x6c\x69\x6e\x65\x95\x02\x00\x00\x91\x93\xa1\x20\x00\x4d\xc2'
+
+      local unpacker = make_unpacker()
+      lib.unpacker_init(unpacker)
+
+      unpacker_goto(unpacker, payload, payload:len() - 1)
+      local finished = unpacker_advance(unpacker)
+      eq(finished, false)
+
+      unpacker[0].read_size = unpacker[0].read_size + 1
+      finished = unpacker_advance(unpacker)
+      eq(finished, true)
+    end)
+
+    itp('does not crash when parsing grid_line event with 0 `cells` #25184', function()
+      local unpacker = make_unpacker()
+      lib.unpacker_init(unpacker)
+
+      unpacker_goto(unpacker,
+        -- [kMessageTypeNotification, "redraw", [
+        --   ["grid_line",
+        --     [2, 0, 0, [], false]
+        --   ]
+        -- ]]
+        '\x93\x02\xa6\x72\x65\x64\x72\x61\x77\x91\x92\xa9\x67\x72\x69\x64\x5f\x6c\x69\x6e\x65\x95\x02\x00\x00\x90\xc2'
+      )
+      local finished = unpacker_advance(unpacker)
+      eq(finished, true)
+    end)
+  end)
+end)