From 1a35dc5b51f4e1f1076f80b8fbe8bde517c5aa7a 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 554383818541c9..5e96ab05bdc06e 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
@@ -457,6 +457,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 75596593c50dba..1cbbdebe426f1e 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
@@ -363,26 +363,11 @@ public void testMoveFile() throws IOException {
 
   @Test
   public void testMoveFileAcrossDevices() throws Exception {
-    class MultipleDeviceFS extends InMemoryFileSystem {
-      MultipleDeviceFS() {
-        super(DigestHashFunction.SHA256);
-      }
-
-      @Override
-      public void renameTo(PathFragment source, PathFragment target) throws IOException {
-        if (!source.startsWith(target.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);
@@ -393,12 +378,34 @@ public void renameTo(PathFragment source, PathFragment target) throws IOExceptio
     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();
@@ -830,4 +837,18 @@ public void testCreateHardLinkForNonEmptyDirectory_success() throws Exception {
     assertThat(fileSystem.stat(linkPath3.asFragment(), false).getNodeId())
         .isEqualTo(fileSystem.stat(originalPath3.asFragment(), false).getNodeId());
   }
+
+  static class MultipleDeviceFS extends InMemoryFileSystem {
+    MultipleDeviceFS() {
+      super(DigestHashFunction.SHA256);
+    }
+
+    @Override
+    public void renameTo(PathFragment source, PathFragment target) throws IOException {
+      if (!source.startsWith(target.subFragment(0, 1))) {
+        throw new IOException("EXDEV");
+      }
+      super.renameTo(source, target);
+    }
+  }
 }