Force source files to be readable before copying them from sandbox.

Rules can (and have) accidentally made outputs unreadable, which makes the sandbox copying fail only when moving across devices. Unreadable outputs make no sense, so we just force them to be readable. RELNOTES: None. PiperOrigin-RevId: 363668244
katre · Jul 13, 2021 · 671e048 · 671e048
1 parent 451b296
commit 671e048
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 19 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -456,6 +456,17 @@ public static MoveResult moveFile(Path from, Path to) throws IOException {
         try (InputStream in = from.getInputStream();
             OutputStream out = to.getOutputStream()) {
           copyLargeBuffer(in, out);
+        } catch (FileAccessException e1) {
+          // Rules can accidentally make output non-readable, let's fix that (b/150963503)
+          if (!from.isReadable()) {
+            from.setReadable(true);
+            try (InputStream in = from.getInputStream();
+                OutputStream out = to.getOutputStream()) {
+              copyLargeBuffer(in, out);
+            }
+          } else {
+            throw e1;
+          }
         }
         to.setLastModifiedTime(stat.getLastModifiedTime()); // Preserve mtime.
         if (!from.isWritable()) {

diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
@@ -364,26 +364,11 @@ public void testMoveFile() throws IOException {
 
   @Test
   public void testMoveFileAcrossDevices() throws Exception {
-    class MultipleDeviceFS extends InMemoryFileSystem {
-      MultipleDeviceFS() {
-        super(DigestHashFunction.SHA256);
-      }
-
-      @Override
-      public void renameTo(Path source, Path target) throws IOException {
-        if (!source.startsWith(target.asFragment().subFragment(0, 1))) {
-          throw new IOException("EXDEV");
-        }
-        super.renameTo(source, target);
-      }
-    }
     FileSystem fs = new MultipleDeviceFS();
-    Path dev1 = fs.getPath("/fs1");
-    dev1.createDirectory();
-    Path dev2 = fs.getPath("/fs2");
-    dev2.createDirectory();
-    Path source = dev1.getChild("source");
-    Path target = dev2.getChild("target");
+    Path source = fs.getPath("/fs1/source");
+    source.getParentDirectory().createDirectoryAndParents();
+    Path target = fs.getPath("/fs2/target");
+    target.getParentDirectory().createDirectoryAndParents();
 
     FileSystemUtils.writeContent(source, UTF_8, "hello, world");
     source.setLastModifiedTime(142);
@@ -394,12 +379,34 @@ public void renameTo(Path source, Path target) throws IOException {
     assertThat(target.getLastModifiedTime()).isEqualTo(142);
 
     source.createSymbolicLink(PathFragment.create("link-target"));
+
     assertThat(FileSystemUtils.moveFile(source, target)).isEqualTo(MoveResult.FILE_COPIED);
+
     assertThat(source.exists(Symlinks.NOFOLLOW)).isFalse();
     assertThat(target.isSymbolicLink()).isTrue();
     assertThat(target.readSymbolicLink()).isEqualTo(PathFragment.create("link-target"));
   }
 
+  @Test
+  public void testMoveFileFixPermissions() throws Exception {
+    FileSystem fs = new MultipleDeviceFS();
+    Path source = fs.getPath("/fs1/source");
+    source.getParentDirectory().createDirectoryAndParents();
+    Path target = fs.getPath("/fs2/target");
+    target.getParentDirectory().createDirectoryAndParents();
+
+    FileSystemUtils.writeContent(source, UTF_8, "linear-a");
+    source.setLastModifiedTime(142);
+    source.setReadable(false);
+
+    MoveResult moveResult = moveFile(source, target);
+
+    assertThat(moveResult).isEqualTo(MoveResult.FILE_COPIED);
+    assertThat(source.exists(Symlinks.NOFOLLOW)).isFalse();
+    assertThat(target.isFile(Symlinks.NOFOLLOW)).isTrue();
+    assertThat(FileSystemUtils.readContent(target, UTF_8)).isEqualTo("linear-a");
+  }
+
   @Test
   public void testReadContentWithLimit() throws IOException {
     createTestDirectoryTree();
@@ -834,4 +841,18 @@ public void testCreateHardLinkForNonEmptyDirectory_success() throws Exception {
     assertThat(fileSystem.stat(linkPath3, false).getNodeId())
         .isEqualTo(fileSystem.stat(originalPath3, false).getNodeId());
   }
+
+  static class MultipleDeviceFS extends InMemoryFileSystem {
+    MultipleDeviceFS() {
+      super(DigestHashFunction.SHA256);
+    }
+
+    @Override
+    public void renameTo(Path source, Path target) throws IOException {
+      if (!source.startsWith(target.asFragment().subFragment(0, 1))) {
+        throw new IOException("EXDEV");
+      }
+      super.renameTo(source, target);
+    }
+  }
 }