Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: unify user-mode canonical mask to 0775 #2420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −12
Open

lib: unify user-mode canonical mask to 0775 #2420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/libostree/ostree-core-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ G_BEGIN_DECLS
#define DEFAULT_DIRECTORY_MODE 0775
#define DEFAULT_REGFILE_MODE 0660

/* Mask to sanitize permissions into a safe canonical subset, for user-bare-only mode. */
#define USERMODE_CANONICAL_MASK 0775

/* This file contains private implementation data format definitions
* read by multiple implementation .c files.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/libostree/ostree-core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2288,7 +2288,7 @@ _ostree_validate_bareuseronly_mode (guint32 content_mode,
{
if (S_ISREG (content_mode))
{
const guint32 invalid_modebits = ((content_mode & ~S_IFMT) & ~0775);
const guint32 invalid_modebits = ((content_mode & ~S_IFMT) & ~USERMODE_CANONICAL_MASK);
if (invalid_modebits > 0)
return glnx_throw (error, "Content object %s: invalid mode 0%04o with bits 0%04o",
checksum, content_mode, invalid_modebits);
Expand Down
2 changes: 1 addition & 1 deletion src/libostree/ostree-repo-checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,7 @@ checkout_tree_at_recurse (OstreeRepo *self,
* See also: https://github.com/ostreedev/ostree/pull/909 i.e. 0c4b3a2b6da950fd78e63f9afec602f6188f1ab0
*/
if (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY || options->bareuseronly_dirs)
canonical_mode = (mode & 0775) | S_IFDIR;
canonical_mode = (mode & USERMODE_CANONICAL_MASK) | S_IFDIR;
else
canonical_mode = mode;
if (TEMP_FAILURE_RETRY (fchmod (destination_dfd, canonical_mode)) < 0)
Expand Down
8 changes: 4 additions & 4 deletions src/libostree/ostree-repo-commit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ commit_loose_regfile_object (OstreeRepo *self,
*/
if (S_ISREG (mode))
{
const mode_t content_mode = (mode & (S_IFREG | 0775)) | S_IRUSR;
const mode_t content_mode = (mode & USERMODE_CANONICAL_MASK) | S_IFREG | S_IRUSR;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. There's a really important difference between bare-user and bare-user-only, which is that bare-user is intended to be losslessly convertible to-from bare. Now, we store the mode as an xattr, but it kind of intentional that the checked out tree mostly resembles what's in the bare.

I'd be happier I think if this change only touched code paths involved in bare-user-only. Then the messaging is clearer.

if (!glnx_fchmod (tmpf->fd, content_mode, error))
return FALSE;
}
Expand Down Expand Up @@ -1318,7 +1318,7 @@ adopt_and_commit_regfile (OstreeRepo *self,
if (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY)
{
const guint32 src_mode = g_file_info_get_attribute_uint32 (finfo, "unix::mode");
if (fchmod (fd, src_mode & 0755) < 0)
if (fchmod (fd, src_mode & USERMODE_CANONICAL_MASK) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I was surprised this didn't seem to break any tests that looked at the commit permissions. That seems to be a bit lacking in the tests. There are several that look at the permissions after a checkout, but only a few that look at the permissions of the committed objects with ostree ls.

return glnx_throw_errno_prefix (error, "fchmod");
}
if (renameat (dfd, name, dest_dfd, loose_path) == -1)
Expand Down Expand Up @@ -3323,11 +3323,11 @@ _ostree_repo_commit_modifier_apply (OstreeRepo *self,
/* In particular, we want to squash the s{ug}id bits, but this also
* catches the sticky bit for example.
*/
g_file_info_set_attribute_uint32 (modified_info, "unix::mode", mode & (S_IFREG | 0755));
g_file_info_set_attribute_uint32 (modified_info, "unix::mode", (mode & USERMODE_CANONICAL_MASK) | S_IFREG);
break;
case G_FILE_TYPE_DIRECTORY:
/* Like the above but for directories */
g_file_info_set_attribute_uint32 (modified_info, "unix::mode", mode & (S_IFDIR | 0755));
g_file_info_set_attribute_uint32 (modified_info, "unix::mode", (mode & USERMODE_CANONICAL_MASK) | S_IFDIR);
break;
case G_FILE_TYPE_SYMBOLIC_LINK:
break;
Expand Down
6 changes: 2 additions & 4 deletions tests/basic-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,10 +789,8 @@ $OSTREE commit ${COMMIT_ARGS} -b content-with-dir-world-writable --tree=dir=file
$OSTREE fsck
rm dir-co -rf
$OSTREE checkout -U -H -M content-with-dir-world-writable dir-co
if is_bare_user_only_repo repo; then
assert_file_has_mode dir-co/worldwritable-dir 755
else
assert_file_has_mode dir-co/worldwritable-dir 775
assert_file_has_mode dir-co/worldwritable-dir 775
if ! is_bare_user_only_repo repo; then
rm dir-co -rf
$CMD_PREFIX ostree --repo=repo checkout -U -H content-with-dir-world-writable dir-co
assert_file_has_mode dir-co/worldwritable-dir 777
Expand Down
4 changes: 2 additions & 2 deletions tests/test-basic-user-only.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ $OSTREE commit ${COMMIT_ARGS} -b perms files
$OSTREE fsck
rm out -rf
$OSTREE checkout --force-copy perms out
assert_file_has_mode out/afile 755
assert_file_has_mode out/afile 775
$OSTREE checkout ${CHECKOUT_H_ARGS} --union-identical perms out
assert_file_has_mode out/afile 755
assert_file_has_mode out/afile 775
echo "ok automatic canonical perms for bare-user-only"