Bug#542915: pbuilder removes data from bind-mounted directories
On Tue, Dec 28, 2010 at 09:38:30AM +1100, Matthew Palmer wrote:
On Mon, Dec 27, 2010 at 05:04:02PM +0100, Julien Cristau wrote:
do you plan on NMUing pbuilder for this bug?
No, I don't consider it appropriate to NMU for an RC bug that I raised the
severity on, withouth the acknowledgement of the maintainer that the
severity is justified. Someone else NMUing is a vote that the severity is
justified.
The original submitter agreed with you on the severity. That's good
enough for me.
I can do the upload if you like. However, I'm confused by your
message of 2010-12-17, where you said Patch 5 in a series:. I'm
confused because I can see only one patch prior to this, and the later
patch seems to be a patch on the original patch. Can you
confirm whether the following is the intended change or whether
there are 4 other patches I'm missing.
Thanks,
-Steve
diff -u -r orig/pbuilder-0.199//pbuilder-modules
pbuilder-0.199//pbuilder-modules
--- orig/pbuilder-0.199//pbuilder-modules 2010-06-19 22:55:28.0
-0500
+++ pbuilder-0.199//pbuilder-modules2010-12-27 22:29:34.803419903 -0600
@@ -319,9 +319,23 @@
log W: Aborting with an error;
fi
if [ ${INTERNAL_BUILD_UML} != yes ]; then
- if [ -d $BUILDPLACE ]; then
- log I: cleaning the build env
- clean_subdirectories $BUILDPLACE
+if [ -d $BUILDPLACE ]; then
+# A directory on the same partition as $BUILDPLACE, bind-mounted
+# into $BUILDPLACE, will be cleaned out by clean_subdirectories
+# (because -xdev doesn't know about bind mounts). To avoid that
+# potential disaster (and also to avoid ugly error messages from
+# rmdir otherwise), we want to make sure that there is *nothing*
+# mounted under the chroot before we do our bulldozer routine.
+#
+# The readlink -f is a simple way to canonicalize the path for
+# $BUILDPLACE (no dirty double slashes for *us*), so it matches
+# what will be in the output of mount.
+if mount |grep -q -F $(readlink -f $BUILDPLACE)/; then
+log E: Something is still mounted under ${BUILDPLACE};
unmount and remove ${BUILDPLACE} manually
+else
+log I: cleaning the build env
+clean_subdirectories $BUILDPLACE
+fi
fi;
fi
}
signature.asc
Description: Digital signature
Bug#542915: pbuilder removes data from bind-mounted directories
On Mon, Dec 27, 2010 at 10:41:36PM -0600, Steve M. Robbins wrote: On Tue, Dec 28, 2010 at 09:38:30AM +1100, Matthew Palmer wrote: On Mon, Dec 27, 2010 at 05:04:02PM +0100, Julien Cristau wrote: do you plan on NMUing pbuilder for this bug? No, I don't consider it appropriate to NMU for an RC bug that I raised the severity on, withouth the acknowledgement of the maintainer that the severity is justified. Someone else NMUing is a vote that the severity is justified. The original submitter agreed with you on the severity. That's good enough for me. I can do the upload if you like. That'd be good. However, I'm confused by your message of 2010-12-17, where you said Patch 5 in a series:. I'm confused because I can see only one patch prior to this, and the later patch seems to be a patch on the original patch. Can you confirm whether the following is the intended change or whether there are 4 other patches I'm missing. Sorry about the confusion. My local branch for this bug, 542915 in http://github.com/mpalmer/pbuilder, has three other patches in it (to auto-unmount everything, rename mountproc to mountall, and improve an error message), and so the other patch I attached to this bug report is #5 in that series. To clarify, the two patches that needed to fix this bug are: 0001-Ensure-that-nothing-is-still-mounted-before-deleting.patch 0005-Robustify-the-is-anything-mounted-check.patch The other three patches in the series are refactorings/improvements, and should not (IMHO) be made as part of an NMU. - Matt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder removes data from bind-mounted directories
On Wed, Dec 15, 2010 at 14:21:05 +1100, Matthew Palmer wrote: tag 542915 +patch usertags 542915 -in-progress +patch-in-git thanks Attached is my minimal patch solving the problem of data loss in bind-mounted directories. It provides a safety net that, in the event that *anything* is still mounted inside the chroot, no attempt to delete anything will be made. Whilst this may, in some corner cases, result in failure to cleanup a chroot when previously (most) things would have been deleted, it should never effect most users. I've exercised this patch in every way I can think of (bind-mounting things left and right, regular mounts, symlinks to mounts, etc) and I haven't been able to get it to actually delete anything any more. Hi Matt, do you plan on NMUing pbuilder for this bug? Cheers, Julien signature.asc Description: Digital signature
Bug#542915: pbuilder removes data from bind-mounted directories
On Mon, Dec 27, 2010 at 05:04:02PM +0100, Julien Cristau wrote: do you plan on NMUing pbuilder for this bug? No, I don't consider it appropriate to NMU for an RC bug that I raised the severity on, withouth the acknowledgement of the maintainer that the severity is justified. Someone else NMUing is a vote that the severity is justified. - Matt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder removes data from bind-mounted directories
* Matthew Palmer mpal...@debian.org, 2010-12-15, 14:21: Jakub, I'd appreciate it if you would be able to apply the patch to your local pbuilder installation (it should go onto pbuilder-modulers in-situ, without a need for a rebuild) and see if you can find any ways to break it. I'm afraid I won't help here; I don't really use pbuilder anymore. +if mount |grep -q $(readlink -f $BUILDPLACE)/; then I think grep -q -F $(readlink -f $BUILDPLACE)/ would be more robust. -- Jakub Wilk -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder removes data from bind-mounted directories
On Thu, Dec 16, 2010 at 06:14:55PM +0100, Jakub Wilk wrote:
* Matthew Palmer mpal...@debian.org, 2010-12-15, 14:21:
+if mount |grep -q $(readlink -f $BUILDPLACE)/; then
I think
grep -q -F $(readlink -f $BUILDPLACE)/
would be more robust.
Right you are. Patch 5 in a series:
From 56adfe0b23582651f3443f6c15214474d8e4702e Mon Sep 17 00:00:00 2001
From: Matt Palmer mpal...@hezmatt.org
Date: Fri, 17 Dec 2010 18:43:38 +1100
Subject: [PATCH] Robustify the 'is anything mounted' check
---
pbuilder-modules |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/pbuilder-modules b/pbuilder-modules
index 16e0714..886ae93 100644
--- a/pbuilder-modules
+++ b/pbuilder-modules
@@ -317,7 +317,7 @@ function cleanbuildplace () {
# The readlink -f is a simple way to canonicalize the path for
# $BUILDPLACE (no dirty double slashes for *us*), so it matches
# what will be in the output of mount.
-if mount |grep -q $(readlink -f $BUILDPLACE)/; then
+if mount |grep -q -F $(readlink -f $BUILDPLACE)/; then
log E: Something is still mounted under ${BUILDPLACE};
unmount and remove ${BUILDPLACE} manually
else
log I: cleaning the build env
--
1.5.6.5
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder removes data from bind-mounted directories
tag 542915 +patch
usertags 542915 -in-progress +patch-in-git
thanks
Attached is my minimal patch solving the problem of data loss in
bind-mounted directories. It provides a safety net that, in the event that
*anything* is still mounted inside the chroot, no attempt to delete anything
will be made. Whilst this may, in some corner cases, result in failure to
cleanup a chroot when previously (most) things would have been deleted, it
should never effect most users.
I've exercised this patch in every way I can think of (bind-mounting things
left and right, regular mounts, symlinks to mounts, etc) and I haven't been
able to get it to actually delete anything any more.
Jakub, I'd appreciate it if you would be able to apply the patch to your
local pbuilder installation (it should go onto pbuilder-modulers in-situ,
without a need for a rebuild) and see if you can find any ways to break it.
- Matt
From b13d3b8a9757da6bd84fd374b7948919fff1df80 Mon Sep 17 00:00:00 2001
From: Matt Palmer mpal...@hezmatt.org
Date: Wed, 15 Dec 2010 07:21:00 +1100
Subject: [PATCH] Ensure that nothing is still mounted before deleting the build chroot. Closes: #542915
Whilst pbuilder is quite careful to minimise the risk of accidental data
removal (with find -xdev, unmounting everything it mounts, etc), there is
one case that it doesn't handle -- directories on the same filesystem as the
chroot being bind-mounted into the chroot, because -xdev doesn't recognise
the mount as a separate filesystem and pbuilder doesn't know to unmount it
because it didn't mount it in the first place.
This patch acts as a final safety net, to avoid attempting to delete
anything if there is anything at all still mounted in the chroot.
---
pbuilder-modules | 20 +---
1 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/pbuilder-modules b/pbuilder-modules
index db6988b..8e23cdb 100644
--- a/pbuilder-modules
+++ b/pbuilder-modules
@@ -319,9 +319,23 @@ function cleanbuildplace () {
log W: Aborting with an error;
fi
if [ ${INTERNAL_BUILD_UML} != yes ]; then
- if [ -d $BUILDPLACE ]; then
- log I: cleaning the build env
- clean_subdirectories $BUILDPLACE
+if [ -d $BUILDPLACE ]; then
+# A directory on the same partition as $BUILDPLACE, bind-mounted
+# into $BUILDPLACE, will be cleaned out by clean_subdirectories
+# (because -xdev doesn't know about bind mounts). To avoid that
+# potential disaster (and also to avoid ugly error messages from
+# rmdir otherwise), we want to make sure that there is *nothing*
+# mounted under the chroot before we do our bulldozer routine.
+#
+# The readlink -f is a simple way to canonicalize the path for
+# $BUILDPLACE (no dirty double slashes for *us*), so it matches
+# what will be in the output of mount.
+if mount |grep -q $(readlink -f $BUILDPLACE)/; then
+log E: Something is still mounted under ${BUILDPLACE}; unmount and remove ${BUILDPLACE} manually
+else
+log I: cleaning the build env
+clean_subdirectories $BUILDPLACE
+fi
fi;
fi
}
--
1.5.6.5
Bug#542915: pbuilder: removes data from bind-mounted directories
submitter 542915 ! thanks * Matthew Palmer mpal...@debian.org, 2010-12-12, 14:07: I'm trying to reproduce this bug, but I can't manage to get pbuilder to behave in the manner you describe -- but I can't find any changes to pbuilder in the meantime that would explain a change in behaviour. Thanks for taking care of this bug. Here is what I did, using pbuilder 0.199: 1) Created a minimal package that failed to build 2) mkdir /tmp/hooks 3) cp /usr/share/pbuilder/examples/C10shell /tmp/hooks s,pbuilder,doc/pbuilder, 4) pbuilder --build --basetgz ~/chroots/sid.tgz --hookdir /tmp/hooks broken.dsc 5) Wait for build to fail and drop into a shell; switch to another xterm 6) mkdir /tmp/valuable 7) touch /tmp/valuable/do-not-delete-please 8) mount -o bind /tmp/valuable /var/cache/pbuilder/build/X/home 9) Switch back to the pbuilder xterm; exit out of the shell Now pbuilder reports a couple of errors: rmdir: failed to remove `/var/cache/pbuilder/build//X/home': Device or resource busy rmdir: failed to remove `/var/cache/pbuilder/build//X': Directory not empty Yup, this is exactly what I was doing. But if I ls /tmp/valuable, the file do-not-delete-please is still there. Are /tmp/valuable and /var/cache/pbuilder/build/X/ on the same partition? If they are not, the bug won't trigger. Would you be able to tell me where my attempt at reproducing the problem differs from your experience, or let me know if the bug no longer appears in pbuilder 0.199? I can reproduce this bug with pbuilder 0.199 using your procedure. -- Jakub Wilk -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
On Sun, Dec 12, 2010 at 12:08:57PM +0100, Jakub Wilk wrote: 3) cp /usr/share/pbuilder/examples/C10shell /tmp/hooks s,pbuilder,doc/pbuilder, Ah, yes, sorry about that. But if I ls /tmp/valuable, the file do-not-delete-please is still there. Are /tmp/valuable and /var/cache/pbuilder/build/X/ on the same partition? If they are not, the bug won't trigger. Eureka! That was the vital bit of the process I was missing. Thanks for that, I'm pretty sure I can hunt it down now. - Matt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
tag 542915 +moreinfo thanks Hi Jakub, I'm trying to reproduce this bug, but I can't manage to get pbuilder to behave in the manner you describe -- but I can't find any changes to pbuilder in the meantime that would explain a change in behaviour. Here is what I did, using pbuilder 0.199: 1) Created a minimal package that failed to build 2) mkdir /tmp/hooks 3) cp /usr/share/pbuilder/examples/C10shell /tmp/hooks 4) pbuilder --build --basetgz ~/chroots/sid.tgz --hookdir /tmp/hooks broken.dsc 5) Wait for build to fail and drop into a shell; switch to another xterm 6) mkdir /tmp/valuable 7) touch /tmp/valuable/do-not-delete-please 8) mount -o bind /tmp/valuable /var/cache/pbuilder/build/X/home 9) Switch back to the pbuilder xterm; exit out of the shell Now pbuilder reports a couple of errors: rmdir: failed to remove `/var/cache/pbuilder/build//X/home': Device or resource busy rmdir: failed to remove `/var/cache/pbuilder/build//X': Directory not empty But if I ls /tmp/valuable, the file do-not-delete-please is still there. Would you be able to tell me where my attempt at reproducing the problem differs from your experience, or let me know if the bug no longer appears in pbuilder 0.199? Thanks, - Matt -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
On Fri, Sep 18, 2009, Jakub Wilk wrote: First of all, you should not close a bug without a proper explanation. mount and rm does exactly what they are meant to do (and what is documented). This is not true for your package. In fact, this bug is easily fixable: - In most cases, `umount -a` called inside chroot does unmount everything that should be unmounted. - If it does not (i.e. if /proc/mounts contents changed during a build), pbuilder should refrain from rm-Rf-ing the chroot. It seems pbuilder doesn't rm -r (it uses find -xdev to rm -f all non-directories and then rmdir on dirs; it could as well use rm --one-file-system IMO) see clean_subdirectories() in pbuilder-modules; it also has some logic to check for mounts not having been properly unmounted (seems_truly_unmounted() in pbuilder-modules). Perhaps you're hitting another rm such as the hooks one? -- Loïc Minier -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
* Loïc Minier l...@dooz.org, 2009-09-21, 15:46: First of all, you should not close a bug without a proper explanation. mount and rm does exactly what they are meant to do (and what is documented). This is not true for your package. In fact, this bug is easily fixable: - In most cases, `umount -a` called inside chroot does unmount everything that should be unmounted. - If it does not (i.e. if /proc/mounts contents changed during a build), pbuilder should refrain from rm-Rf-ing the chroot. It seems pbuilder doesn't rm -r (it uses find -xdev to rm -f all non-directories and then rmdir on dirs; it could as well use rm --one-file-system IMO) see clean_subdirectories() in pbuilder-modules; None of these methods are safe for bind-mounts: # mkdir foo # touch foo/foo1 foo/foo2 # mkdir -p bar/foo # mount --bind foo bar/foo # find bar -xdev bar bar/foo bar/foo/foo1 bar/foo/foo2 # rm -Rf --one-file-system bar/ rm: cannot remove directory `bar/foo': Device or resource busy # ls foo/ | wc -l 0 it also has some logic to check for mounts not having been properly unmounted (seems_truly_unmounted() in pbuilder-modules). Yes, but these checks are only for things that were mounted by pbuilder itself (if I understand correctly). Perhaps you're hitting another rm such as the hooks one? Nope, I'm using only C10shell. -- Jakub Wilk -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
* Junichi Uekawa dan...@netfort.gr.jp, 2009-09-19, 00:20: Yeah, reassign this bug to 'mount' and 'rm'. First of all, you should not close a bug without a proper explanation. mount and rm does exactly what they are meant to do (and what is documented). This is not true for your package. In fact, this bug is easily fixable: - In most cases, `umount -a` called inside chroot does unmount everything that should be unmounted. - If it does not (i.e. if /proc/mounts contents changed during a build), pbuilder should refrain from rm-Rf-ing the chroot. -- Jakub Wilk -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#542915: pbuilder: removes data from bind-mounted directories
Package: pbuilder Version: 0.189 Severity: grave Justification: causes data loss How to lose your data with pbuilder: 0. Install the C10shell hook. 1. Try to build a package which happens to FTBFS. 2. Realize that you need some files from your $HOME to investigate the failure. 3. Bind-mount $HOME into the chroot. 4. Investigate things, logout from the chroot. 5. Say good-bye to your data. -- Jakub Wilk -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
