Skip to content

fix: Handle when readlink -f goes out of BIN-DIR #1052

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ewianda
Copy link
Contributor

@ewianda ewianda commented Feb 27, 2025

Noticed that on MacOS readlink -f goes out the sandbox in some cases

Noticed that on MacOS readlink -f goes out the sandbox in some cases
@ewianda
Copy link
Contributor Author

ewianda commented Feb 27, 2025

@thesayyn Notice that the preserve symlink fails on MacOs for rules-python. Not sure how to repro this in CI.

@ewianda ewianda closed this Feb 27, 2025
@ewianda ewianda reopened this Feb 27, 2025
if (resolved_path == "") {
# 1. If readlink -f fails use readlink for relative links
# 2. Make sure readlink -f doesn't escape BIN_DIR
if (resolved_path == "" || !(resolved_path ~ bin_dir)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is wrong, we need to handle running out of bindir cases for two reasons;

  • Execution strategies on macos use symlinks, there readlink might go out of sandbox, so we need to find things relative to execroot
  • When running the action with no sandbox. eg tags = ["local"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, is it the checking logic that is wrong, or the reasons stated for the check wrong

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its insufficent is the problem. This reference implementation that correctly detects if a file is a symlink under most circumstances
https://github.com/aspect-build/rules_js/blob/1c1823d605578d7266fad8b5410d0aae6b9bfc6d/js/private/js_image_layer.mjs#L55-L90

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following on this, so the right logic for this would be, we follow the symlink only one level, check if its out of the bindir, and if it is then its not a symlink that stays within the output tree therefore an implementation detail symlink.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants