From dd506fe2c8fea6bef0cd9533ac7f978eaff6f4a8 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Aug 2021 15:25:52 -0400 Subject: [PATCH 1/6] checkout: Also ignore xattrs for union in bare-user-only mode Followup to PRs related to https://github.com/ostreedev/ostree/issues/2410 Since the test suite now covers this the test was failing on a Fedora SELinux enabled host where we see `security.selinux` even if not in the commit. --- src/libostree/ostree-repo-checkout.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c index eaa33a2866..2dec8545b4 100644 --- a/src/libostree/ostree-repo-checkout.c +++ b/src/libostree/ostree-repo-checkout.c @@ -372,7 +372,7 @@ create_file_copy_from_input_at (OstreeRepo *repo, * checkout_file_hardlink(). */ OstreeChecksumFlags flags = 0; - if (repo->disable_xattrs) + if (repo->disable_xattrs || repo->mode == OSTREE_REPO_MODE_BARE_USER_ONLY) flags |= OSTREE_CHECKSUM_FLAGS_IGNORE_XATTRS; if (repo->mode == OSTREE_REPO_MODE_BARE_USER_ONLY) @@ -528,7 +528,7 @@ checkout_file_hardlink (OstreeRepo *self, * shouldn't hit this anymore. https://github.com/ostreedev/ostree/pull/1258 * */ OstreeChecksumFlags flags = 0; - if (self->disable_xattrs) + if (self->disable_xattrs || self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY) flags |= OSTREE_CHECKSUM_FLAGS_IGNORE_XATTRS; if (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY) From 82e5df83b872846695f57982b1be28a5e0af2580 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 25 Aug 2021 15:18:43 -0400 Subject: [PATCH 2/6] lib: Change read_commit_detached_metadata to be nullable Hit this while working on some Rust code. --- src/libostree/ostree-repo-commit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index d5ab57a2f5..8dc2355e9e 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -3117,7 +3117,7 @@ ostree_repo_write_commit_with_time (OstreeRepo *self, * ostree_repo_read_commit_detached_metadata: * @self: Repo * @checksum: ASCII SHA256 commit checksum - * @out_metadata: (out) (transfer full): Metadata associated with commit in with format "a{sv}", or %NULL if none exists + * @out_metadata: (out) (nullable) (transfer full): Metadata associated with commit in with format "a{sv}", or %NULL if none exists * @cancellable: Cancellable * @error: Error * @@ -3132,6 +3132,8 @@ ostree_repo_read_commit_detached_metadata (OstreeRepo *self, GCancellable *cancellable, GError **error) { + g_assert (out_metadata != NULL); + char buf[_OSTREE_LOOSE_PATH_MAX]; _ostree_loose_path (buf, checksum, OSTREE_OBJECT_TYPE_COMMIT_META, self->mode); From 6518b0147b74e077623db6171f626992640ce5ba Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Aug 2021 09:53:56 -0400 Subject: [PATCH 3/6] ci: Run main GH action CI build+test as non-root This is really the standard best practice, matching how e.g. dpkg/rpm work, as well as most local development environments (including mine) with e.g. `toolbox`. --- .github/workflows/tests.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index df1b1e07e7..c17a1c0dfb 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -120,8 +120,11 @@ jobs: - name: Install dependencies run: ./ci/gh-install.sh ${{ matrix.extra-packages }} + - name: Add non-root user + run: "useradd builder && chown -R -h builder: ." + - name: Build and test - run: ./ci/gh-build.sh ${{ matrix.configure-options }} + run: runuser -u builder -- ./ci/gh-build.sh ${{ matrix.configure-options }} env: # GitHub hosted runners currently have 2 CPUs, so run 2 # parallel make jobs. From a28f50915a023eccb23082dd4cda74ea07524523 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Aug 2021 08:03:55 -0400 Subject: [PATCH 4/6] tests/basic: Use --mode-ro-executables instead of --owner-uid=0 This allows the test suite to run as non-root again. When operating on a bare non-root repo, we can't `chown()`. (Arguably, non-root `bare` repos are not very useful and we shouldn't test them or potentially even support them, but that's a whole other thing) --- tests/basic-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/basic-test.sh b/tests/basic-test.sh index 935544d91e..e84c385827 100644 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -435,7 +435,7 @@ echo "ok user checkout" $OSTREE commit ${COMMIT_ARGS} -b test2 -s "Another commit" --tree=ref=test2 echo "ok commit from ref" -$OSTREE commit ${COMMIT_ARGS} -b test2 -s "Another commit with modifier" --tree=ref=test2 --owner-uid=0 +$OSTREE commit ${COMMIT_ARGS} -b test2 -s "Another commit with modifier" --tree=ref=test2 --mode-ro-executables echo "ok commit from ref with modifier" $OSTREE commit ${COMMIT_ARGS} -b trees/test2 -s 'ref with / in it' --tree=ref=test2 From 1e55c844e06cee41bd4e38d4b5da9b8367778990 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Aug 2021 12:47:00 -0400 Subject: [PATCH 5/6] tests/basic: Skip --no-xattrs if we have selinux It cannot work to use `--no-xattrs` when SELinux is enabled because we get a `security.selinux` attribute on created files regardless. So just skip this test if true. Also add some `ostree fsck`s in here which helped me debug this. --- tests/basic-test.sh | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/basic-test.sh b/tests/basic-test.sh index e84c385827..8e9521749a 100644 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -455,11 +455,21 @@ $OSTREE commit ${COMMIT_ARGS} --skip-if-unchanged -b trees/test2 -s 'should not $OSTREE ls -R -C test2 new_rev=$($OSTREE rev-parse test2) assert_streq "${old_rev}" "${new_rev}" +$OSTREE fsck echo "ok commit --skip-if-unchanged" -cd ${test_tmpdir}/checkout-test2-4 -$OSTREE commit ${COMMIT_ARGS} -b test2 -s "no xattrs" --no-xattrs -echo "ok commit with no xattrs" +if have_selinux_relabel; then + # Unfortunately later tests depend on this right now, so commit anyways + cd ${test_tmpdir}/checkout-test2-4 + $OSTREE commit ${COMMIT_ARGS} -b test2 + echo "ok # SKIP we get an injected security.selinux xattr regardless, so we can't do this" +else + cd ${test_tmpdir}/checkout-test2-4 + $OSTREE commit ${COMMIT_ARGS} -b test2-noxattrs -s "no xattrs" --no-xattrs + # Validate our assumptions + $OSTREE fsck + echo "ok commit with no xattrs" +fi mkdir tree-A tree-B touch tree-A/file-a tree-B/file-b From a5c95a9b6eee524292629a8cd0742fd442dc1d1d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 26 Aug 2021 15:16:37 -0400 Subject: [PATCH 6/6] checkout: Save errno when re-throwing I was seeing an `EPERM` here which was confusing. It turned out the real error was `EEXIST`. Since we're referring to the original error, but we do a lot of computation in the middle, we need to save errno. --- src/libostree/ostree-repo-checkout.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c index 2dec8545b4..eadaf905ac 100644 --- a/src/libostree/ostree-repo-checkout.c +++ b/src/libostree/ostree-repo-checkout.c @@ -481,6 +481,7 @@ checkout_file_hardlink (OstreeRepo *self, } else if (errno == EEXIST) { + int saved_errno = errno; /* When we get EEXIST, we need to handle the different overwrite modes. */ switch (options->overwrite_mode) { @@ -566,6 +567,7 @@ checkout_file_hardlink (OstreeRepo *self, else { g_assert_cmpint (options->overwrite_mode, ==, OSTREE_REPO_CHECKOUT_OVERWRITE_UNION_IDENTICAL); + errno = saved_errno; return glnx_throw_errno_prefix (error, "Hardlinking %s to %s", loose_path, destination_name); } break;