isolated-vm: Timeout is ignored when creating very large `BigInt`s

The bad news is that this error can’t be fixed with a polyfill like the Array.fill issue. Even with a polyfill you could end up with ctx.eval('eval("1".repeat(1e8) + "n")') which invokes the same problematic v8 code but directly from the parser.

The good news is that the fix in v8 seems relatively simple. I hacked this together which fixes the issue on my end:

diff --git a/src/numbers/conversions.cc b/src/numbers/conversions.cc
index 70d914e6df..790a836aa8 100644
--- a/src/numbers/conversions.cc
+++ b/src/numbers/conversions.cc
@@ -246,6 +246,7 @@ class StringToIntHelper {
   void set_state(State state) { state_ = state; }
 
  private:
+  bool CheckTermination();
   template <class Char>
   void DetectRadixInternal(Char current, int length);
   template <class Char>
@@ -295,6 +296,16 @@ void StringToIntHelper<LocalIsolate>::ParseInt() {
   DCHECK_NE(state_, State::kRunning);
 }
 
+template <typename LocalIsolate>
+bool StringToIntHelper<LocalIsolate>::CheckTermination() { return false; }
+
+template <>
+bool StringToIntHelper<Isolate>::CheckTermination() {
+  StackLimitCheck interrupt_check(isolate());
+  return interrupt_check.InterruptRequested() &&
+    isolate()->stack_guard()->HandleInterrupts().IsException(isolate());
+}
+
 template <typename LocalIsolate>
 template <class Char>
 void StringToIntHelper<LocalIsolate>::DetectRadixInternal(Char current,
@@ -425,7 +436,7 @@ void StringToIntHelper<LocalIsolate>::ParseInternal(Char start) {
       DCHECK(multiplier > part);
 
       ++current;
-      if (current == end) {
+      if (current == end || CheckTermination()) {
         done = true;
         break;
       }

The pipeline from v8 pull request to nodejs release is a slow one so if this issue is causing you problems you’ll have to patch nodejs and compile yourself. I’ll post here with updates when I reach out to the v8 team about the issue.

Regardless, it’s clear that a very big rearchitecture needs to happen in order to harden isolated-vm. The whole thing really needs to be rebuilt on a multi-process abstraction to protect against the speculative execution attacks disclosed in 2018. This would also address the shared heap issues which popped up in nodejs v19.x as well. That’s a month long project that I just don’t have the spare time for though.

onCatastrophicError is a stop gap solution for this kind of issue though. It provides a signal that v8 has failed to terminate and isolated-vm intentionally permanently deadlocks the thread to prevent it from doing any more damage. It gives you the opportunity to gracefully shut down the server, and potentially mark this script or user as problematic to avoid running their code in the future.

@Gongreg that’s actually surprising to hear because I personally fixed this exact issue [very large BigInt parses] in v8 back in 2020: https://github.com/v8/v8/commit/7e8e76e784061277e13112c67c21c3f9438da257. I did kind of regret not making the chunk size smaller since it would take at least 250ms to time out if I remember correctly. I wonder if it regressed.

Hello, I wanted to get some clarification on this issue.

Lets take the following code snippet:

const { Isolate } = require("isolated-vm")

let isolate = new Isolate({ memoryLimit: 128 })
let ctx = isolate.createContextSync()

setInterval(async () => {
  //try to stop the vm even if it is still running (simulating that we've noticed that vm is stalled).
  ctx.release()
  isolate.dispose()

  isolate = new Isolate({ memoryLimit: 128 })
  ctx = isolate.createContextSync()

  ctx.eval('BigInt("1".repeat(1e8))', {
    timeout: 100,
  })

  console.log(process.memoryUsage())
}, 1000)

As far as I understand it is expected that doing the isolates will go over defined memory limit (saw that mentioned in multiple issues).

But what I am not able to find info about, is it is expected that trying to dispose of the isolate is going to fail & the memory usage is going to keep increasing?

{
  rss: 103579648,
  heapTotal: 40501248,
  heapUsed: 34076056,
  external: 1162687,
  arrayBuffers: 41942
}
{
  rss: 357187584,
  heapTotal: 40501248,
  heapUsed: 34090424,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 558678016,
  heapTotal: 40501248,
  heapUsed: 34098112,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 743833600,
  heapTotal: 40501248,
  heapUsed: 34105560,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 945946624,
  heapTotal: 40501248,
  heapUsed: 34113160,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 1131200512,
  heapTotal: 40501248,
  heapUsed: 34120936,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 1323040768,
  heapTotal: 40501248,
  heapUsed: 34128552,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 1517076480,
  heapTotal: 40501248,
  heapUsed: 34137200,
  external: 1162727,
  arrayBuffers: 41942
}
{
  rss: 1696989184,
  heapTotal: 33947648,
  heapUsed: 31329152,
  external: 1142050,
  arrayBuffers: 24798
}
{
  rss: 1882669056,
  heapTotal: 33947648,
  heapUsed: 31415704,
  external: 1142050,
  arrayBuffers: 24798
}
{
  rss: 2080964608,
  heapTotal: 33947648,
  heapUsed: 31448464,
  external: 1142050,
  arrayBuffers: 24798
}

If this is expected, maybe there are recommendations how to recover from the following scenario?

This one I think fails in the compiler. And compiler in v8 is not interruptable. To fix: fix it in v8. V8 has two main security problems, IMHO.

1. Compiler is not interruptable.
2. OOM just kills everything. I have kinda “fixed” those by doing ugly stuff above in plv8. But it is ugly stuff and I’m lucky that it worked.

I resubmitted the patch after the revert and it was accepted with some broader changes: https://github.com/v8/v8/commit/7e8e76e784061277e13112c67c21c3f9438da257 https://chromium-review.googlesource.com/c/v8/v8/+/2392629

You can definitely report them here first. v8’s parser is uninterruptible and that will probably never change. In this case, v8 gives you a hook to validate any source passed to the “eval family” of functions. So we can just throw out any unreasonably long strings and refuse to compile them.

My fix landed in v8 today – https://chromium-review.googlesource.com/c/v8/v8/+/2384166

I can probably convince the nodejs team to cherrypick this into nodejs v14.x. I found out that this materially affects vanilla nodejs. If you copy and paste BigInt("1".repeat(1e8)) into the nodejs REPL it locks up the process before you even press enter.