Skip to content

Commit

Permalink
fix(assertions): stack overflow while parsing template (#26767)
Browse files Browse the repository at this point in the history
Closes #26766

The function `findCycle` tries to find a cycle by using a depth-first search (DFS). The DFS is implemented recursively in the recurse function. For each node, it tries to find a path that eventually leads back to the start of the path. If such a path is found, a cycle exists, and the nodes forming this cycle are returned.

One of the bugs in the current implementation is that it only checks whether the current dependency `dep` is equal to the first node of the current path `path[0]`. This means it will only detect a cycle if the cycle includes the first node of the search, which might not always be the case.

To fix this, the function should check whether the current dependency `dep` is already somewhere in the current path `path`. If it is, then a cycle has been found.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
Chriscbr authored Aug 18, 2023
1 parent dbe5615 commit 01a7b5b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/assertions/lib/private/cyclic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,12 @@ function findCycle(deps: ReadonlyMap<string, ReadonlySet<string>>): string[] {

function recurse(node: string, path: string[]): string[] | undefined {
for (const dep of deps.get(node) ?? []) {
if (dep === path[0]) { return [...path, dep]; }
if (path.includes(dep)) { return [...path, dep]; }

const cycle = recurse(dep, [...path, dep]);
if (cycle) { return cycle; }
}

return undefined;
}
}
}
23 changes: 23 additions & 0 deletions packages/aws-cdk-lib/assertions/test/template.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,29 @@ describe('Template', () => {
}).toThrow(/dependency cycle/);
});

test('throws when given a more complex template with cyclic dependencies', () => {
expect(() => {
Template.fromJSON({
Resources: {
Res1: {
Type: 'Foo',
Properties: {
Thing: { Ref: 'Res2' },
},
},
Res2: {
Type: 'Foo',
DependsOn: ['Res3'],
},
Res3: {
Type: 'Foo',
DependsOn: ['Res2'],
},
},
});
}).toThrow(/dependency cycle/);
});

test('does not throw when given a template with cyclic dependencies if check is skipped', () => {
expect(() => {
Template.fromJSON({
Expand Down

0 comments on commit 01a7b5b

Please sign in to comment.