-
Notifications
You must be signed in to change notification settings - Fork 635
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.
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
[Cherry-picked to v0.20.2] alpine: avoid wiping out writable host mounts under /home, etc. #2234
[Cherry-picked to v0.20.2] alpine: avoid wiping out writable host mounts under /home, etc. #2234
Conversation
4f2d96c
to
16e0e77
Compare
I have not tested it, but I suspect the same will be true for |
16e0e77
to
691c030
Compare
Cherry-picked from lima-vm/lima#2234 Signed-off-by: Jan Dubois <jan.dubois@suse.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some code that has become redundant with this change and should be removed.
d4dc678
to
f0b317a
Compare
DNM, since fstab is gone, the PR has to be worked out again from scratch |
f0b317a
to
d3c65fd
Compare
784061d
to
8781f6d
Compare
aa09cbe
to
65f247d
Compare
Confirmed that the issue affects 9p as well |
80e8d9e
to
99c93b2
Compare
Added a test |
cf056f3
to
994df39
Compare
8caa477
to
a8c8bd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM and worked in my local tests.
I don't think the mounting code will work with mount points that include backslashes, due to the way we have to do the quoting, but I think that is acceptable; who is using backslashes in directory names?
[ "${MNTTYPE}" = "ext4" ] && continue | ||
[ "${MNTTYPE}" = "tmpfs" ] && continue | ||
MNTOPTS="$(echo "${LINE}" | awk '{print $4}')" | ||
echo "mount -t \"${MNTTYPE}\" -o \"${MNTOPTS}\" \"${MNTDEV}\" \"${MNTPNT}\"" >>/mnt.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine to merge as-is. But if you want to support double-quotes and backslashes in mountpoints, then you could to
echo "mount -t \"${MNTTYPE}\" -o \"${MNTOPTS}\" \"${MNTDEV}\" \"${MNTPNT}\"" >>/mnt.sh | |
MNTPNT=${MNTPNT//\\/\\\\} | |
MNTPNT=${MNTPNT//\"/\\\"} | |
echo "mount -t \"${MNTTYPE}\" -o \"${MNTOPTS}\" \"${MNTDEV}\" \"${MNTPNT}\"" >>/mnt.sh |
I've tested it with
$ MNTPNT=$'foo\\bar"baz\nfoo\\bar"baz'
$ echo "$MNTPNT"
foo\bar"baz
foo\bar"baz
$ MNTPNT=${MNTPNT//\\/\\\\}
$ MNTPNT=${MNTPNT//\"/\\\"}
$ echo "$MNTPNT"
foo\\bar\"baz
foo\\bar\"baz
If you do that, then you should probably also update the test case to use backslashes and quotes in the test directory name.
Just realized that lima-init.sh
in alpine-lima doesn't support backslashes and quotes in mountpoint either right now: https://github.com/rancher-sandbox/alpine-lima/blob/5a55b61512a6e7b4affb8a9779d179f559b0ac18/lima-init.sh#L65-L66
So the tests would fail unless we fix this in alpine-lima as-well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, applied
A host directory could be wiped out when all the following conditions are met: - The directory is mounted to Lima via virtiofs or 9p (reverse-sshfs is not affected) - The mount is writable - The mount point in the guest is under one of: /etc /home /root /usr/local /var/lib - The guest OS is Alpine Linux Fix issue 2221 Fix rancher-sandbox/rancher-desktop issue 6582 Co-authored-by: Jan Dubois <jan.dubois@suse.com> Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
a8c8bd7
to
795ada2
Compare
Cherry-picked to v0.20.2 |
Cherry-pick of lima-vm/lima#2234 Replaces 81cdd3c Signed-off-by: Jan Dubois <jan.dubois@suse.com>
A host directory could be wiped out when all the following conditions are met:
Fix #2221
Fix rancher-sandbox/rancher-desktop#6582