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

Crashes when using ajv on Alpine Linux #11991

Closed
daveisfera opened this issue Mar 22, 2017 · 21 comments
Closed

Crashes when using ajv on Alpine Linux #11991

daveisfera opened this issue Mar 22, 2017 · 21 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@daveisfera
Copy link

  • Version:
    Happens on 4.x, 6.x and 7.x.

  • Platform:
    Alpine Linux 3.4

  • Subsystem:
    v8

There's an issue open for docker-node here and it has a backtrace from gdb.

Running this script reproduces the crash:

#!/usr/bin/env node
'use strict';

const Ajv = require('ajv');
const validator = new Ajv({ allErrors: true, extendedRefs: false });

const STRING_KEY = {
    id: '/StringKey',
    type: 'string',
    maxLength: 10000,
};

validator.addSchema(STRING_KEY);

let schema = {
    type: 'object',
    properties: {},
};

const NUM_COLUMNS = parseInt(process.argv[2] || '81');
console.log(`Testing with ${NUM_COLUMNS} columns`);
let c;
for (c=0; c<NUM_COLUMNS; c++) {
    schema.properties[`s${c}`] = { $ref: '/StringKey'};
}
console.log('schema:', schema);
const validate = validator.compile(schema);

let value = {};
for (c=0; c<NUM_COLUMNS; c++) {
    const cS = `s${c}`;
    value[cS] = '';
}
console.log('value:', value);

const NUM_ROWS = parseInt(process.argv[3] || '394');
console.log(`Testing with ${NUM_ROWS} rows`);
let r;
for (r=0; r<NUM_ROWS; r++) {
    validate(value);
}

console.log('Done');

I used this Dockerfile to run the test (also crashes when using FROM node:4.8.0-alpine and FROM node:7.7.3-alpine:

FROM node:6.10.0-alpine

RUN mkdir -p /usr/src/app
WORKDIR /usr/src/app

RUN yarn add ajv

COPY test_ajv.js /usr/src/app

CMD [ "node", "test_ajv.js" ]

And ran these commands:

docker build -t test_ajv .
docker run --rm -it test_ajv sh
node test_ajv.js
@mscdex
Copy link
Contributor

mscdex commented Mar 22, 2017

Backtrace:

#0  0x0000564f3d67e63e in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
#1  0x0000564f3d67e6da in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
#2  0x0000564f3d67e6da in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
...
#1287 0x0000564f3d67e6da in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
#1288 0x0000564f3d67e6da in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
#1289 0x0000564f3d67e6da in v8::internal::HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock(v8::internal::HBasicBlock*, v8::internal::HBasicBlock*) ()
#1290 0x0000564f3d68020c in v8::internal::HGlobalValueNumberingPhase::AnalyzeGraph() ()
#1291 0x0000564f3d6807dd in v8::internal::HGlobalValueNumberingPhase::Run() ()
#1292 0x0000564f3d6b13c5 in void v8::internal::HGraph::Run<v8::internal::HGlobalValueNumberingPhase>() ()
#1293 0x0000564f3d6be624 in v8::internal::HGraph::Optimize(v8::internal::BailoutReason*) ()
#1294 0x0000564f3d640aec in v8::internal::OptimizedCompileJob::OptimizeGraph() ()
#1295 0x0000564f3d8cc379 in v8::internal::OptimizingCompileDispatcher::CompileTask::Run() ()
#1296 0x0000564f3dc67329 in v8::platform::WorkerThread::Run() ()
#1297 0x0000564f3de923b0 in v8::base::ThreadEntry(void*) ()
#1298 0x00007f97cec3a655 in ?? () from /lib/ld-musl-x86_64.so.1
#1299 0x0000000000000000 in ?? ()

@mscdex
Copy link
Contributor

mscdex commented Mar 22, 2017

/cc @nodejs/v8

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Mar 22, 2017
@hashseed
Copy link
Member

Looks like Crankshaft is running into stack overflow. Turbofan likely won't have this issue. I'll take a look whether there is an easy fix here.

@hashseed
Copy link
Member

I can't actually reproduce it on my Ubuntu workstation. I installed ajv 1.3.10 via npm. The test case runs fine on v7.7.5-pre both release and debug mode, and on v7.7.3 debug mode.

I wonder whether Alpine Linux has different settings wrt stack limit. Maybe you can try with running with --no-use-gvn?

@daveisfera
Copy link
Author

I can't actually reproduce it on my Ubuntu workstation. I installed ajv 1.3.10 via npm. The test case runs fine on v7.7.5-pre both release and debug mode, and on v7.7.3 debug mode.

I can only reproduce the crash when running on Alpine Linux.

I wonder whether Alpine Linux has different settings wrt stack limit. Maybe you can try with running with --no-use-gvn?

It does not crash when I run with --no-use-gvn.

@daveisfera
Copy link
Author

I just tried with node 6.10.2 and it now crashes with a smaller number of columns (70) but the same number of rows is required (394).

@johan13
Copy link

johan13 commented Apr 17, 2017

I wonder whether Alpine Linux has different settings wrt stack limit.

Alpine Linux uses musl libc which has a default stack size of just 80 kB. Glibc for example has a default stack size of 8 MB.

@hashseed
Copy link
Member

80kb sounds very limited. The correct fix would be to add stack checks to Crankshaft's GVN, but Crankshaft in upstream V8 is no longer under development.

@johan13
Copy link

johan13 commented Apr 17, 2017

It doesn't crash if you use the Turbofan compiler.
node --turbo test_ajv.js

@bnoordhuis
Copy link
Member

Does node --stack_size=$kb test_ajv.js, where $kb is a value < 80, work? If yes, I suppose we could detect RLIMIT_STACK and adjust --stack_size accordingly.

Is the limit set my musl or alpine? I don't see anything in musl that suggests it modifies RLIMIT_STACK.

@hashseed
Copy link
Member

@bnoordhuis that would be a good idea generally, but wouldn't help this case since the recursion in GVN doesn't perform stack checks. It would prevent crashes where we do though.

@bnoordhuis
Copy link
Member

Good point. I think we could float a patch that adds stack checks in the appropriate places or make it non-recursive. CollectSideEffectsOnPathsToDominatedBlock appears to be the main offender.

@johan13
Copy link

johan13 commented Apr 17, 2017

@bnoordhuis
Copy link
Member

@johan13 Can you try #12460?

the limit is in musl: https://anonscm.debian.org/cgit/collab-maint/musl.git/tree/src/internal/pthread_impl.h#n144

Ah, I understand now - I was looking at execve() and friends but it's a per-thread setting. In that case --noconcurrent_recompilation can probably be used as a workaround too, depending on the RLIMIT_STACK of the main thread (which hopefully is bigger.)

@johan13
Copy link

johan13 commented Apr 17, 2017

Yes, --noconcurrent_recompilation seems to do the trick as well.

I have never built node from source and I don't have a system with musl and build tools installed, so it would probably take me a little while to try the pull request.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 21, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs#11991
PR-URL: nodejs#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
@bnoordhuis
Copy link
Member

Fixed in 30989d3 and coming to a release near you soon.

@daveisfera
Copy link
Author

Awesome! Thanks for putting that fix in, but are there any other concerns about the stack being smaller with Alpine?

@bnoordhuis
Copy link
Member

80 kB is on the slim side but it should be sufficient. Just file new issues if you hit more bugs and we'll take a look.

cc @jbergstroem - didn't you mention an alpine buildbot a while ago?

targos pushed a commit that referenced this issue Apr 24, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit that referenced this issue Apr 26, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit that referenced this issue Apr 27, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit that referenced this issue Apr 28, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
rvagg pushed a commit that referenced this issue Jun 29, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 10, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs/node#11991
PR-URL: nodejs/node#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
targos pushed a commit to targos/node that referenced this issue Jul 21, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs#11991
PR-URL: nodejs#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
addaleax pushed a commit that referenced this issue Jul 24, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Aug 1, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs#11991
PR-URL: nodejs#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Aug 1, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs#11991
Refs: nodejs#12460

PR-URL: nodejs#14004
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Aug 1, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

PR-URL: #14004
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit that referenced this issue Aug 2, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <[email protected]>
Backport-Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: #14004
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Backport-PR-URL: #13080
PR-URL: #12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
gibfahn pushed a commit to ibmruntimes/node that referenced this issue Nov 24, 2017
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: nodejs/node#11991
Backport-PR-URL: nodejs/node#13080
PR-URL: nodejs/node#12460
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yang Guo <[email protected]>
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.
Projects
None yet
Development

No branches or pull requests

5 participants