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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/private/modify_mtree.awk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,9 @@ function make_relative_link(path1, path2, i, common, target, relative_path, back
cmd = "readlink -f \"" path "\""
cmd | getline resolved_path
close(cmd)
# If readlink -f fails use readlink for relative links
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.

cmd = "readlink \"" path "\""
cmd | getline resolved_path
close(cmd)
Expand Down