From bf2fe88a5c0c7f778502f87ddacb5072919d455e Mon Sep 17 00:00:00 2001
From: Florian Klink <flokli@flokli.de>
Date: Sun, 17 Sep 2023 14:36:15 +0300
Subject: [PATCH] fix(tvix/store/fuse): fix executable bit

We were blindly returning 0o444 for all regular files, but regular files
with executable bit need to be 0o555.

This wasn't spotted because stat'ing executable files was not part of
the test suite, it's now added.

Change-Id: I04c69784053e7e43d838c01bb288f2df48f40b4e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9345
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: Connor Brewster <cbrewster@hey.com>
---
 tvix/store/src/fuse/file_attr.rs |  3 ++-
 tvix/store/src/fuse/tests.rs     | 36 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tvix/store/src/fuse/file_attr.rs b/tvix/store/src/fuse/file_attr.rs
index b2b971d9e..25cfd28dd 100644
--- a/tvix/store/src/fuse/file_attr.rs
+++ b/tvix/store/src/fuse/file_attr.rs
@@ -43,7 +43,8 @@ pub fn gen_file_attr(inode_data: &InodeData, inode: u64) -> FileAttr {
         crtime: SystemTime::UNIX_EPOCH,
         kind: inode_data.into(),
         perm: match inode_data {
-            InodeData::Regular(..) => 0o444,
+            InodeData::Regular(_, _, false) => 0o444, // no-executable files
+            InodeData::Regular(_, _, true) => 0o555,  // executable files
             InodeData::Symlink(_) => 0o444,
             InodeData::Directory(..) => 0o555,
         },
diff --git a/tvix/store/src/fuse/tests.rs b/tvix/store/src/fuse/tests.rs
index 29856433b..015e27ee9 100644
--- a/tvix/store/src/fuse/tests.rs
+++ b/tvix/store/src/fuse/tests.rs
@@ -16,6 +16,7 @@ use crate::{proto, FUSE};
 
 const BLOB_A_NAME: &str = "00000000000000000000000000000000-test";
 const BLOB_B_NAME: &str = "55555555555555555555555555555555-test";
+const HELLOWORLD_BLOB_NAME: &str = "66666666666666666666666666666666-test";
 const SYMLINK_NAME: &str = "11111111111111111111111111111111-test";
 const SYMLINK_NAME2: &str = "44444444444444444444444444444444-test";
 const DIRECTORY_WITH_KEEP_NAME: &str = "22222222222222222222222222222222-test";
@@ -103,6 +104,37 @@ async fn populate_blob_b(
     path_info_service.put(path_info).expect("must succeed");
 }
 
+/// adds a blob containing helloworld and marks it as executable
+async fn populate_helloworld_blob(
+    blob_service: &Arc<dyn BlobService>,
+    _directory_service: &Arc<dyn DirectoryService>,
+    path_info_service: &Arc<dyn PathInfoService>,
+) {
+    // Upload BLOB_B
+    let mut bw = blob_service.open_write().await;
+    tokio::io::copy(
+        &mut Cursor::new(fixtures::HELLOWORLD_BLOB_CONTENTS.to_vec()),
+        &mut bw,
+    )
+    .await
+    .expect("must succeed uploading");
+    bw.close().await.expect("must succeed closing");
+
+    // Create a PathInfo for it
+    let path_info = PathInfo {
+        node: Some(proto::Node {
+            node: Some(proto::node::Node::File(FileNode {
+                name: HELLOWORLD_BLOB_NAME.into(),
+                digest: fixtures::HELLOWORLD_BLOB_DIGEST.clone().into(),
+                size: fixtures::HELLOWORLD_BLOB_CONTENTS.len() as u32,
+                executable: true,
+            })),
+        }),
+        ..Default::default()
+    };
+    path_info_service.put(path_info).expect("must succeed");
+}
+
 fn populate_symlink(
     _blob_service: &Arc<dyn BlobService>,
     _directory_service: &Arc<dyn DirectoryService>,
@@ -760,6 +792,7 @@ async fn check_attributes() {
     populate_blob_a(&blob_service, &directory_service, &path_info_service).await;
     populate_directory_with_keep(&blob_service, &directory_service, &path_info_service).await;
     populate_symlink(&blob_service, &directory_service, &path_info_service);
+    populate_helloworld_blob(&blob_service, &directory_service, &path_info_service).await;
 
     let fuser_session = do_mount(
         blob_service,
@@ -773,14 +806,17 @@ async fn check_attributes() {
     let p_file = tmpdir.path().join(BLOB_A_NAME);
     let p_directory = tmpdir.path().join(DIRECTORY_WITH_KEEP_NAME);
     let p_symlink = tmpdir.path().join(SYMLINK_NAME);
+    let p_executable_file = tmpdir.path().join(HELLOWORLD_BLOB_NAME);
 
     // peek at metadata. We use symlink_metadata to ensure we don't traverse a symlink by accident.
     let metadata_file = fs::symlink_metadata(&p_file).expect("must succeed");
+    let metadata_executable_file = fs::symlink_metadata(&p_executable_file).expect("must succeed");
     let metadata_directory = fs::symlink_metadata(&p_directory).expect("must succeed");
     let metadata_symlink = fs::symlink_metadata(&p_symlink).expect("must succeed");
 
     // modes should match. We & with 0o777 to remove any higher bits.
     assert_eq!(0o444, metadata_file.mode() & 0o777);
+    assert_eq!(0o555, metadata_executable_file.mode() & 0o777);
     assert_eq!(0o555, metadata_directory.mode() & 0o777);
     assert_eq!(0o444, metadata_symlink.mode() & 0o777);