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

tasks-runner component doesn't validate sock path #26936

Open
3 of 4 tasks
vergilfromadyen opened this issue Jul 15, 2024 · 6 comments
Open
3 of 4 tasks

tasks-runner component doesn't validate sock path #26936

vergilfromadyen opened this issue Jul 15, 2024 · 6 comments
Assignees
Labels
community This is a good first issue for contributing scope: core core nx functionality type: bug

Comments

@vergilfromadyen
Copy link
Contributor

vergilfromadyen commented Jul 15, 2024

Current Behavior

  • Setting NX_DAEMON_SOCKET_DIR to a path longer than 108 characters leads to an EADDRINUSE error
  • This can be triggered accidentally if a CI process sets TMPDIR for its own purposes

Expected Behavior

  • NX throws an error that explains the sock path is too long

Steps to Reproduce

  1. Set NX_DAEMON_SOCKET_DIR to something long, e.g. /tmp/xxxxxxxx-xxxxxxxxxxxxx-xxxxxxxxxx-xxxxxxxxxxxxx-xxxxxxxxxxx-xxxxxxxxxxxxxxx-xxxxxxxxxxxxxx-xxxxxxxxxxx
  2. Run any NX task

Operating System

  • macOS
  • Linux
  • Windows
  • Other (Please specify)

Additional Information

Linux has a 108-char path limit, Windows has a MAX_PATH constant which is 256ish characters.

Extracted from this issue and addresses a subset of the problems in there.

Fix should be placed around here.

@MaxKless
Copy link
Collaborator

Hey @vergilfromadyen thanks for the report and the detailed investigation. It seems reasonable to provide a better experience here... we already fall back to the workspace-folder socket dir if we can't access the socket dir.
Maybe it would be easiest to try again with that folder if we can't open a socket as well.

Otherwise a validation step might work as well though I'll have to research the proper limits.

@vergilfromadyen
Copy link
Contributor Author

Thanks @MaxKless for the quick response.

Setting NX_DAEMON_SOCKET_DIR to something generated can exceed the limits and break NX with an error that appears unrelated to NX. So falling back is nice but it's not enough because the OS tmpdir can be overridden so it can end up too long anyway. So this should probably fail with a more descriptive error.

To help your research:

  • MAX_PATH for the Windows API is 260 chars (source) - unless long paths are enabled. To be absolutely sure, process.env.MAX_PATH can be used on Windows
  • For MacOS and Linux it's 108 characters (for sock files only)

Let me know if I can help in any other way.

@MaxKless
Copy link
Collaborator

for sure but if it just fails, you'll have to set the env var anyways right. If both the temp dir and the workspace path are too long, you won't get around setting it manually.
So providing a fallback and better error message is probably the best we can do here. Unless you have another idea?

@vergilfromadyen
Copy link
Contributor Author

When throwing an error I'd still differentiate between a path potentially too long, and a generic "can't create sock file" message, because the former isn't really common knowledge. Other than that it sounds great.

@MaxKless
Copy link
Collaborator

If you'd be willing to contribute a fix, I think this shouldn't be too complicated and I'll gladly help guide you in the right direction. But seeing as you've already identified a place for a possible fix, you probably won't even need it :P

@MaxKless MaxKless added the community This is a good first issue for contributing label Jul 26, 2024
@vergilfromadyen
Copy link
Contributor Author

Sure, I'll make some time; you guys have a very cool product, might as well give back a bit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This is a good first issue for contributing scope: core core nx functionality type: bug
Projects
None yet
Development

No branches or pull requests

3 participants