Skip to content

Preserve POSIX ACLs when replacing archives - #561

Open
srkyn wants to merge 3 commits into
nih-at:mainfrom
srkyn:codex/preserve-posix-acl
Open

srkyn wants to merge 3 commits into
nih-at:mainfrom
srkyn:codex/preserve-posix-acl

Conversation

@srkyn

@srkyn srkyn commented Aug 15, 2026 •

Copy link
Copy Markdown

When libzip replaces an archive, the new inode can lose the original POSIX access ACL even though its mode bits are restored. The converse also matters: a temporary file can inherit an access ACL from its directory when the original archive has none. Mode bits alone do not preserve the effective permissions of an extended ACL.

This fills the ACL-copy TODO in copy_permissions() on top of the current temporary-file permission flow. It copies a source access ACL to the open temporary file with fsetxattr(), or clears an inherited ACL with fremovexattr() when the source has none. It then applies the final mode with fchmod() before closing and renaming the temporary file. Errors that would leave permissions uncertain stop the replacement. The Linux ACL path is feature-gated and adds no dependency.

The regression covers an ordinary archive update, a direct named-source replacement, and removal of a directory-inherited ACL when the source has no access ACL. It runs as a direct CTest so unsupported filesystems can report a skip.

Validation on Linux:

  • GCC Debug: 194 tests listed, 180 passed and 14 skipped for unavailable optional features.
  • Clang ASan/UBSan: focused ACL regression passed.
  • Feature-disabled build: passed, with no ACL test registered.
  • Negative control: removing the inherited-ACL cleanup caused the new regression to fail as intended.
  • clang-format --dry-run --Werror and the PR delta whitespace check passed.

@dillof

dillof commented Aug 19, 2026

Copy link
Copy Markdown
Member

Reading your changes lead us to redesign how permission copying is implemented: If we are going to replace an existing file, we create the temporary file with permissions 0600 and copy the permissions when replacing the file.

Could you please adapt your PR to that? There is a TODO comment where the ACL copy function should be called.

Also, don't restrict it to Linux, other system also implement this API. And check for the functions you call, not the existence of the header file. If you need to check for multiple functions, define a USE_ACL at the top of the file if all requirements are met, and use that throughout the rest of the file (to avoid duplicating the logic).

@dillof dillof added the feedback Waiting for feedback from submitter. label Aug 19, 2026
@srkyn srkyn closed this Aug 19, 2026
@srkyn
srkyn deleted the codex/preserve-posix-acl branch August 19, 2026 14:16
@srkyn
srkyn restored the codex/preserve-posix-acl branch August 19, 2026 14:17
@srkyn srkyn reopened this Aug 19, 2026
@srkyn
srkyn force-pushed the codex/preserve-posix-acl branch from 5ef3f43 to cae933b Compare August 19, 2026 14:28
@srkyn

srkyn commented Aug 19, 2026 •

Copy link
Copy Markdown
Author

Thanks. I rebased onto the new permission-copy flow and adapted the PR.

It now uses the portable acl_get_file() / acl_set_file() API, checks the functions it calls, and keeps the implementation behind a single USE_ACL guard. I also updated the regression to use the portable API and enabled libacl in Linux CI. The ACL-enabled, ACL-disabled, and ASan/UBSan checks all pass.

@dillof

dillof commented Aug 19, 2026

Copy link
Copy Markdown
Member

Thanks. However, since acl_get_file is in a separate library on Linux, and we would like to avoid extra dependencies for libzip, I would prefer if you reverted to the previous API.

@srkyn

srkyn commented Aug 19, 2026

Copy link
Copy Markdown
Author

Understood. I restored the dependency-free getxattr() / setxattr() implementation and kept it in the new copy_permissions() flow. The libacl detection, link, and CI package are gone. I also changed configuration to check the two functions directly and reran the ACL, disabled-feature, full, and sanitizer builds.

@dillof dillof removed the feedback Waiting for feedback from submitter. label Sep 16, 2026
Signed-off-by: David Sarkisyan <281478990+srkyn@users.noreply.github.com>
@dillof dillof added the feature For issues, use type instead. label Sep 30, 2026
@dillof dillof added this to the later milestone Sep 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature For issues, use type instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants