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

Failing assertion in topological sorting #25

Closed
andreaphylum opened this issue May 30, 2023 · 3 comments
Closed

Failing assertion in topological sorting #25

andreaphylum opened this issue May 30, 2023 · 3 comments
Assignees
Labels
bug Something isn't working high priority This should be addressed immediately

Comments

@andreaphylum
Copy link
Contributor

andreaphylum commented May 30, 2023

In the topological sorting function, the non-cyclicity of the dependency graph fails for some inputs.

We should investigate whether it is a genuine case of circular dependencies, there are issues with the way we load modules, or the sorting algorithm is incorrect.

How to reproduce

Run this file in the vuln-reach-cli. Adjust the tarballs = ... property if necessary.

cd vuln-reach/vuln-reach-cli
gunzip /path/to/fastify.toml.gz
cargo run --release -- fastify.toml
@andreaphylum andreaphylum added bug Something isn't working high priority This should be addressed immediately labels May 30, 2023
@andreaphylum
Copy link
Contributor Author

A few hypotheses:

  • Some packages vendor some of their dependencies in their releases (it's the case of mocha-qunit-ui). If those dependencies aren't properly accounted for in package.json, maybe there could be be gaps in the tree?
  • Some packages may have failing/unparseable modules that would normally import dependencies, but those being skipped leads to the imports not being recognized and thus missing edges in the tree. The QUnit example in Skip processing modules with parse errors #27 could be one of such cases.
  • Some packages may have correctly parseable modules that aren't actually part of the package -- say, template files or something similar (same QUnit example if the preamble were a whole, parseable script).
  • There may be some sort-of-circular dependencies, i.e. package A@2.0.0 depends on B@1.0.0 which depends on A@1.0.0. I'm not sure if this is supported by npm, but we don't have a way to support this case yet (see Support multiple versions of the same package in a tree #31) anyway.

@andreaphylum andreaphylum self-assigned this Jun 15, 2023
@andreaphylum
Copy link
Contributor Author

The algorithm is working correctly.

The following cycles were found in the dependency graph:

['@fastify/fast-json-stringify-compiler', 'fastify']
['@fastify/ajv-compiler', 'fastify', 'light-my-request']
['@fastify/ajv-compiler', 'fastify']
['babel-register', 'babel-core']

babel-register and babel-core were manually verified and indeed their respective package.json file reference each other. It is not an issue of multiple versions (see #31) because the cyclical import happens with the same version of both babel-register and babel-core.

The only way to solve this would be to understand what are the intended semantics for dependency cycles in npm and implement the same resolution algorithm.

@andreaphylum
Copy link
Contributor Author

Closing this in favor of #56. The assertion works as intended, but the overall approach is not conducive to working with dependency cycles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority This should be addressed immediately
Projects
None yet
Development

No branches or pull requests

1 participant