Skip to content

Commit

Permalink
Follow symlinks for getDigest and getFastDigest.
Browse files Browse the repository at this point in the history 
The methods are documented as such in FileSystem. If we don't do this, there will be a discrepancy between getFastDigest and stat, as the latter can follow symlinks. This can manifest as a crash (see bazelbuild#20246) as the digest computation will take the missing fast digest for a symlink as a signal to compute the digest manually; this would fail when the symlink target is an in-memory file, which doesn't have an associated inode as required to compute the cache key (see DigestUtils#manuallyComputeDigest).

Fixes bazelbuild#20246.

PiperOrigin-RevId: 584297990
Change-Id: I65e586ea84635a279208e24c421f54ae46ee21b8
  • Loading branch information
@tjgq @bazel-io
tjgq authored and bazel-io committed Nov 21, 2023
1 parent e2249f9 commit e33fb44
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 25 deletions.
15 changes: 11 additions & 4 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,16 +349,22 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)

@Override
protected byte[] getFastDigest(PathFragment path) throws IOException {
var stat = statInMemory(path, FollowMode.FOLLOW_ALL);
if (stat instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) stat).getDigest();
path = resolveSymbolicLinks(path).asFragment();
// The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
// effectively the same as FOLLOW_PARENT, but more efficient.
var status = statInMemory(path, FollowMode.FOLLOW_NONE);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
return localFs.getPath(path).getFastDigest();
}

@Override
protected byte[] getDigest(PathFragment path) throws IOException {
var status = statInMemory(path, FollowMode.FOLLOW_ALL);
path = resolveSymbolicLinks(path).asFragment();
// The parent path has already been canonicalized by resolveSymbolicLinks, so FOLLOW_NONE is
// effectively the same as FOLLOW_PARENT, but more efficient.
var status = statInMemory(path, FollowMode.FOLLOW_NONE);
if (status instanceof FileStatusWithDigest) {
return ((FileStatusWithDigest) status).getDigest();
}
Expand Down Expand Up @@ -566,6 +572,7 @@ private FileStatus statUnchecked(PathFragment path, FollowMode followMode) throw
if (status != null) {
return status;
}

// The path has already been canonicalized above.
return localFs.getPath(path).stat(Symlinks.NOFOLLOW);
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/testing/vfs/SpiedFileSystem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ public boolean createWritableDirectory(PathFragment path) throws IOException {
return super.createWritableDirectory(path);
}

@Override
public byte[] getDigest(PathFragment path) throws IOException {
return super.getDigest(path);
}

@Override
public void chmod(PathFragment path, int mode) throws IOException {
super.chmod(path, mode);
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/runtime/commands",
"//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/testing/vfs:spied_filesystem",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
"//src/main/java/com/google/devtools/build/lib/util:exit_code",
"//src/main/java/com/google/devtools/build/lib/util:os",
Expand Down
129 changes: 108 additions & 21 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

import com.google.common.base.Utf8;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.hash.HashCode;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.actions.ActionExecutionMetadata;
import com.google.devtools.build.lib.actions.ActionInput;
Expand All @@ -50,7 +52,7 @@
import com.google.devtools.build.lib.clock.JavaClock;
import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.testing.vfs.SpiedFileSystem;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
Expand All @@ -62,7 +64,6 @@
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
Expand All @@ -81,12 +82,10 @@ public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTest
new RemoteOutputChecker(
new JavaClock(), "build", RemoteOutputsMode.MINIMAL, ImmutableList.of());

private static final DigestHashFunction HASH_FUNCTION = DigestHashFunction.SHA256;

private static final String RELATIVE_OUTPUT_PATH = "out";

private final RemoteActionInputFetcher inputFetcher = mock(RemoteActionInputFetcher.class);
private final FileSystem fs = new InMemoryFileSystem(HASH_FUNCTION);
private final SpiedFileSystem fs = SpiedFileSystem.createInMemorySpy();
private final Path execRoot = fs.getPath("/exec");
private final ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(execRoot));
private final ArtifactRoot outputRoot =
Expand Down Expand Up @@ -461,6 +460,91 @@ public void statAndExists_notFound() throws Exception {
FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
}

@Test
public void getDigest_fromInputArtifactData_forLocalArtifact() throws Exception {
ActionInputMap inputs = new ActionInputMap(1);
Artifact artifact = createRemoteArtifact("file", "local contents", inputs);
PathFragment path = artifact.getPath().asFragment();
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);

// Verify that we don't fall back to a slow digest.
reset(fs);
assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("local contents"));
verify(fs, never()).getDigest(any());

assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
}

@Test
public void getDigest_fromInputArtifactData_forRemoteArtifact() throws Exception {
ActionInputMap inputs = new ActionInputMap(1);
Artifact artifact = createRemoteArtifact("file", "remote contents", inputs);
PathFragment path = artifact.getPath().asFragment();
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);

// Verify that we don't fall back to a slow digest.
reset(fs);
assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
verify(fs, never()).getDigest(any());

assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
}

@Test
public void getDigest_fromRemoteOutputTree() throws Exception {
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
PathFragment path = artifact.getPath().asFragment();
injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents");

assertThat(actionFs.getFastDigest(path)).isEqualTo(getDigest("remote contents"));
assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("remote contents"));
}

@Test
public void getDigest_fromLocalFilesystem() throws Exception {
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
PathFragment path = artifact.getPath().asFragment();
writeLocalFile(actionFs, artifact.getPath().asFragment(), "local contents");

assertThat(actionFs.getFastDigest(path)).isNull();
assertThat(actionFs.getDigest(path)).isEqualTo(getDigest("local contents"));
}

@Test
public void getDigest_notFound() throws Exception {
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
PathFragment path = artifact.getPath().asFragment();

assertThrows(FileNotFoundException.class, () -> actionFs.getFastDigest(path));
assertThrows(FileNotFoundException.class, () -> actionFs.getDigest(path));
}

@Test
public void getDigest_followSymlinks(
@TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
throws Exception {
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
FileSystem fromFs = from.getFilesystem(actionFs);
FileSystem toFs = to.getFilesystem(actionFs);

PathFragment linkPath = getOutputPath("sym");
PathFragment targetPath = getOutputPath("target");
fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());

if (toFs.equals(actionFs.getLocalFileSystem())) {
writeLocalFile(actionFs, targetPath, "content");
assertThat(actionFs.getFastDigest(linkPath)).isNull();
} else {
injectRemoteFile(actionFs, targetPath, "content");
assertThat(actionFs.getFastDigest(linkPath)).isEqualTo(getDigest("content"));
}

assertThat(actionFs.getDigest(linkPath)).isEqualTo(getDigest("content"));
}

@Test
public void readdir_fromRemoteOutputTree() throws Exception {
RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
Expand Down Expand Up @@ -943,16 +1027,12 @@ public void renameTo_onlyLocalFile_renameLocalFile() throws Exception {
@CanIgnoreReturnValue
protected FileArtifactValue injectRemoteFile(
FileSystem actionFs, PathFragment path, String content) throws IOException {
byte[] contentBytes = content.getBytes(UTF_8);
HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentBytes);
byte[] digest = getDigest(content);
int size = Utf8.encodedLength(content);
((RemoteActionFileSystem) actionFs)
.injectRemoteFile(
path, hashCode.asBytes(), contentBytes.length, /* expireAtEpochMilli= */ -1);
.injectRemoteFile(path, digest, size, /* expireAtEpochMilli= */ -1);
return RemoteFileArtifactValue.create(
hashCode.asBytes(),
contentBytes.length,
/* locationIndex= */ 1,
/* expireAtEpochMilli= */ -1);
digest, size, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1);
}

@Override
Expand All @@ -964,13 +1044,14 @@ protected void writeLocalFile(FileSystem actionFs, PathFragment path, String con

/** Returns a remote artifact and puts its metadata into the action input map. */
private Artifact createRemoteArtifact(
String pathFragment, String contents, ActionInputMap inputs) {
String pathFragment, String content, ActionInputMap inputs) {
Artifact a = ActionsTestUtil.createArtifact(outputRoot, pathFragment);
byte[] b = contents.getBytes(UTF_8);
HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
RemoteFileArtifactValue f =
RemoteFileArtifactValue.create(
h.asBytes(), b.length, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1);
getDigest(content),
Utf8.encodedLength(content),
/* locationIndex= */ 1,
/* expireAtEpochMilli= */ -1);
inputs.putWithNoDepOwner(a, f);
return a;
}
Expand All @@ -989,11 +1070,13 @@ private TreeArtifactValue createRemoteTreeArtifactValue(
TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(a);
for (Map.Entry<String, String> entry : contentMap.entrySet()) {
TreeFileArtifact child = TreeFileArtifact.createTreeOutput(a, entry.getKey());
byte[] b = entry.getValue().getBytes(UTF_8);
HashCode h = HASH_FUNCTION.getHashFunction().hashBytes(b);
String content = entry.getValue();
RemoteFileArtifactValue childMeta =
RemoteFileArtifactValue.create(
h.asBytes(), b.length, /* locationIndex= */ 0, /* expireAtEpochMilli= */ -1);
getDigest(content),
Utf8.encodedLength(content),
/* locationIndex= */ 0,
/* expireAtEpochMilli= */ -1);
builder.putChild(child, childMeta);
}
return builder.build();
Expand Down Expand Up @@ -1045,4 +1128,8 @@ private TreeArtifactValue createLocalTreeArtifactValue(
}
return builder.build();
}

private byte[] getDigest(String content) {
return fs.getDigestFunction().getHashFunction().hashString(content, UTF_8).asBytes();
}
}

0 comments on commit e33fb44

Please sign in to comment.