From 671e0489a5bd6d5abb4dcd9bcfc85134cee38385 Mon Sep 17 00:00:00 2001
From: larsrc <larsrc@google.com>
Date: Thu, 18 Mar 2021 08:56:04 -0700
Subject: [PATCH] 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
---
 .../build/lib/vfs/FileSystemUtils.java        | 11 ++++
 .../build/lib/vfs/FileSystemUtilsTest.java    | 59 +++++++++++++------
 2 files changed, 51 insertions(+), 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
index ec30e22d309b07..60158401881ff6 100644
--- 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
index 88e9a7de077fe7..c934a9e152a943 100644
--- 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);
+    }
+  }
 }