Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions cli/src/command/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use path_slash::*;
pub(crate) use path_transformer::PathTransformers;
use pna::{
Archive, EntryBuilder, EntryPart, LinkTargetType, MIN_CHUNK_BYTES_SIZE, NormalEntry,
PNA_HEADER, ReadEntry, SolidEntryBuilder, WriteOptions, prelude::*,
PNA_HEADER, ReadEntry, ReadOptions, SolidEntryBuilder, WriteOptions, prelude::*,
};
use std::{
borrow::Cow,
Expand Down Expand Up @@ -1242,7 +1242,8 @@ impl TransformStrategy for TransformStrategyUnSolid {
{
match read_entry? {
ReadEntry::Solid(s) => {
for n in s.entries(password)? {
let mut read_options = ReadOptions::with_password(password);
for n in s.entries(&mut read_options)? {
Comment on lines +1245 to +1246
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reuse one ReadOptions across solid transforms.

These branches still create a fresh ReadOptions per ReadEntry::Solid, so unsolid/keep-solid rewrites lose the new KeyCache between solid blocks and re-run the KDF for every block. Thread a shared &mut ReadOptions from run_transform_entry() into TransformStrategy::transform() instead of rebuilding it here.

♻️ Minimal sketch
 trait TransformStrategy {
-    fn transform<W, T, F>(archive: &mut Archive<W>, password: Option<&[u8]>, read_entry: io::Result<ReadEntry<T>>, transformer: F) -> io::Result<()>
+    fn transform<W, T, F>(archive: &mut Archive<W>, read_options: &mut ReadOptions, read_entry: io::Result<ReadEntry<T>>, transformer: F) -> io::Result<()>
 }
 ...
- let password = password_provider();
+ let mut read_options = ReadOptions::with_password(password_provider());
 ...
- |entry| Transform::transform(&mut out_archive, password, entry, &mut processor),
+ |entry| Transform::transform(&mut out_archive, &mut read_options, entry, &mut processor),

Also applies to: 1236-1237

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/src/command/core.rs` around lines 1191 - 1192, The loop currently
constructs a new ReadOptions inside the s.entries(&mut read_options)? call which
causes ReadEntry::Solid branches to lose KeyCache between solid blocks; instead
thread a single mutable ReadOptions from run_transform_entry() into
TransformStrategy::transform() and stop rebuilding it per-entry—modify
run_transform_entry() to create and pass &mut ReadOptions into calls that invoke
TransformStrategy::transform(), update TransformStrategy::transform() signature
to accept &mut ReadOptions, and remove the local
ReadOptions::with_password(password) construction inside the s.entries loop so
the same ReadOptions (and its KeyCache) is reused across solid transforms.

if let Some(entry) = transformer(n.map(Into::into))? {
archive.add_entry(entry)?;
}
Expand Down Expand Up @@ -1286,7 +1287,8 @@ impl TransformStrategy for TransformStrategyKeepSolid {
.password(password)
.build(),
)?;
for n in s.entries(password)? {
let mut read_options = ReadOptions::with_password(password);
for n in s.entries(&mut read_options)? {
if let Some(entry) = transformer(n.map(Into::into))? {
builder.add_entry(entry)?;
}
Expand Down Expand Up @@ -1389,10 +1391,13 @@ where
F: FnMut(io::Result<NormalEntry>) -> io::Result<()>,
{
let password = password_provider();
let mut read_options = ReadOptions::with_password(password);
run_read_entries(
archive_provider,
|entry| match entry? {
ReadEntry::Solid(solid) => solid.entries(password)?.try_for_each(&mut processor),
ReadEntry::Solid(solid) => solid
.entries(&mut read_options)?
.try_for_each(&mut processor),
ReadEntry::Normal(regular) => processor(Ok(regular)),
},
allow_concatenated_archives,
Expand All @@ -1410,11 +1415,12 @@ where
F: FnMut(io::Result<NormalEntry>) -> io::Result<ProcessAction>,
{
let password = password_provider();
let mut read_options = ReadOptions::with_password(password);
run_read_entries_stoppable(
archive_provider,
|entry| match entry? {
ReadEntry::Solid(solid) => {
for n in solid.entries(password)? {
for n in solid.entries(&mut read_options)? {
match processor(n)? {
ProcessAction::Continue => {}
ProcessAction::Stop => return Ok(ProcessAction::Stop),
Expand Down Expand Up @@ -1492,11 +1498,12 @@ where
F: FnMut(io::Result<NormalEntry<Cow<'d, [u8]>>>) -> io::Result<()>,
{
let password = password_provider();
let mut read_options = ReadOptions::with_password(password);
run_read_entries_mem(
archives,
|entry| match entry? {
ReadEntry::Solid(s) => s
.entries(password)?
.entries(&mut read_options)?
.try_for_each(|r| processor(r.map(Into::into))),
ReadEntry::Normal(r) => processor(Ok(r)),
},
Expand Down Expand Up @@ -1577,11 +1584,12 @@ where
F: FnMut(io::Result<NormalEntry<Cow<'d, [u8]>>>) -> io::Result<ProcessAction>,
{
let password = password_provider();
let mut read_options = ReadOptions::with_password(password);
run_read_entries_mem_stoppable(
archives,
|entry| match entry? {
ReadEntry::Solid(s) => {
for n in s.entries(password)? {
for n in s.entries(&mut read_options)? {
match processor(n.map(Into::into))? {
ProcessAction::Continue => {}
ProcessAction::Stop => return Ok(ProcessAction::Stop),
Expand Down
5 changes: 4 additions & 1 deletion cli/src/command/core/archive_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
use std::borrow::Cow;
use std::{fs, io};

#[cfg(feature = "memmap")]
use pna::ReadOptions;
use pna::{NormalEntry, ReadEntry};

use super::TransformStrategy;
Expand Down Expand Up @@ -90,11 +92,12 @@ impl SplitArchiveReader {
password: Option<&[u8]>,
mut processor: impl FnMut(io::Result<NormalEntry<Cow<'s, [u8]>>>) -> io::Result<()>,
) -> io::Result<()> {
let mut read_options = ReadOptions::with_password(password);
super::run_read_entries_mem(
self.mmaps.iter().map(|m| m.as_ref()),
|entry| match entry? {
ReadEntry::Solid(s) => s
.entries(password)?
.entries(&mut read_options)?
.try_for_each(|r| processor(r.map(Into::into))),
ReadEntry::Normal(n) => processor(Ok(n)),
},
Expand Down
7 changes: 4 additions & 3 deletions cli/src/command/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ fn compare_entry<T: AsRef<[u8]>>(
let data_kind = entry.header().data_kind();
let path = entry.header().path();
let path_str = path.as_str();
let mut read_options = ReadOptions::with_password(password);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Creating a new ReadOptions instance inside compare_entry prevents the KeyCache from being reused across different entries in the archive. Since compare_entry is called for every entry during a diff operation, this results in redundant and expensive password hashing (e.g., Argon2) for every file if they share the same encryption parameters. This significantly impacts performance, especially for archives with many small encrypted files. Consider passing a mutable reference to a shared ReadOptions object from the caller.

let meta = match fs::symlink_metadata(path) {
Ok(meta) => meta,
Err(e) if e.kind() == io::ErrorKind::NotFound => {
Expand All @@ -292,7 +293,7 @@ fn compare_entry<T: AsRef<[u8]>>(
println!("{}", DiffKind::SizeDiffers.display(path_str));
} else {
let fs_file = fs::File::open(path)?;
let archive_reader = entry.reader(ReadOptions::with_password(password))?;
let archive_reader = entry.reader(&mut read_options)?;
if !streams_equal(fs_file, archive_reader)? {
println!("{}", DiffKind::ContentsDiffer.display(path_str));
}
Expand All @@ -306,7 +307,7 @@ fn compare_entry<T: AsRef<[u8]>>(
}
DataKind::SymbolicLink if meta.is_symlink() => {
let link = fs::read_link(path)?;
let mut reader = entry.reader(ReadOptions::with_password(password))?;
let mut reader = entry.reader(&mut read_options)?;
let mut link_str = String::new();
reader.read_to_string(&mut link_str)?;
if link.as_path() != Path::new(&link_str) {
Expand All @@ -317,7 +318,7 @@ fn compare_entry<T: AsRef<[u8]>>(
println!("{}", DiffKind::TypeMismatch.display(path_str));
}
DataKind::HardLink if meta.is_file() => {
let mut reader = entry.reader(ReadOptions::with_password(password))?;
let mut reader = entry.reader(&mut read_options)?;
let mut target = String::new();
reader.read_to_string(&mut target)?;

Expand Down
13 changes: 8 additions & 5 deletions cli/src/command/extract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@
pub(crate) filter: PathFilter<'a>,
pub(crate) keep_options: KeepOptions,
pub(crate) pathname_editor: PathnameEditor,
pub(crate) ordered_path_locks: Arc<OrderedPathLocks>,

Check warning on line 612 in cli/src/command/extract.rs

View workflow job for this annotation

GitHub Actions / Test WebAssembly (nightly, wasm32-unknown-emscripten)

field `ordered_path_locks` is never read

Check warning on line 612 in cli/src/command/extract.rs

View workflow job for this annotation

GitHub Actions / Test WebAssembly (nightly, wasm32-unknown-emscripten)

field `ordered_path_locks` is never read
pub(crate) unlink_first: bool,
pub(crate) time_filters: TimeFilters,
pub(crate) safe_writes: bool,
Expand Down Expand Up @@ -1353,11 +1353,12 @@
return Ok(());
};

let mut read_options = ReadOptions::with_password(password);
if *safe_writes {
let mut safe_writer = SafeWriter::new(&path)?;
{
let mut writer = io::BufWriter::with_capacity(64 * 1024, safe_writer.as_file_mut());
let mut reader = item.reader(ReadOptions::with_password(password))?;
let mut reader = item.reader(&mut read_options)?;
io::copy(&mut reader, &mut writer)?;
writer.flush()?;
}
Expand All @@ -1369,7 +1370,7 @@
}
let file = utils::fs::file_create(&path, remove_existing)?;
let mut writer = io::BufWriter::with_capacity(64 * 1024, file);
let mut reader = item.reader(ReadOptions::with_password(password))?;
let mut reader = item.reader(&mut read_options)?;
io::copy(&mut reader, &mut writer)?;
let mut file = writer.into_inner().map_err(|e| e.into_error())?;
restore_timestamps(&mut file, item.metadata(), keep_options)?;
Expand Down Expand Up @@ -1418,9 +1419,10 @@
return Ok(());
};

let mut read_options = ReadOptions::with_password(password);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Instantiating ReadOptions within extract_entry defeats the purpose of the KeyCache for NormalEntry decryption across the archive. Because extract_entry is invoked per file, the cache is lost between calls, forcing a full key derivation (Argon2/PBKDF2) for every entry. This is a major performance bottleneck. For sequential extraction, a single ReadOptions should be reused. For parallel extraction (via Rayon), consider using thread-local ReadOptions or cloning a 'warmed' instance if applicable.

match item.header().data_kind() {
DataKind::SymbolicLink => {
let reader = item.reader(ReadOptions::with_password(password))?;
let reader = item.reader(&mut read_options)?;
let original = io::read_to_string(reader)?;
let original = pathname_editor.edit_symlink(original.as_ref());
if !allow_unsafe_links && is_unsafe_link(&original) {
Expand All @@ -1436,7 +1438,7 @@
symlink_with_type(&original, &path, link_target_type)?;
}
DataKind::HardLink => {
let reader = item.reader(ReadOptions::with_password(password))?;
let reader = item.reader(&mut read_options)?;
let original = io::read_to_string(reader)?;
let Some((original, had_root)) = pathname_editor.edit_hardlink(original.as_ref())
else {
Expand Down Expand Up @@ -1625,7 +1627,7 @@
MacMetadataStrategy::Always
) && item.mac_metadata().is_some();
#[cfg(not(target_os = "macos"))]
let skip_xattr_acl = false;

Check warning on line 1630 in cli/src/command/extract.rs

View workflow job for this annotation

GitHub Actions / Test WebAssembly (nightly, wasm32-wasip1)

unused variable: `skip_xattr_acl`

Check warning on line 1630 in cli/src/command/extract.rs

View workflow job for this annotation

GitHub Actions / Test WebAssembly (nightly, wasm32-wasip1)

unused variable: `skip_xattr_acl`

#[cfg(unix)]
if !skip_xattr_acl && matches!(keep_options.xattr_strategy, XattrStrategy::Always) {
Expand Down Expand Up @@ -1881,7 +1883,8 @@
return Ok(());
}

let mut reader = item.reader(ReadOptions::with_password(password))?;
let mut read_options = ReadOptions::with_password(password);
let mut reader = item.reader(&mut read_options)?;
let mut stdout = io::stdout().lock();
io::copy(&mut reader, &mut stdout)?;
stdout.flush()?;
Expand Down
13 changes: 8 additions & 5 deletions cli/src/command/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ impl TableRow {
// Only read link target if needed (requires decompression)
if collect.link_target {
entry
.reader(ReadOptions::with_password(password))
.reader(&mut ReadOptions::with_password(password))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

ReadOptions is instantiated here (and again at line 442) for every symbolic or hard link encountered during a long listing. This triggers expensive key derivation twice per link entry. TableRow::from_entry should be refactored to accept a &mut ReadOptions from the caller (e.g., list_archive or run_list_archive), where a persistent options object already exists.

.and_then(io::read_to_string)
.unwrap_or_else(|_| "-".into())
} else {
Expand All @@ -445,7 +445,7 @@ impl TableRow {
// Only read link target if needed (requires decompression)
if collect.link_target {
entry
.reader(ReadOptions::with_password(password))
.reader(&mut ReadOptions::with_password(password))
.and_then(io::read_to_string)
.unwrap_or_else(|_| "-".into())
} else {
Expand Down Expand Up @@ -559,12 +559,13 @@ fn list_archive(ctx: &crate::cli::GlobalContext, args: ListCommand) -> anyhow::R
let password = password.as_deref();
let mut entries = Vec::new();
let collect_opts = CollectOptions::from_list_options(&options);
let mut read_options = ReadOptions::with_password(password);
source.for_each_read_entry(
#[hooq::skip_all]
|entry| {
match entry? {
ReadEntry::Solid(solid) if options.solid => {
for entry in solid.entries(password)? {
for entry in solid.entries(&mut read_options)? {
entries.push(TableRow::from_entry(
&entry?,
password,
Expand Down Expand Up @@ -646,14 +647,16 @@ pub(crate) fn run_list_archive<'a>(
) -> anyhow::Result<()> {
let collect_opts = CollectOptions::from_list_options(&args);

let mut read_options = ReadOptions::with_password(password);

if !fast_read || files_globs.is_empty() {
let mut entries = Vec::new();
run_read_entries(
archive_provider,
|entry| {
match entry? {
ReadEntry::Solid(solid) if args.solid => {
for entry in solid.entries(password)? {
for entry in solid.entries(&mut read_options)? {
entries.push(TableRow::from_entry(
&entry?,
password,
Expand Down Expand Up @@ -686,7 +689,7 @@ pub(crate) fn run_list_archive<'a>(
|entry| {
match entry? {
ReadEntry::Solid(solid) if args.solid => {
for entry in solid.entries(password)? {
for entry in solid.entries(&mut read_options)? {
let entry = entry?;
let entry_path = entry.name().to_string();
if !globs.matches_any_pattern(&entry_path) {
Expand Down
23 changes: 14 additions & 9 deletions cli/tests/cli/stdio/option_ignore_zeros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ fn read_archive_entries(path: impl AsRef<Path>) -> Vec<(String, String)> {
let mut archive = Archive::read_header(fs::File::open(path).unwrap()).unwrap();
archive
.entries()
.extract_solid_entries(None)
.extract_solid_entries(&mut ReadOptions::builder().build())
.map(|entry| {
let entry = entry.unwrap();
let mut reader = entry.reader(ReadOptions::builder().build()).unwrap();
let mut reader = entry.reader(&mut ReadOptions::builder().build()).unwrap();
let mut content = String::new();
reader.read_to_string(&mut content).unwrap();
(entry.name().to_string(), content)
Expand All @@ -88,13 +88,18 @@ fn read_all_archive_entries_from_bytes(bytes: &[u8]) -> Vec<(String, String)> {
Err(err) if err.kind() == std::io::ErrorKind::UnexpectedEof => break,
Err(err) => panic!("unexpected archive read error: {err}"),
};
entries.extend(archive.entries().extract_solid_entries(None).map(|entry| {
let entry = entry.unwrap();
let mut reader = entry.reader(ReadOptions::builder().build()).unwrap();
let mut content = String::new();
reader.read_to_string(&mut content).unwrap();
(entry.name().to_string(), content)
}));
entries.extend(
archive
.entries()
.extract_solid_entries(&mut ReadOptions::builder().build())
.map(|entry| {
let entry = entry.unwrap();
let mut reader = entry.reader(&mut ReadOptions::builder().build()).unwrap();
let mut content = String::new();
reader.read_to_string(&mut content).unwrap();
(entry.name().to_string(), content)
}),
);
let _ = archive.into_inner();
}

Expand Down
2 changes: 1 addition & 1 deletion cli/tests/cli/update/no_timestamp_archive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ fn update_no_timestamp_archive_always_updates() {
if entry.header().path().as_str() == text_txt_path {
let mut buf = Vec::new();
entry
.reader(ReadOptions::with_password::<&[u8]>(None))
.reader(&mut ReadOptions::with_password::<&[u8]>(None))
.unwrap()
.read_to_end(&mut buf)
.unwrap();
Expand Down
4 changes: 2 additions & 2 deletions cli/tests/cli/update/option_archive_missing_mtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ fn update_default_mtime_missing_still_updates() {
.expect("entry should exist");
let mut buf = Vec::new();
entry
.reader(ReadOptions::with_password::<&[u8]>(None))
.reader(&mut ReadOptions::with_password::<&[u8]>(None))
.unwrap()
.read_to_end(&mut buf)
.unwrap();
Expand Down Expand Up @@ -140,7 +140,7 @@ fn update_archive_missing_mtime_exclude_keeps_entry() {
.expect("entry should exist");
let mut buf = Vec::new();
entry
.reader(ReadOptions::with_password::<&[u8]>(None))
.reader(&mut ReadOptions::with_password::<&[u8]>(None))
.unwrap()
.read_to_end(&mut buf)
.unwrap();
Expand Down
8 changes: 5 additions & 3 deletions cli/tests/cli/utils/archive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ pub fn extract_single_entry(
name: &str,
) -> io::Result<Option<pna::NormalEntry>> {
let mut archive = pna::Archive::open(path)?;
let entries = archive.entries().extract_solid_entries(None);
let mut read_options = pna::ReadOptions::with_password::<&[u8]>(None);
let entries = archive.entries().extract_solid_entries(&mut read_options);
for entry in entries {
let entry = entry?;
if entry.header().path() == name {
Expand All @@ -136,7 +137,8 @@ where
{
let password = password.into().map(|p| p.as_bytes());
let mut archive = pna::Archive::open(path)?;
let entries = archive.entries().extract_solid_entries(password);
let mut read_options = pna::ReadOptions::with_password(password);
let entries = archive.entries().extract_solid_entries(&mut read_options);
for entry in entries {
f(entry?);
}
Expand All @@ -146,7 +148,7 @@ where
pub fn read_symlink_target(entry: &pna::NormalEntry) -> String {
let mut target = Vec::new();
entry
.reader(pna::ReadOptions::with_password::<&[u8]>(None))
.reader(&mut pna::ReadOptions::with_password::<&[u8]>(None))
.unwrap()
.read_to_end(&mut target)
.unwrap();
Expand Down
4 changes: 2 additions & 2 deletions fuzz/fuzz_targets/aes_cbc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ fuzz_target!(|data: &[u8]| {
let mut builder = EntryBuilder::new_file("fuzz".into(), write_option).unwrap();
builder.write_all(data).unwrap();
let entry = builder.build().unwrap();
let read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(read_option).unwrap();
let mut read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(&mut read_option).unwrap();
let mut buf = Vec::with_capacity(data.len());
reader.read_to_end(&mut buf).unwrap();
assert_eq!(data, buf);
Expand Down
4 changes: 2 additions & 2 deletions fuzz/fuzz_targets/aes_ctr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ fuzz_target!(|data: &[u8]| {
let mut builder = EntryBuilder::new_file("fuzz".into(), write_option).unwrap();
builder.write_all(data).unwrap();
let entry = builder.build().unwrap();
let read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(read_option).unwrap();
let mut read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(&mut read_option).unwrap();
let mut buf = Vec::with_capacity(data.len());
reader.read_to_end(&mut buf).unwrap();
assert_eq!(data, buf);
Expand Down
4 changes: 2 additions & 2 deletions fuzz/fuzz_targets/camellia_cbc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ fuzz_target!(|data: &[u8]| {
let mut builder = EntryBuilder::new_file("fuzz".into(), write_option).unwrap();
builder.write_all(data).unwrap();
let entry = builder.build().unwrap();
let read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(read_option).unwrap();
let mut read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(&mut read_option).unwrap();
let mut buf = Vec::with_capacity(data.len());
reader.read_to_end(&mut buf).unwrap();
assert_eq!(data, buf);
Expand Down
4 changes: 2 additions & 2 deletions fuzz/fuzz_targets/camellia_ctr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ fuzz_target!(|data: &[u8]| {
let mut builder = EntryBuilder::new_file("fuzz".into(), write_option).unwrap();
builder.write_all(data).unwrap();
let entry = builder.build().unwrap();
let read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(read_option).unwrap();
let mut read_option = ReadOptions::with_password(Some("password"));
let mut reader = entry.reader(&mut read_option).unwrap();
let mut buf = Vec::with_capacity(data.len());
reader.read_to_end(&mut buf).unwrap();
assert_eq!(data, buf);
Expand Down
Loading
Loading