Skip to content

Commit

Permalink
Track blob executable bit.
Browse files Browse the repository at this point in the history
Fixes #34
  • Loading branch information
jsirois committed Nov 17, 2022
1 parent e6223f8 commit 31a4e27
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 28 deletions.
6 changes: 6 additions & 0 deletions jump/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,9 @@ pub struct File {
#[serde(default, rename = "type")]
pub file_type: Option<FileType>,
#[serde(default)]
#[serde(skip_serializing_if = "Option::is_none")]
pub executable: Option<bool>,

This comment has been minimized.

Copy link
@benjyw

benjyw Nov 17, 2022

Collaborator

Why does this need to be tri-state and not just an unwrapped bool?

This comment has been minimized.

Copy link
@jsirois

jsirois Nov 17, 2022

Author Collaborator

This was driven solely by the cat example comparison test against the boot-pack for a byte-for-byte identical result, which failed without it. The idea is in the ser-de round trip, only true should be written down, false is implied for everything else. When writing a lift manifest, IOW, you don't write "executable": false (although you could to handle some strange corner). As such,, for round tripping, you don't want the default bool of false to serialize.

#[serde(default)]
#[serde(skip_serializing_if = "is_false")]
pub eager_extract: bool,
#[serde(default)]
Expand Down Expand Up @@ -378,6 +381,7 @@ mod tests {
size: Some(1137),
hash: Some("abc".to_string()),
file_type: Some(FileType::Blob),
executable: Some(true),
eager_extract: true,
source: None,
},
Expand All @@ -389,6 +393,7 @@ mod tests {
file_type: Some(FileType::Archive(ArchiveType::CompressedTar(
Compression::Zstd
))),
executable: None,
eager_extract: false,
source: None,
},
Expand All @@ -398,6 +403,7 @@ mod tests {
size: Some(42),
hash: Some("def".to_string()),
file_type: Some(FileType::Archive(ArchiveType::Zip)),
executable: None,
eager_extract: false,
source: None,
}
Expand Down
70 changes: 42 additions & 28 deletions jump/src/installer.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2022 Science project contributors.
// Licensed under the Apache License, Version 2.0 (see LICENSE).

use std::fs::OpenOptions;
use std::fs::{OpenOptions, Permissions};
use std::io::{Cursor, Read, Seek, SeekFrom};
use std::path::Path;

Expand Down Expand Up @@ -95,8 +95,20 @@ where
})
}

#[cfg(not(target_family = "unix"))]
fn executable_permissions() -> Option<Permissions> {
None
}

#[cfg(target_family = "unix")]
fn executable_permissions() -> Option<Permissions> {
use std::os::unix::fs::PermissionsExt;
Some(Permissions::from_mode(0o755))

This comment has been minimized.

Copy link
@benjyw

benjyw Nov 17, 2022

Collaborator

We're assuming world-readable is OK, because this is all happening under a user homedir, so we're relying on the permissions on the parent dir to prevent world access?

This comment has been minimized.

Copy link
@jsirois

jsirois Nov 17, 2022

Author Collaborator

I honestly hadn't thought this through at all. Taking your tack though, that's not a valid assumption anyhow since ~/.nce is only the default, this can be changed to be anything. In fact, #9 contemplates the case where scies are used to install system binaries in /usr/local/nce; so this may be a poor choice as you sniffed out. I've filed #36 to think about this more deeply.

}

#[time("debug")]
fn unpack_blob<R: Read + Seek, T, F>(
executable: bool,
bytes_source: F,
expected_hash: &str,
dst: &Path,
Expand All @@ -117,6 +129,16 @@ where
blob_dst = blob_dst.display()
)
})?;
if executable {
if let Some(permissions) = executable_permissions() {
blob_out.set_permissions(permissions).map_err(|e| {
format!(
"Failed to set executable premissions on {dst}: {e}",
dst = dst.display()
)
})?;
}
}
std::io::copy(&mut hashed_bytes, &mut blob_out)
.map(|_| ())
.map_err(|e| format!("{e}"))?;
Expand All @@ -126,6 +148,7 @@ where

fn unpack<R: Read + Seek, T, F>(
file_type: FileType,
executable: bool,
bytes: F,
expected_hash: &str,
dst: &Path,
Expand All @@ -135,7 +158,7 @@ where
{
match file_type {
FileType::Archive(archive_type) => unpack_archive(archive_type, bytes, expected_hash, dst),
FileType::Blob => unpack_blob(bytes, expected_hash, dst),
FileType::Blob => unpack_blob(executable, bytes, expected_hash, dst),
FileType::Directory => unpack_archive(ArchiveType::Zip, bytes, expected_hash, dst),
}
}
Expand All @@ -154,6 +177,7 @@ pub(crate) fn install(files: &[FileEntry], payload: &[u8]) -> Result<(), String>
let bytes = &payload[location..(location + file.size)];
unpack(
file.file_type,
file.executable.unwrap_or(false),
|| Ok((Cursor::new(bytes), ())),
file.hash.as_str(),
dst,
Expand Down Expand Up @@ -182,9 +206,13 @@ pub(crate) fn install(files: &[FileEntry], payload: &[u8]) -> Result<(), String>
})?;
Ok((buffer, child))
};
if let Some(mut child) =
unpack(file.file_type, buffer_source, file.hash.as_str(), dst)?
{
if let Some(mut child) = unpack(
file.file_type,
file.executable.unwrap_or(false),
buffer_source,
file.hash.as_str(),
dst,
)? {
let exit_status = child.wait().map_err(|e| format!("{e}"))?;
if !exit_status.success() {
return Err(format!("Failed to load file {file:?}: {exit_status:?}"));
Expand All @@ -202,6 +230,7 @@ pub(crate) fn install(files: &[FileEntry], payload: &[u8]) -> Result<(), String>
let bytes = &payload[location..(location + tote_file.size)];
unpack(
tote_file.file_type,
tote_file.executable.unwrap_or(false),
|| Ok((Cursor::new(bytes), ())),
tote_file.hash.as_str(),
&scie_tote_path,
Expand All @@ -215,30 +244,15 @@ pub(crate) fn install(files: &[FileEntry], payload: &[u8]) -> Result<(), String>
src = src_path.display()
)
})?;
let permissions = file
.metadata()
.map_err(|e| {
format!(
"Failed to read permissions of {src_path}: {e}",
src_path = src_path.display()
)
})?
.permissions();
Ok((file, permissions))
Ok((file, ()))
};
if let Some(permissions) =
unpack(file.file_type, scie_tote_src, file.hash.as_str(), dst)?
{
// We let archives handle their internally stored permission bits.
if file.file_type == FileType::Blob {
std::fs::set_permissions(dst, permissions).map_err(|e| {
format!(
"Failed to set permissions of {dst}: {e}",
dst = dst.display()
)
})?;
}
}
unpack(
file.file_type,
file.executable.unwrap_or(false),
scie_tote_src,
file.hash.as_str(),
dst,
)?;
}
tote_file.size
}
Expand Down
28 changes: 28 additions & 0 deletions jump/src/lift.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub struct File {
pub size: usize,
pub hash: String,
pub file_type: FileType,
pub executable: Option<bool>,
pub eager_extract: bool,
pub source: Source,
}
Expand All @@ -37,6 +38,7 @@ impl From<File> for crate::config::File {
},
hash: Some(value.hash),
file_type: Some(value.file_type),
executable: value.executable,
eager_extract: value.eager_extract,
source: match value.source {
Source::Scie => None,
Expand Down Expand Up @@ -128,6 +130,23 @@ fn determine_file_type(path: &Path) -> Result<FileType, String> {
))
}

#[cfg(not(target_family = "unix"))]
fn is_executable(_path: &Path) -> Result<bool, String> {
Ok(false)
}

#[cfg(target_family = "unix")]
fn is_executable(path: &Path) -> Result<bool, String> {
use std::os::unix::fs::PermissionsExt;
let metadata = path.metadata().map_err(|e| {
format!(
"Failed to read metadata for {path}: {e}",
path = path.display()
)
})?;
Ok(metadata.permissions().mode() & 0o111 != 0)
}

#[time("debug")]
fn assemble(
resolve_base: &Path,
Expand Down Expand Up @@ -169,12 +188,21 @@ fn assemble(
}
};

let executable = if let Some(executable) = file.executable {
Some(executable)
} else if reconstitute && path.is_file() && is_executable(&path)? {
Some(true)
} else {
None
};

files.push(File {
name: file.name,
key: file.key,
size,
hash,
file_type,
executable,
eager_extract: file.eager_extract,
source: match file.source {
None => Source::Scie,
Expand Down
1 change: 1 addition & 0 deletions src/boot/pack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ fn pack(
size,
hash,
file_type: FileType::Archive(ArchiveType::Zip),
executable: None,
eager_extract: false,
source: Source::Scie,
};
Expand Down

0 comments on commit 31a4e27

Please sign in to comment.