Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when array is growing to a normally valid size #49041

Closed
WRtux opened this issue Aug 6, 2023 · 2 comments
Closed

Crash when array is growing to a normally valid size #49041

WRtux opened this issue Aug 6, 2023 · 2 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. wrong repo Issues that should be opened in another repository.

Comments

@WRtux
Copy link

WRtux commented Aug 6, 2023

Version

v18.17.0

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

No response

What steps will reproduce the bug?

// Safe
/* try */ {
  let sparse = Array(130_000_000);

  for (let i = 0; i < sparse.length; ) {
    sparse[i] = i;
    if (++i % 1_000_000 === 0)
      console.log(i);
  }
}

// Safe, throws correctly
try {
  let sparse = Array(140_000_000);

  for (let i = 0; i < sparse.length; ) {
    sparse[i] = i;
    if (++i % 1_000_000 === 0)
      console.log(i);
  }
} catch (err) {
  console.error(err);
}

// Crash!
{
  let dataset = [];

  for (let i = 0; i < 120_000_000; ) {
    dataset.push(i);
    if (++i % 1_000_000 === 0)
      console.log(i);
  }
}

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior? Why is that the expected behavior?

No errors raised on the third case, or throwing RangeError: Invalid array length like the second case.

What do you see instead?

For the first two cases, memory is initially allocated on-the-go until reaching about 12M loops, when the rest of the full array is allocated at once (or throwing an error if exceeding the limit).

For the last case, memory usage increases by step, and Node.js crashes a short time after the last increase (may need adding some delay in the snippet to slow down the process to see).

Console log:

1000000
2000000
...
11000000
RangeError: Invalid array length
    at ...
1000000
2000000
...
112000000

#
# Fatal error in , line 0
# Fatal JavaScript invalid size error 169220804
#
#
#
#FailureMessage Object: 0000003FF3DFEC10
 1: 00007FF635E6B34F v8::internal::CodeObjectRegistry::~CodeObjectRegistry+123599
 2: 00007FF635D87F8F std::basic_ostream<char,std::char_traits<char> >::operator<<+65407
 3: 00007FF636A646C2 V8_Fatal+162
 4: 00007FF6365E78A5 v8::internal::FactoryBase<v8::internal::Factory>::NewFixedArray+101
 5: 00007FF6364A7D22 v8::Message::GetIsolate+16706
 6: 00007FF63631B7E1 v8::internal::CompilationCache::IsEnabledScriptAndEval+26913
 7: 00007FF6367BA061 v8::internal::SetupIsolateDelegate::SetupHeap+494417
 8: 000001F83E565AF5

Additional information

I've checked #47928 but I believe this is a problem beyond that. It seems that the buffer containing the array may try to grow to an invalid size if its size is changed dynamically, while it could still accommodate about 20M elements (by adding a check to limit size to grow to) before it really can't hold anything more. I find this quite annoying when I need to set a safe limit of element count and don't know how the arrays will be used in other places.

@bnoordhuis
Copy link
Member

This is ultimately a V8 issue, not a node issue, so this bug tracker is not the right place to report it. Having said that:

No errors raised on the third case, or throwing RangeError: Invalid array length like the second case.

V8 makes no guarantees about what happens when arrays grow too big. Both a RangeError or process termination are equally plausible outcomes.

The outcome you see in that third test case is because the array grows dynamically, whereas the first two arrays are allocated once and never resized.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Aug 6, 2023
@bnoordhuis bnoordhuis added v8 engine Issues and PRs related to the V8 dependency. wrong repo Issues that should be opened in another repository. labels Aug 6, 2023
@WRtux
Copy link
Author

WRtux commented Aug 10, 2023

Both a RangeError or process termination are equally plausible outcomes.

Maybe the "expected behavior" I provided is not so appropriate. Since the real limit of an array is about 128M elements, the third test case actually crashed when it was about 20M elements smaller than limit. Therefore, I think it is better not to cause any errors, because the code looks quite valid if I don't know array growing's mechanism. And that is the concern of this issue.

Anyway, having known that this is a V8 problem, I will ask about the details there. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. wrong repo Issues that should be opened in another repository.
Projects
None yet
Development

No branches or pull requests

2 participants