From fa0bf1373248dee2947201354fd37a9f7bd50064 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Mon, 24 Aug 2026 15:10:23 +0000 Subject: [PATCH] Fix volume init to always reconcile the host directory's own attributes resolve_volume_mount() was supposed to initialize a fresh host directory from the image's own content at the mounted container path, but only ever did anything when that image-side directory was non-empty. Two bugs followed: an image declaring an *empty* directory with specific ownership/ permissions (e.g. a data directory owned by a non-root uid/gid) got a host directory with plain create_directories() defaults instead, and even the non-empty case never reconciled the directory's own attributes -- only each copied entry's. Fixed by splitting into two independent steps in the embedded cp script: copy contents only when non-empty (as before), then always reconcile the host directory's own mode/ownership/timestamps/xattrs via `cp -a --attributes-only -T`. -T/--no-target-directory turned out to be required -- caught by direct testing: without it, cp nests the image directory *into* the already-existing host directory instead of reconciling its attributes, which silently produced a spurious nested copy and left the host directory's own attributes untouched. Verified end-to-end against images/gitea.tar's real declared /etc/gitea and /var/lib/gitea volumes under a real rootless mount: the resulting host directories' mode/ownership now match the image's own declared values, with no leftover mounts/layers afterward. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz --- CLAUDE.md | 46 ++++++++++++++++++++++++++++++-------------- src/volume_mount.cpp | 44 +++++++++++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 698ea39..293bf6e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -471,20 +471,38 @@ Source layout (all under `src/`): `resolve_volume_mount()`, called once per `-v` occurrence from `run_container()` when running with `-r`. A spec with no `/` is looked up in `config.volumes` by name (error if unknown); one with `/` is treated as a host directory path and - `create_directories()`'d if missing. If the resulting host directory is empty and - the image already has non-empty content at the given container path, that content - is copied in first. Both the existence check and the copy run as a single - `sh -c '[ -d ... ] && cp -a ...'` invocation wrapped through - `wrap_for_root_namespace()` (`bwrap.h`) — **not** a plain `std::filesystem` check - — because a rootless `containers-storage mount`'s content isn't visible to this - process at all without `nsenter`, the same constraint `run_bwrap()` itself works - around (see the root-vs-rootless paragraph below). `cp -a - --preserve=mode,ownership,timestamps,links[,xattr]` does the copy; whether - `,xattr` is included is decided by a direct `setxattr()`/`removexattr()` probe on - the host directory (no new library dependency — Linux POSIX ACLs are themselves - stored as xattrs, so this one probe stands in for both, logging a single - `spdlog::warn` if unsupported). A nonzero `cp` exit is only ever a warning, never - fatal — often just an ownership-preservation shortfall when not running as root. + `create_directories()`'d if missing. If the resulting host directory is empty, + `initialize_volume_directory()` reconciles it against the image's own directory + at the given container path: if that image directory is non-empty, its contents + are copied in first; then, **whether or not there was content to copy**, the + host directory's own mode/ownership/timestamps (and xattrs/ACLs where + supported) are always set to match the image directory's own — **real bug + fixed by the user, not assumed**: an earlier version only ever copied when the + image directory was non-empty, so an image declaring an *empty* directory with + specific ownership/permissions (e.g. a data directory owned by a non-root + uid/gid) got a host directory with default `create_directories()` permissions + instead, and even the non-empty case never reconciled the directory's *own* + attributes (only each copied entry's). The existence check, content copy, and + attribute reconciliation all run as a single `sh -c` invocation wrapped through + `wrap_for_root_namespace()` (`bwrap.h`) — **not** a plain `std::filesystem` + check — because a rootless `containers-storage mount`'s content isn't visible + to this process at all without `nsenter`, the same constraint `run_bwrap()` + itself works around (see the root-vs-rootless paragraph below). `cp -a + --preserve=mode,ownership,timestamps,links[,xattr] --attributes-only -T` does + the attribute-reconciliation step; whether `,xattr` is included is decided by + a direct `setxattr()`/`removexattr()` probe on the host directory (no new + library dependency — Linux POSIX ACLs are themselves stored as xattrs, so this + one probe stands in for both, logging a single `spdlog::warn` if unsupported). + `-T`/`--no-target-directory` is required on that second `cp` — confirmed by + direct testing: without it, since the host directory already exists, plain + `cp SRC DST` copies `SRC` *into* `DST` as a nested `DST/basename(SRC)` + subdirectory instead of reconciling `DST`'s own attributes, which is exactly + the bug this fix closes. A nonzero `cp` exit is only ever a warning, never + fatal — often just an ownership-preservation shortfall when not running as + root. Verified end-to-end against `images/gitea.tar`'s real declared + `/etc/gitea`/`/var/lib/gitea` volumes under a real rootless mount: the + resulting host directories' mode/ownership matched the image's own declared + values in both cases, and no mounts/layers were left behind afterward. Errors are logged via `spdlog::error`; every external command is also traced at debug level in `run_process()`/`run_process_foreground()` (`src/process.cpp`) — visible via diff --git a/src/volume_mount.cpp b/src/volume_mount.cpp index 0703c7c..ab40f9d 100644 --- a/src/volume_mount.cpp +++ b/src/volume_mount.cpp @@ -43,11 +43,16 @@ bool xattr_supported(const std::filesystem::path& dir) { return true; } -// If `image_side` (a path under the mounted image root) turns out to be a -// non-empty directory, recursively copies its contents into the already-existing -// `dst`, preserving mode/ownership/timestamps/links (and xattrs/ACLs where dst's -// filesystem supports them). Both the existence check and the copy run as a single -// `sh -c` invocation wrapped through wrap_for_root_namespace(): a rootless +// If `image_side` (a path under the mounted image root) is a directory, +// recursively copies its contents (if any) into the already-existing `dst`, +// then always reconciles `dst`'s own attributes (mode/ownership/timestamps, +// and xattrs/ACLs where dst's filesystem supports them) to match `image_side` +// itself -- an empty image-side directory still has attributes even though it +// has no content, and those must carry over too (e.g. an image declaring an +// empty data directory owned by a specific uid/gid with restrictive +// permissions), not just a non-empty one's. The existence check, the content +// copy, and the attribute reconciliation all run as a single `sh -c` +// invocation wrapped through wrap_for_root_namespace(): a rootless // containers-storage mount lives in a private namespace this process can't see // into directly (the same reason run_bwrap() itself needs nsenter), so a plain // std::filesystem check on `image_side` from here would see nothing at all. Never @@ -55,8 +60,8 @@ bool xattr_supported(const std::filesystem::path& dir) { // shortfall when not running as root (chown() -> EPERM) rather than a real // failure, and cp's own stderr is inherited straight to the user (see process.h), // so -r/--run proceeds with whatever did copy. -void copy_if_present(const std::string& root, bool use_nsenter, const std::filesystem::path& image_side, - const std::filesystem::path& dst) { +void initialize_volume_directory(const std::string& root, bool use_nsenter, + const std::filesystem::path& image_side, const std::filesystem::path& dst) { if (!find_in_path("cp") || !find_in_path("sh")) { spdlog::warn("cp/sh not found in PATH; cannot populate volume directory {} from the image", dst.string()); @@ -79,12 +84,25 @@ void copy_if_present(const std::string& root, bool use_nsenter, const std::files preserve_arg += preserve[i]; } - // $0=sh $1=image_side $2=preserve_arg $3=dst. Trailing "/." (not just $1) copies - // image_side's *contents* into the already-created dst instead of nesting a - // subdirectory inside it. + // $0=sh $1=image_side $2=preserve_arg $3=dst. The first cp (trailing "/." on + // $1, not just $1) copies image_side's *contents* into the already-created + // dst instead of nesting a subdirectory inside it -- only when there's + // something to copy. The second cp always runs when $1 is a directory + // (content or not): --attributes-only sets dst's *own* mode/ownership/ + // timestamps/xattrs to match image_side's own, which the first cp alone + // never does (it only preserves each copied entry's own attributes, not + // the containing directory's). -T/--no-target-directory is required here -- + // without it, since dst already exists as a directory, plain `cp SRC DST` + // copies SRC *into* DST as a nested DST/basename(SRC) subdirectory instead + // of reconciling DST's own attributes (confirmed by direct testing: without + // -T, dst ended up with a spurious nested copy of image_side and its own + // attributes were left untouched -- exactly the bug this is fixing). std::vector script = { "sh", "-c", - "if [ -d \"$1\" ] && [ -n \"$(ls -A \"$1\" 2>/dev/null)\" ]; then cp -a \"$2\" \"$1/.\" \"$3\"; fi", + "if [ -d \"$1\" ]; then " + "if [ -n \"$(ls -A \"$1\" 2>/dev/null)\" ]; then cp -a \"$2\" \"$1/.\" \"$3\"; fi; " + "cp -a \"$2\" --attributes-only -T \"$1\" \"$3\"; " + "fi", "sh", image_side.string(), preserve_arg, dst.string()}; auto argv = wrap_for_root_namespace(root, use_nsenter, script); @@ -96,7 +114,7 @@ void copy_if_present(const std::string& root, bool use_nsenter, const std::files auto result = run_process(*argv); if (result.exit_code != 0) { - spdlog::warn("copying initial contents into {} reported errors (exit code {})", dst.string(), + spdlog::warn("initializing {} from the image reported errors (exit code {})", dst.string(), result.exit_code); } } @@ -138,7 +156,7 @@ std::optional resolve_volume_mount(const std::string& spec, // absolute, so relative_path() strips container_path's leading '/' first. std::filesystem::path image_side = std::filesystem::path(merged_path) / std::filesystem::path(container_path).relative_path(); - copy_if_present(merged_path, use_nsenter, image_side, host_directory); + initialize_volume_directory(merged_path, use_nsenter, image_side, host_directory); } return ResolvedVolumeMount{host_directory, container_path};