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-rc-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 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 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-rc-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 Thu, Dec 16, 2010 at 06:14:55PM +0100, Jakub Wilk wrote: > * Matthew Palmer , 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 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-rc-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 , 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-rc-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 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
* Loïc Minier , 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-rc-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-rc-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 , 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-rc-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-rc-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org