Skip to content

Conversation

kskalski
Copy link

@kskalski kskalski commented Oct 13, 2025

Problem

  • set_perms in hardened_unpack is calling crate::file_io::set_file_readonly for a non-unix platforms which introduces a bit weird dependency (i.e. windows-only API)
  • the set_file_readonly call actually ignores the mode, which matches the way we currently call set_perms in hardened_unpack (set files as writable, I suppose for mmap functionality), but we could as well follow the passed mode
  • set_file_readonly is used in both file_io and hardened_unpack, but those calls can be replaced with a call to more generic set path permissions function

Summary of Changes

  • move set_perms to file_io as set_path_permissions
  • remove set_file_readonly and replace calls with set_path_permissions

Note: this PR will be useful for cleaner change when extracting hardened_unpack into dedicated agave-snapshots crate (#8428)

@kskalski kskalski marked this pull request as ready for review October 13, 2025 01:46
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.1%. Comparing base (c79c80f) to head (8ea87b7).

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #8439     +/-   ##
=========================================
- Coverage    83.1%    83.1%   -0.1%     
=========================================
  Files         839      839             
  Lines      367836   367847     +11     
=========================================
+ Hits       305943   305946      +3     
- Misses      61893    61901      +8     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

@kskalski kskalski added this pull request to the merge queue Oct 13, 2025
Merged via the queue into anza-xyz:master with commit 13a4656 Oct 13, 2025
43 checks passed
@kskalski kskalski deleted the ks/move_set_perms branch October 13, 2025 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants