Skip to content

Commit 891a0bb

Browse files
simonwalzsimonwalz
committed
fix(updater): do not leak the export container, write the fstab into the target
Four things, each with its test: A failed "docker export" left the container behind. Every failed attempt made one more, and a device that cannot reach the registry for a week makes seven. It is removed before raise 107 now. A run that is killed still leaks one - nothing short of a reaper covers that. write_fstab wrote /etc/fstab - the updater container's own - and copied it into the new rootfs afterwards. It describes the target's layout, so a tool that read it there would take the target's disk for its own; nothing does. Checked: no script in the four repos reads an fstab, no "mount -a" anywhere, every mount names its device and options, and the grub backend writes grub.cfg by hand with root=LABEL=system rootflags=subvol= from CONTAINER_ROOTFS. grub-install itself resolves devices through grub-probe and the kernel's mount table, not fstab - that is upstream behaviour, not measured here, and the first x86 build says so either way, loudly (raise 122/123). check_environment refuses to run when TARGET_ROOT or TARGET_BOOT is not a mountpoint or no image was named. prepare_slot deletes ${TARGET_BOOT}/${SLOT}_* and creates a subvolume under TARGET_ROOT; with the variables empty that is rm -f /A_* and a subvolume in /. docker_environment.sh has always mounted both and raises if it cannot, so this is insurance, not a live bug - but it is the kind of insurance that costs three lines. Smaller: is_local_build() replaces the inverted condition in pull_image, image_digest takes the digest the caller already has instead of asking docker a second time, and prepare_slot skips "docker image rm" when the old slot had no /etc/image rather than calling it with an empty argument and swallowing the error. The suite is at 53 cases. Removing the container cleanup, either mountpoint check, the fstab target or the -n guard each turns it red - checked. Not run in a real build. Signed-off-by: Simon Walz <contact@simonwalz.de>
1 parent 72c1628 commit 891a0bb

2 files changed

Lines changed: 114 additions & 22 deletions

File tree

‎updater/perform_update.sh‎

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,18 @@ product_name() {
7777
echo "${name}"
7878
}
7979

80+
# prepare_slot deletes "${TARGET_BOOT}/${SLOT}"_* and creates a subvolume under
81+
# TARGET_ROOT. Both are destructive at the wrong path if the caller has not
82+
# mounted what it says it has.
83+
check_environment() {
84+
[[ -n "${IMAGE}" ]] \
85+
|| raise 100 "No image to install given"
86+
mountpoint -q "${TARGET_ROOT}" \
87+
|| raise 100 "TARGET_ROOT (${TARGET_ROOT}) is not a mounted filesystem"
88+
mountpoint -q "${TARGET_BOOT}" \
89+
|| raise 100 "TARGET_BOOT (${TARGET_BOOT}) is not a mounted filesystem"
90+
}
91+
8092
check_free_space() {
8193
local avail_bytes required_bytes
8294

@@ -168,21 +180,20 @@ repo_digest() {
168180
| cut -d '@' -f 2
169181
}
170182

171-
# A locally built image has no RepoDigests, and its id identifies it just as
172-
# well.
183+
# $1 is the digest from the registry, empty for a locally built image: that one
184+
# has no RepoDigests, and its id identifies it just as well.
173185
image_digest() {
174-
local digest
175-
digest="$(repo_digest)"
176-
177-
echo "${digest:-"$(docker inspect --format='{{.Id}}' "${IMAGE}")"}"
186+
echo "${1:-"$(docker inspect --format='{{.Id}}' "${IMAGE}")"}"
178187
}
179188

180189
prepare_slot() {
181190
local old_image_path
182191
old_image_path="$(grep -oE '^[^@]+' "${CONTAINER_ROOTFS_FS}/etc/image" 2>/dev/null)"
183192

184193
if [[ -d "${CONTAINER_ROOTFS_FS}" ]]; then
185-
docker image rm --platform "${TARGET_PLATFORM}" "${old_image_path}" || true
194+
if [[ -n "${old_image_path}" ]]; then
195+
docker image rm --platform "${TARGET_PLATFORM}" "${old_image_path}" || true
196+
fi
186197

187198
btrfs subvolume delete -R "${CONTAINER_ROOTFS_FS}" || true
188199
fi
@@ -191,11 +202,17 @@ prepare_slot() {
191202
btrfs subvolume create "${CONTAINER_ROOTFS_FS}"
192203
}
193204

205+
# The factory tags the image it has just built ':build' and hands it over
206+
# locally, so there is no registry to pull it from.
207+
is_local_build() {
208+
[[ "${INSTALLIMAGE}" = "true" && "${IMAGE}" = *:build ]]
209+
}
210+
194211
pull_image() {
195-
if [[ "${INSTALLIMAGE}" != "true" || "${IMAGE}" != *:build ]]; then
196-
docker image pull --platform "${TARGET_PLATFORM}" "${IMAGE}" \
197-
|| raise 103 "Faild to fetch system image"
198-
fi
212+
if is_local_build; then return; fi
213+
214+
docker image pull --platform "${TARGET_PLATFORM}" "${IMAGE}" \
215+
|| raise 103 "Faild to fetch system image"
199216
}
200217

201218
export_rootfs() {
@@ -205,6 +222,7 @@ export_rootfs() {
205222

206223
if ! docker export "${container}" \
207224
| tar -C "${CONTAINER_ROOTFS_FS}" --numeric-owner --xattrs --xattrs-include='*' -xf -; then
225+
docker rm "${container}" >/dev/null 2>&1 || true
208226
raise 107 "Failed to export the container"
209227
fi
210228
docker rm "${container}" \
@@ -213,10 +231,14 @@ export_rootfs() {
213231
rm -f "${CONTAINER_ROOTFS_FS}/.dockerenv" || true
214232
}
215233

234+
# Straight into the new rootfs: this describes the target's layout, and nothing
235+
# in the updater or the factory reads an fstab of its own - every mount they
236+
# make names its device and options, and the grub backend writes the root
237+
# filesystem into the kernel command line rather than reading it back.
216238
write_fstab() {
217-
local fstab_file="${T_FILE_FSTAB:-"/etc/fstab"}"
239+
local fstab_file="${T_FILE_FSTAB:-"${CONTAINER_ROOTFS_FS}/etc/fstab"}"
218240

219-
cat <<EOF >"${fstab_file}"
241+
cat >"${fstab_file}" <<EOF || raise 119 "Failed to write ${fstab_file}"
220242
# <file system> <dir> <type> <options> <dump> <pass>
221243
# /dev/sda3
222244
LABEL=system / btrfs rw,relatime,discard=async,space_cache=v2,subvol=${CONTAINER_ROOTFS} 0 0
@@ -226,11 +248,6 @@ LABEL=system /data btrfs rw,relatime,discard=async,space_cache=v2,subvol=@dat
226248
# /dev/sda2
227249
#LABEL=boot /boot vfat rw,relatime,fmask=0022,dmask=0022,shortname=mixed,errors=remount-ro 0 2
228250
EOF
229-
230-
if [[ -n "${TEST:-}" ]]; then return; fi
231-
232-
cp "${fstab_file}" "${CONTAINER_ROOTFS_FS}/etc/fstab" \
233-
|| raise 119 "Failed to copy fstab"
234251
}
235252

236253
run_first_run() {
@@ -261,6 +278,8 @@ main() {
261278
IMAGE="${2:-}"
262279
IMAGE_DIGEST="${3:-""}"
263280

281+
check_environment
282+
264283
export PRODUCT_NAME
265284
PRODUCT_NAME="$(product_name)"
266285

@@ -295,7 +314,7 @@ main() {
295314

296315
report_disk_usage "before"
297316

298-
DIGEST="$(image_digest)"
317+
DIGEST="$(image_digest "${NEW_DIGEST}")"
299318
if [[ -z "${DIGEST}" ]]; then
300319
raise 105 "Failed to get image digest"
301320
fi

‎updater/perform_update.test.sh‎

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,21 @@ docker() {
3838
export)
3939
return "${MOCK_EXPORT_RC-0}"
4040
;;
41+
rm)
42+
echo "rm ${2}" >>"${WORK_DIR}/docker.log"
43+
;;
44+
image)
45+
echo "${1} ${2}" >>"${WORK_DIR}/docker.log"
46+
;;
4147
esac
4248
return 0
4349
}
4450

51+
mountpoint() {
52+
# "mountpoint -q <path>", so the path is $2.
53+
[[ "${2}" != "${MOCK_UNMOUNTED-}" ]]
54+
}
55+
4556
tar() {
4657
cat >/dev/null
4758
return "${MOCK_TAR_RC-0}"
@@ -140,16 +151,58 @@ expect_rc "remote_digest_is_installed: no answer falls through" 1 \
140151

141152
MOCK_REPO_DIGEST="sha256:cccc"
142153
expect "repo_digest: from RepoDigests" "sha256:cccc" repo_digest
143-
expect "image_digest: from RepoDigests" "sha256:cccc" image_digest
144-
MOCK_REPO_DIGEST=""
154+
expect "image_digest: from the registry" "sha256:cccc" image_digest "sha256:cccc"
145155
MOCK_IMAGE_ID="sha256:dddd"
146-
expect "image_digest: a local build falls back to the id" "sha256:dddd" image_digest
156+
expect "image_digest: a local build falls back to the id" "sha256:dddd" image_digest ""
157+
158+
export INSTALLIMAGE="true"
159+
export IMAGE="cuos-system:build"
160+
expect_rc "is_local_build: the factory's own build" 0 rc_of is_local_build
161+
export IMAGE="ghcr.io/cuos-dev/cuos-system:development"
162+
expect_rc "is_local_build: a pulled image" 1 rc_of is_local_build
163+
export INSTALLIMAGE=""
164+
export IMAGE="cuos-system:build"
165+
expect_rc "is_local_build: not the factory, so :build is pulled too" 1 \
166+
rc_of is_local_build
167+
export IMAGE="ghcr.io/cuos-dev/cuos-system:development"
168+
169+
pulls_of() {
170+
: >"${WORK_DIR}/docker.log"
171+
pull_image
172+
grep -c "^image pull$" "${WORK_DIR}/docker.log" || true
173+
}
174+
expect "pull_image: an image from a registry is pulled" "1" pulls_of
175+
export INSTALLIMAGE="true"
176+
export IMAGE="cuos-system:build"
177+
expect "pull_image: a local build is not" "0" pulls_of
178+
export INSTALLIMAGE=""
179+
export IMAGE="ghcr.io/cuos-dev/cuos-system:development"
180+
181+
expect_rc "check_environment: everything mounted" 0 rc_of check_environment
182+
MOCK_UNMOUNTED="${TARGET_ROOT}"
183+
expect_rc "check_environment: no root filesystem" 100 rc_of check_environment
184+
MOCK_UNMOUNTED="${TARGET_BOOT}"
185+
expect_rc "check_environment: no boot partition" 100 rc_of check_environment
186+
MOCK_UNMOUNTED=""
187+
without_image() {
188+
( IMAGE=""; check_environment )
189+
}
190+
expect_rc "check_environment: no image given" 100 rc_of without_image
147191

148192
export SLOT="B"
149193
export CONTAINER_ROOTFS="@os/system-B"
150194
export CONTAINER_ROOTFS_FS="${TARGET_ROOT}/system-B"
151195
export T_FILE_FSTAB="/dev/stdout"
152196

197+
fstab_in_the_rootfs() {
198+
( unset T_FILE_FSTAB
199+
mkdir -p "${CONTAINER_ROOTFS_FS}/etc"
200+
write_fstab
201+
grep -c "subvol=@os/system-B" "${CONTAINER_ROOTFS_FS}/etc/fstab" )
202+
}
203+
expect "write_fstab: it lands in the new rootfs, not in ours" "1" \
204+
fstab_in_the_rootfs
205+
153206
expect "write_fstab: the root entry carries the slot's subvolume" \
154207
'# <file system> <dir> <type> <options> <dump> <pass>
155208
# /dev/sda3
@@ -183,6 +236,17 @@ slot_exists() {
183236
}
184237
expect "prepare_slot: the slot subvolume is there afterwards" "yes" slot_exists
185238

239+
image_removals() {
240+
: >"${WORK_DIR}/docker.log"
241+
prepare_slot >/dev/null 2>&1
242+
grep -c "^image rm$" "${WORK_DIR}/docker.log" || true
243+
}
244+
expect "prepare_slot: a slot without /etc/image removes no image" "0" image_removals
245+
mkdir -p "${CONTAINER_ROOTFS_FS}/etc"
246+
echo "ghcr.io/cuos-dev/cuos-system:development@sha256:eeee" \
247+
>"${CONTAINER_ROOTFS_FS}/etc/image"
248+
expect "prepare_slot: the old slot's image is removed" "1" image_removals
249+
186250
dockerenv_state() {
187251
[[ -f "${CONTAINER_ROOTFS_FS}/.dockerenv" ]] && echo "there" || echo "gone"
188252
}
@@ -192,10 +256,19 @@ expect "export_rootfs: .dockerenv is written by docker" "there" dockerenv_state
192256
expect_rc "export_rootfs: a working export" 0 rc_of export_rootfs
193257
expect "export_rootfs: .dockerenv is removed" "gone" dockerenv_state
194258

259+
containers_removed() {
260+
: >"${WORK_DIR}/docker.log"
261+
rc_of export_rootfs >/dev/null
262+
grep -c "^rm container123$" "${WORK_DIR}/docker.log" || true
263+
}
264+
expect "export_rootfs: the container is removed" "1" containers_removed
265+
195266
# The regression test for the missing pipefail: docker export fails, tar
196267
# succeeds, and the truncated rootfs must not be installed.
197268
MOCK_EXPORT_RC=1
198269
expect_rc "export_rootfs: a failing export is not masked by tar" 107 rc_of export_rootfs
270+
expect "export_rootfs: a failed export does not leak the container" "1" \
271+
containers_removed
199272
MOCK_EXPORT_RC=0
200273

201274
MOCK_TAR_RC=2

0 commit comments

Comments
 (0)