Fix unsafe write in wide array insertion (#4850) (#4855)

docs/CONTRIBUTORS

@@ -134,6 +134,7 @@ Nathan Kohagen
 Nathan Myers
 Oleh Maksymenko
 Patrick Stewart
+Paul Swirhun
 Paul Wright
 Pawel Sagan
 Pengcheng Xu

include/verilated_funcs.h

@@ -1266,6 +1266,7 @@ static inline void _vl_insert_QQ(QData& lhsr, QData ld, int hbit, int lbit, int
 }
 static inline void _vl_insert_WI(WDataOutP iowp, IData ld, int hbit, int lbit,
                                  int rbits = 0) VL_MT_SAFE {
+    // Insert value ld into iowp at bit slice [hbit:lbit]. iowp is rbits wide.
     const int hoffset = VL_BITBIT_E(hbit);
     const int loffset = VL_BITBIT_E(lbit);
     const int roffset = VL_BITBIT_E(rbits);
@@ -1276,7 +1277,7 @@ static inline void _vl_insert_WI(WDataOutP iowp, IData ld, int hbit, int lbit,
 
     if (hoffset == VL_SIZEBITS_E && loffset == 0) {
         // Fast and common case, word based insertion
-        iowp[VL_BITWORD_E(lbit)] = ld & cleanmask;
+        iowp[lword] = ld & cleanmask;
     } else {
         const EData lde = static_cast<EData>(ld);
         if (hword == lword) {  // know < EData bits because above checks it
@@ -1289,8 +1290,11 @@ static inline void _vl_insert_WI(WDataOutP iowp, IData ld, int hbit, int lbit,
             const EData linsmask = (VL_MASK_E((VL_EDATASIZE - 1) - loffset + 1)) << loffset;
             const int nbitsonright = VL_EDATASIZE - loffset;  // bits that end up in lword
             iowp[lword] = (iowp[lword] & ~linsmask) | ((lde << loffset) & linsmask);
-            iowp[hword]
-                = (iowp[hword] & ~hinsmask) | ((lde >> nbitsonright) & (hinsmask & cleanmask));
+            // Prevent unsafe write where lword was final writable location and hword is out-of-bounds.
+            if (VL_LIKELY(!(hword == rword && roffset == 0))) {
+                iowp[hword]
+                    = (iowp[hword] & ~hinsmask) | ((lde >> nbitsonright) & (hinsmask & cleanmask));
+            }
         }
     }
 }

test_regress/t/t_math_insert_bound.pl

@@ -0,0 +1,21 @@
+#!/usr/bin/env perl
+if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; }
+# DESCRIPTION: Verilator: Verilog Test driver/expect definition
+#
+# Copyright 2024 by Wilson Snyder. This program is free software; you
+# can redistribute it and/or modify it under the terms of either the GNU
+# Lesser General Public License Version 3 or the Perl Artistic License
+# Version 2.0.
+# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0
+
+scenarios(simulator => 1);
+
+compile(
+    );
+
+execute(
+    check_finished => 1,
+    );
+
+ok(1);
+1;

test_regress/t/t_math_insert_bound.v

@@ -0,0 +1,100 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2024 by Paul Swirhun.
+// SPDX-License-Identifier: CC0-1.0
+
+// Demonstrates the bug in https://github.com/verilator/verilator/issues/4850
+//
+// Specifically, _vl_insert_WI() writes to lword and hword when lword != hword
+// may be unsafe, because (for example), lword was the highest valid place to
+// perform a write and hword is out-of-bounds (and will in fact clobber other
+// state in the generated C++ struct!).
+
+
+module t(/*AUTOARG*/
+   // Inputs
+   clk
+   );
+   input clk;
+
+   integer cyc = 13;
+
+   // These need to be generated/consumed in this testbench so that
+   // they do not get pruned away when verilated
+   logic insert = '0;
+   logic [3:0] used, free;
+   logic [95:0] data;
+
+   always_ff @(posedge clk) begin
+      insert <= '1;
+      cyc <= cyc - 1;
+
+`ifdef TEST_VERBOSE
+      $write("used [4'd%2d], free [4'd%2d], data = [96'h%012x]\n", used, free, data);
+`endif
+
+      if (used + free != 12) begin
+         $write("used [4'd%2d] + free [4'd%2d] != 4'd12\n", used, free);
+         $stop();
+      end
+      if (used == 0) begin
+         $write("used [4'd%2d] was clobbered (should always be nonzero).\n", used);
+         $stop();
+      end
+      if (cyc == 0) begin
+         if (used == 12 && free == 0 && data == 96'hFF) begin
+            $write("*-* All Finished *-*\n");
+            $finish;
+         end else begin
+            $write("Test Failed! used/free/data had unexpected final value(s).\n");
+            $stop();
+         end
+      end
+   end
+
+   dut dut_i(
+      .clk(clk),
+      .insert(insert),
+      .used(used),
+      .free(free),
+      .data(data)
+   );
+
+endmodule
+
+module dut(
+   input logic clk,
+   input logic insert,
+   output logic [3:0] used,
+   output logic [3:0] free,
+   output logic [95:0] data
+   );
+
+   // This declaration order matters -- the fact that d_data is *before* d_used/d_free
+   // means that with the existing bug, writes to d_data that extend beyond its length
+   // will overwrite other fields in the state struct -- basically an "unsafe writes"
+   // problem because the existing code wrote beyond the end of the array d_data.
+   logic [11:0][7:0] d_data = '1, d_data_next;
+   logic [3:0] d_used = 4'd1, d_free = 4'd11, d_used_next;
+   assign used = d_used;
+   assign free = d_free;
+   assign data = d_data;
+
+   always_ff @(posedge clk) begin
+      d_data <= d_data_next;
+      d_used <= d_used_next;
+      d_free <= 12 - d_used_next;
+   end
+
+   always_comb begin
+    d_data_next = d_data;
+    d_used_next = d_used;
+
+    if ((insert == 1'b1) && (d_free >= {3'b0, insert})) begin
+      // This write to d_data would clobber d_used before the issue was fixed
+      d_data_next[d_used+:4] = 32'd0;
+      d_used_next += 4'd1;
+    end
+  end
+endmodule