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

Bazel sets all input files as executable when actions are executed remotely #12818

Closed
vsiva opened this issue Jan 13, 2021 · 5 comments
Closed
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug

Comments

@vsiva
Copy link

vsiva commented Jan 13, 2021

Description of the problem / feature request:

When used with RBE (or presumably any remote execution solution), all input files have executable permissions set.
Our product is a desktop application that is built using RBE, and only certain files should have exe permissions, not all. This issue prevents us from using RBE to build the final artifacts.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

$ ls -lht
total 4.0K
-rw-r----- 1 vsiva primarygroup 231 Jan 12 16:08 BUILD
-rwxr-x--- 1 vsiva primarygroup   0 Jan 12 16:06 bar.exe
-rw-r----- 1 vsiva primarygroup   0 Jan 12 16:06 foo.txt

$ cat BUILD 
genrule(
        name = "genzip",
        srcs = ["foo.txt", "bar.exe"],
        tools = ["@bazel_tools//tools/zip:zipper"],
        outs = ["foobar.zip"],
        cmd = "ls -l $(SRCS); $(location @bazel_tools//tools/zip:zipper) c $@ $(SRCS)",
)

$ bazel build :genzip

What's the output of bazel info release?

3.1.0

Any other information, logs, or outputs that you want to share?

https:/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/remote/merkletree/MerkleTree.java#L201 has:

 private static FileNode buildProto(DirectoryTree.FileNode file) {
    return FileNode.newBuilder()
        .setName(file.getPathSegment())
        .setDigest(file.getDigest())
        .setIsExecutable(true)
        .build();
  }

The issue seems to be with the setIsExecutable call.

@vsiva
Copy link
Author

vsiva commented Jan 13, 2021

This is probably an unintended side effect of the fix to #4751

@coeuvre coeuvre added team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug P1 I'll work on this now. (Assignee required) labels Jan 13, 2021
@coeuvre coeuvre self-assigned this Jan 13, 2021
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 13, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
@ruiqimao
Copy link

Thanks for the update! I've tested the latest patch, and it does set the executable bit correctly now.

I ran a test on a couple of files that have various permissions:

-r-xrw---- 1 ruiqimao primarygroup    0 Jan 13 16:32 560.txt
-rw-r----- 1 ruiqimao primarygroup    0 Jan 12 15:51 640.txt
-rw-rw---- 1 ruiqimao primarygroup    0 Jan 12 15:53 660.txt
-rwxr----- 1 ruiqimao primarygroup    0 Jan 12 15:53 740.txt
-rwxrwx--- 1 ruiqimao primarygroup    0 Jan 12 15:53 770.txt

After running remotely, the permissions changed to:

-rwxr-xr-x 1 nobody nogroup    0 Jan 14 00:32 560.txt
-rw-r--r-- 1 nobody nogroup    0 Jan 14 00:32 640.txt
-rw-r--r-- 1 nobody nogroup    0 Jan 14 00:32 660.txt
-rwxr-xr-x 1 nobody nogroup    0 Jan 14 00:32 740.txt
-rwxr-xr-x 1 nobody nogroup    0 Jan 14 00:32 770.txt
  • 560.txt has an extra u+w permission.
  • 660.txt and 770.txt are missing g+w permission.
  • All files have an extra o+r permission, and all the executable ones have extra go+x permissions.

Is it possible for the file permissions to reflect the original permissions rather than just adding the executable bit on top of 0644?

@coeuvre
Copy link
Member

coeuvre commented Jan 14, 2021

Thanks for sharing your test results. Unfortunately, we can't keep the original permissions since there is no field in FileNode to set the permissions.

A workaround could be embedding a script which is used to chmod artifacts with the desired permissions to the packaging action and run it before generating the final artifact.

coeuvre added a commit to coeuvre/bazel that referenced this issue Jan 18, 2021
When build without bytes is enabled, we use isExecutable field of OutputFile for intermediate input files. This is achieved by injecting the metadata into the MetadataProvider.

The "always mark" was introduced by 3e3b71a which was a workaround for bazelbuild#4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. bazelbuild#12818.
philwo pushed a commit that referenced this issue Mar 15, 2021
The "always mark" was introduced by 3e3b71a which was a workaround for #4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. #12818.

Fixes #12818.

Closes #12820.

PiperOrigin-RevId: 351940694
philwo pushed a commit that referenced this issue Mar 15, 2021
The "always mark" was introduced by 3e3b71a which was a workaround for #4751. However, that issue was then fixed by fc44891. There is no reason to keep the workaround which is causing other issues e.g. #12818.

Fixes #12818.

Closes #12820.

PiperOrigin-RevId: 351940694
@coeuvre
Copy link
Member

coeuvre commented Nov 6, 2021

Note that the fix was rolled back in d2b9428 due to regression #13262.

The general idea is, if the outputs of an action depend on the permission bits of its inputs, we should either force the action running locally or explicitly change the permission of inputs during execution to make the action hermitic.

@thesayyn
Copy link
Contributor

Thanks for sharing your test results. Unfortunately, we can't keep the original permissions since there is no field in FileNode to set the permissions.

A workaround could be embedding a script which is used to chmod artifacts with the desired permissions to the packaging action and run it before generating the final artifact.

This is really a foot gun when producing bit-by-bit reproducible tar files. As permissions are part of the tar files, it is impossible to get an identical output when running on RBE vs Local. are there any plans to implement this functionality?

Currently seem like all RBE implementations are free to choose whatever permission bits they like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants