Skip to content

Commit

Permalink
fix: avoid stack overflow with spread operator
Browse files Browse the repository at this point in the history
A customer cannot scan their project with a supposedly very large
dependency graph because as soon as it reaches our back-end,
we get a stack overflow error. It was identified that the error is
due to the combination of Array.push() and the spread operator.

For reference: nodejs/node#16870

To reproduce, create an array of size 2^17 then do [].push(...myArray);

This fix replaces all uses of push() and spread with a simple loop.
  • Loading branch information
ivanstanev authored and darscan committed Oct 24, 2019
1 parent fd8b897 commit 0bd6afb
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 2 deletions.
9 changes: 7 additions & 2 deletions src/core/dep-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ class DepGraphImpl implements types.DepGraphInternal {

const pathsToRoot: types.PkgInfo[][] = [];
for (const id of this.getPkgNodeIds(pkg)) {
pathsToRoot.push(...this.pathsFromNodeToRoot(id));
const paths = this.pathsFromNodeToRoot(id);
for (const path of paths) {
pathsToRoot.push(path);
}
}
// note: sorting to get shorter paths first -
// it's nicer - and better resembles older behaviour
Expand Down Expand Up @@ -281,7 +284,9 @@ class DepGraphImpl implements types.DepGraphInternal {
const out = this.pathsFromNodeToRoot(id).map((path) => {
return [this.getNodePkg(nodeId)].concat(path);
});
allPaths.push(...out);
for (const path of out) {
allPaths.push(path);
}
});

return allPaths;
Expand Down
36 changes: 36 additions & 0 deletions test/core/stress.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as depGraphLib from '../../src';

const dependencyName = 'needle';

async function generateLargeGraph(width: number) {
const builder = new depGraphLib.DepGraphBuilder(
{ name: 'npm' },
{ name: 'root', version: '1.2.3' },
);
const rootNodeId = 'root-node';

const deepDependency = { name: dependencyName, version: '1.2.3' };

builder.addPkgNode(deepDependency, dependencyName);
builder.connectDep(rootNodeId, dependencyName);

for (let j = 0; j < width; j++) {
const shallowName = `id-${j}`;
const shallowDependency = { name: shallowName, version: '1.2.3' };

builder.addPkgNode(shallowDependency, shallowName);
builder.connectDep(rootNodeId, shallowName);
builder.connectDep(shallowName, dependencyName);
}

return builder.build();
}

describe('stress tests', () => {
test('pkgPathsToRoot() does not cause stack overflow for large dep-graphs', async () => {
const graph = await generateLargeGraph(125000);

const result = graph.pkgPathsToRoot({ name: dependencyName, version: '1.2.3' });
expect(result).toBeDefined();
});
});

0 comments on commit 0bd6afb

Please sign in to comment.