Bug#1030284: nodejs: [arm64] RangeError: Maximum call stack size exceeded

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

Hi Thorsten,

Are you able to determine whether https://github.com/nodejs/node/issues/41163
(and/or any of the guidance within that thread) seems relevant to this bug?

If so, your repro example could be useful to help upstream/contributors to
develop and test a solution.

Thanks,
James

[Thorsten Glaser]

Hi James,

(you might wish to Cc <${bugnumber}-subm...@bugs.debian.org> so they
actually get the reply…)

>Are you able to determine whether https://github.com/nodejs/node/issues/41163
>(and/or any of the guidance within that thread) seems relevant to this bug?

It appears so. I commented there, thank you for finding that link.

I guess there is even a… quick patch… to make from this. I admit
I’m very confused by that statement:

“if you set it too high, you risk crashes”

That can’t be right.

Searching through the nodejs source for where this stack size is
set, I see multiple time bombs for all architectures.

deps/v8/src/common/globals.h does set the default stack size to
864/984 KiB in order to achieve an about 1 MiB stack for JS plus
C++ parts combined.

I wonder if this shouldn’t be getrlimit(RLIMIT_STACK) - overhead,
and then define the per-architecture overhead in a suitable way.

That lower 1 MiB total limit seems to be for Windows. The lower
arm64 limit is caused by “allocating MacroAssembler takes 120 [KiB]”
but the total could still be raised I think… at least on unixoid
platforms other than WebView-on-Android. Since the location of these
defaults is in v8, it also applies for browsers and whatnot, but
nodejs could indeed inspect the current ulimit and set a better
default for at least nōn-Windows systems.

I’m not, unfortunately, in the position to provide a quick patch,
being a C developer, not CFrustFrust, and all that. I think that
InitializeNodeWithArgs in src/node.cc, which already has a call
to V8::SetFlagsFromString(NODE_V8_OPTIONS, …), is the likely place
for adding code (suitably platform-ifdef’d) that does:

- get the ulimit
- subtract some arch-specific overhead target
- check that that’s positive (or >= V8_DEFAULT_STACK_SIZE_KB even,
that might be a good idea)
- if so, pass this as synthetic --stack-size (or --stack_size?) to
v8, overriding its default but allowing for a later option given
by the user’s argv[] to override _that_, again

Might need to adjust some tests, too :~


Good luck,
//mirabilos
--
<igli> exceptions: a truly awful implementation of quite a nice idea.
<igli> just about the worst way you could do something like that, afaic.
<igli> it's like anti-design. <mirabilos> that too… may I quote you on that?
<igli> sure, tho i doubt anyone will listen ;)

[Jérémy Lal]

While waiting for the proper fix you describe, I propose to set higher constants

for V8_DEFAULT_STACK_SIZE_KB - especially for arm64.

Jérémy

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

On Wed, 15 Feb 2023 14:54:03 +0100, Jérémy wrote:
> While waiting for the proper fix you describe, I propose to set higher
> constants
> for V8_DEFAULT_STACK_SIZE_KB - especially for arm64.

That sounded good to me when I read it a week ago, but now I'm not so sure.

It seems that the Node ecosystem has known unusual architecture-specific
behaviours here that derive from a lower-level element (V8) in the stack.

Is it wise for us to add another layer of configuration settings that are
Debian-specific and will require future users/developers yet more time to
unpick and understand? Especially when the configuration may -- if set too
high -- introduce as many failures as it solves? (how could we tell?)

Maybe it's rare to propose 'do nothing' as a technical suggestion but I think
it is worth considering here, since we are not the arbiters of Node.

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

Control: forwarded -1 https://github.com/nodejs/node/issues/41163

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

X-Debbugs-Cc: t...@debian.org, reply+AAGSHFQLULDIWI3OBW...@reply.github.com

mirabilos gesagt:

> We know the default ulimits for users in Debian, and they are higher
> than the 1 MiB assumed by v8, by quite some factor, so this won’t break
> things which are not currently broken (by that exception). This will do
> for the release I think.

Relaying my understanding of this, so far:

An increase in the V8 stack size should not cause earlier-process-exits for any
processes that previously ran on Debian systems where the
architecture-default-or-greater stack size is configured[1].

In other words: the same-number-or-greater of JavaScript processes should
continue to run on any given Debian system where the configured stack size is
greater-than-or-equal to the architecture's default, after the V8 stack size
limit is increased.

And we expect that it should repair a failing reproducible build test for at
least one Debian package on arm64.

[1] - see limits.conf

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

X-Debbugs-Cc: t...@mirbsd.de

Hi - what do you both think of the attached patch, which brings the ARM stack
size into line with almost all other architectures (= 984 KB)?

(there is a reason not to increase the stack size further: there is a static
assertion[1] that the stack size plus 256 bytes is not greater than one MB,
and I don't feel keen to change that assertion's logic for this bug)

[1] - https://sources.debian.org/src/nodejs/18.13.0%2Bdfsg1-1/deps/v8/src/common/globals.h/#L105-L108

[James Addison]

On Tue, Feb 28, 2023, 17:55 Thorsten Glaser <t...@mirbsd.de> wrote:

Can you test it? I don’t have the bandwidth for that right now…

Should be able to, yep - I seem to remember seeing some repro instructions from you on the GitHub thread and will give those a try in an emulator/vm.

[James Addison]

That'd be great, thank you - my local (emulated) aarch64 build of
nodejs is proving to be much more time consuming than I expected.

On Tue, 28 Feb 2023 at 23:21, Jérémy Lal <kap...@melix.org> wrote:

> I can build nodejs on amhdal.debian.org if you're not comfortable with that.
>
>> --
>> Pkg-javascript-devel mailing list
>> Pkg-javasc...@alioth-lists.debian.net
>> https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-javascript-devel

[Thorsten Glaser]

Jérémy Lal dixit:

>I can build nodejs on amhdal.debian.org if you're not comfortable with that.

The problem with the DSA porterboxen is that you cannot install your own
built packages in the chroot to use them there… unless there’s a
solution not yet known to me?

bye,
//mirabilos
--
“ah that reminds me, thanks for the stellar entertainment that you and certain
other people provide on the Debian mailing lists │ sole reason I subscribed to
them (I'm not using Debian anywhere) is the entertainment factor │ Debian does
not strike me as a place for good humour, much less German admin-style humour”

[Jérémy Lal]

Le mer. 1 mars 2023 à 02:30, Thorsten Glaser <t...@mirbsd.de> a écrit :

Jérémy Lal dixit:

>I can build nodejs on amhdal.debian.org if you're not comfortable with that.

The problem with the DSA porterboxen is that you cannot install your own
built packages in the chroot to use them there… unless there’s a
solution not yet known to me?

Indeed, but the binary can be run from build dir, so I just need to try and reproduce the bug from there.

[Emanuele Rocca]

Hi,

On Wed, Mar 01, 2023 at 02:41:05PM +0100, Jérémy Lal wrote:
> For now I'm unlucky with the porterbox, because /var/run/schroot
> disappeared yesterday.

I can confirm that the issue isn't reproducible with V8_DEFAULT_STACK_SIZE_KB
set to 984. Built and tested on a Macbook M1.

(sid-arm64)ema@sarzana:~/debian/dygraphs-2.2.1
$ babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests
Successfully compiled 59 files with Babel (994ms).

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

X-Debbugs-Cc: t...@debian.org

Guidance received from the V8 project (a vendored dependency in the upstream
NodeJS codebase) on the v8-dev mailing list is, in summary/interpretation:

* It is not yet safe to increase the stack size limit on ARM64 systems.

* For a given constant stack size, recursion depth is architecture-dependent,
and so the patch to restore the 984K stack size on ARM64 would not
provide equal recursion depth on all systems.

* Exceeding stack depth limits is generally a sign of an application that
would benefit from relevant refactoring (personal note: for example, by
reducing the depth of recursion required, or by replacing a recursive
algorithm with an equivalent iterative algorithm).


Sidenotes:

A patch for 32-bit architectures could apparently be acceptable, although may
be best offered to NodeJS rather than V8. For what it's worth: NodeJS seems
to have a policy of not accepting patches to their vendored dependencies.

None of this rules out an rlimit-based approach as suggested by Thorsten.

[James Addison]

Package: nodejs
Followup-For: Bug #1030284

X-Debbugs-Cc: t...@mirbsd.de

On Thu, 02 Feb 2023 01:56:10 +0000, Thorsten wrote:
> I consider this an architecture-specific release-critical bug. Maybe
> having a reproducer and access to a porterbox will allow a nodejs
> maintainer to track this down.

Based on what I've learned about this bug, I believe that architecture-specific
behaviour related to stack sizes is inherent in the V8 library vendored by
upstream NodeJS.

Improvements may be possible and we can continue to track and assist towards
those, however there are likely to be runtime differences that remain for a
long time. Individual codebases such as the affected 'src:dygraphs' package
can also be improved to reduce the likelihood of call stack size overflow.

Given those, and the absence of any sense that this is a regression, I think we
should lower the priority of this bug to below release-critical.

[Andres Salomon]

On Sat, 11 Mar 2023 11:04:15 +0000 James Addison <j...@jp-hosting.net>
wrote:

> Package: nodejs
> Followup-For: Bug #1030284

> X-Debbugs-Cc: t...@debian.org
>
> Guidance received from the V8 project (a vendored dependency in the
upstream
> NodeJS codebase) on the v8-dev mailing list is, in
summary/interpretation:
>
> * It is not yet safe to increase the stack size limit on ARM64
systems.

[...]

> Sidenotes:
>
> A patch for 32-bit architectures could apparently be acceptable,
although may
> be best offered to NodeJS rather than V8. For what it's worth:
NodeJS seems
> to have a policy of not accepting patches to their vendored
dependencies.
>

In reading Jakob's response
(https://groups.google.com/g/v8-dev/c/7ZI3vxtabcU/m/c9qvHkOBBAAJ), I'm
interpreting it slightly differently-

He says that raising the stack limit *is* safe for 32-bit ARM, even
inside of the V8 upstream source tree.

For ARM64, he says that raising the stack limit is not safe for v8
*embedded inside WebView*, and therefore not appropriate for upstream
v8. But then he says it could/should be safe for v8 *embedded inside
NodeJS*.

Based on that, I suggest patching Debian's NodeJS with the patch to
adjust armhf/arm64 stack limit size to 984kb. With the caveat that the
javascript code that is triggering this bug should really be fixed to
not be so stack-intensive, of course. Perhaps this bug cloned at a
lower severity, filed against those packages that this bug is affecting?

(As chromium maintainer, which also embeds v8, I haven't heard of any
issues and hadn't planned on touching stacks limits. It sure would be
nice to have just one copy of V8 in the archive, though..)

[Thorsten Glaser]

James Addison dixit:

>On Thu, 11 May 2023 at 02:43, Andres Salomon <dili...@queued.net> wrote:

>> For ARM64, he says that raising the stack limit is not safe for v8
>> *embedded inside WebView*, and therefore not appropriate for upstream
>> v8. But then he says it could/should be safe for v8 *embedded inside
>> NodeJS*.
>>
>> Based on that, I suggest patching Debian's NodeJS with the patch to
>> adjust armhf/arm64 stack limit size

That would be a good thing (huh, wasn’t armhf good?), but…

>I have a question: if we apply the patch and begin using the same
>constant stack size of 984kb on 32-bit ARM and 64-bit ARM as is
>defined for other architectures, then does NodeJS on those platforms
>begin supporting exactly the same stack frame capacity (maximum call
>depth for any given recursive function, for example) as a build of the
>same NodeJS source on x86 and amd64 respectively?

… no, because both stack usage and other stuff on stack differ.

Which is why I’d rather have the getrlimit-based one for nodejs.
That would give us twice to four times the limit.

>> (As chromium maintainer, which also embeds v8, I haven't heard of any
>> issues and hadn't planned on touching stacks limits. It sure would be

Yes, yes, definitely don’t change it outside of nodejs.

>> javascript code that is triggering this bug should really be fixed to
>> not be so stack-intensive, of course. Perhaps this bug cloned at a
>> lower severity, filed against those packages that this bug is affecting?

That’s got a dependency chain so long that don’t hold your breath
for ALL those to be changed is true. Besides, how would the
respective maintainers be aware of this in the first place?

Short of failing with an arbitrary nesting limit on all arches,
in upstream nodejs, I doubt you’d get half of the upstream code
maintainers to care… if they even care about their code any more
at all. I am not a friend of such limits either, in addition.

bye,
//mirabilos
--
Solange man keine schmutzigen Tricks macht, und ich meine *wirklich*
schmutzige Tricks, wie bei einer doppelt verketteten Liste beide
Pointer XORen und in nur einem Word speichern, funktioniert Boehm ganz
hervorragend. -- Andreas Bogk über boehm-gc in d.a.s.r

[Thorsten Glaser]

James Addison dixit:

>I'm going to stay involved with this thread, but I think that it is
>upon you to develop or provide further guidance towards a patch if
>it's something you'd like to have implemented, Thorsten.

I actually have looked into that but I don’t understand the nodejs
and v8 source code enough to be able. I know C, but not CFrustFrust.
I would rather prefer asm…

bye,
//mirabilos
--
When he found out that the m68k port was in a pretty bad shape, he did
not, like many before him, shrug and move on; instead, he took it upon
himself to start compiling things, just so he could compile his shell.
How's that for dedication. -- Wouter, about my Debian/m68k revival

[James Addison]

On Sat, 13 May 2023 at 02:18, Thorsten Glaser <t...@debian.org> wrote:
>
> James Addison dixit:
>
> >I'm going to stay involved with this thread, but I think that it is
> >upon you to develop or provide further guidance towards a patch if
> >it's something you'd like to have implemented, Thorsten.
>
> I actually have looked into that but I don’t understand the nodejs
> and v8 source code enough to be able. I know C, but not CFrustFrust.
> I would rather prefer asm…

Ok, thanks. We may be stalled temporarily in that case.

On Sat, 13 May 2023 at 00:20, James Addison <j...@jp-hosting.net> wrote:
> That said: perhaps it could be useful if someone could check whether
> the following commit is relevant to this:
> https://github.com/libuv/libuv/commit/18c7530a75d813801f819caae4dff47fc4a1d4a1

I ran the repro case (with some simplifications) from the GitHub
thread using 'strace' and grepped for rlimit-related syscalls:

# on arm64, this currently replicates the problem using debian bookworm
$ strace babeljs --config-file $PWD/babel.config.json --compact
false --source-maps inline -d tests5 auto_tests 2>&1 | grep -i rlimit

All of the resulting calls (on an ARM64 host) are to the 'prlimit64'
syscall and have a zero exit-code (success).

[James Addison]

Followup-For: Bug #1030284
X-Debbugs-Cc: t...@mirbsd.de

Thanks, Thorsten. I'm currently rebuilding (on x86) from the attached patch,
adapted from yours.

* prefers a one-time repeat division over the clever-but-fragile div-assign

* removes the upperbound check because integer greater-than checks can be
problematic

* places comparison constants on the lhs for safety

I'll post test results when they are available.

Cheers,
James

[James Addison]

Followup-For: Bug #1030284
X-Debbugs-Cc: t...@mirbsd.de

It does seem to (continue to) function, at least on x86:

~/dygraphs-2.2.0$ NODE_PATH=/usr/share/nodejs ../nodejs-18.13.0+dfsg1/out/Release/node /usr/bin/babeljs --config-file $PWD/babel.config.json --compact false --source-maps inline -d tests5 auto_tests
Successfully compiled 59 files with Babel (2744ms).

(NOTE: this was an x86 system, not an ARM64 system)


And the relevant v8 help text output appears to update accordingly:

~/dygraphs-2.2.0$ ../nodejs-18.13.0+dfsg1/out/Release/node --v8-options | grep -i stack-size
--stack-size (default size of stack region v8 is allowed to use (in kBytes))
type: int default: --stack-size=8192