Debian Bug report logs - #588077
/usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Andreas Beckmann <debian@abeckmann.de>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Sun, 04 Jul 2010 18:57:16 +0200

[Message part 1 (text/plain, inline)]

Package: dpkg
Version: 1.15.7.2
Severity: important
File: /usr/bin/dpkg-divert
Tags: patch

Hi,

dpkg-divert --add --rename incorrectly renames an existing file that is
owned by the package given with --package. For example see the following
commands:

# dpkg -S /usr/bin/users
coreutils: /usr/bin/users
# ls -la /usr/bin/users*
-rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users
# dpkg-divert --add --rename --package coreutils --divert /usr/bin/users.diverted /usr/bin/users
Adding `diversion of /usr/bin/users to /usr/bin/users.diverted by coreutils'
# ls -la /usr/bin/users*
-rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users.diverted

Oops, that should not have happened!


A patch that adds a check for the owning package of the to be renamed
file is attached.


Andreas

-- System Information:
Debian Release: squeeze/sid
  APT prefers testing
  APT policy: (800, 'testing'), (800, 'stable'), (600, 'unstable'), (130, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.31-0-amd64 (SMP w/4 CPU cores)
Locale: LANG=C, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages dpkg depends on:
ii  coreutils         8.5-1                  GNU core utilities
ii  libbz2-1.0        1.0.5-4                high-quality block-sorting file co
ii  libc6             2.11.1-3               Embedded GNU C Library: Shared lib
ii  libselinux1       2.0.94-1               SELinux runtime shared libraries
ii  xz-utils          4.999.9beta+20100527-1 XZ-format compression utilities
ii  zlib1g            1:1.2.3.4.dfsg-3       compression library - runtime

dpkg recommends no packages.

Versions of packages dpkg suggests:
ii  apt                           0.7.25.3   Advanced front-end for dpkg

-- no debconf information

[dpkg-divert_checkowner_before_rename.patch (text/plain, attachment)]

Message #12 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Colin Watson <cjwatson@ubuntu.com>

To: Andreas Beckmann <debian@abeckmann.de>, 588077@bugs.debian.org

Cc: Steve Langasek <steve.langasek@ubuntu.com>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 11:38:20 +0100

On Sun, Jul 04, 2010 at 06:57:16PM +0200, Andreas Beckmann wrote:
> dpkg-divert --add --rename incorrectly renames an existing file that is
> owned by the package given with --package. For example see the following
> commands:
> 
> # dpkg -S /usr/bin/users
> coreutils: /usr/bin/users
> # ls -la /usr/bin/users*
> -rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users
> # dpkg-divert --add --rename --package coreutils --divert /usr/bin/users.diverted /usr/bin/users
> Adding `diversion of /usr/bin/users to /usr/bin/users.diverted by coreutils'
> # ls -la /usr/bin/users*
> -rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users.diverted
> 
> Oops, that should not have happened!
> 
> 
> A patch that adds a check for the owning package of the to be renamed
> file is attached.

We ran into this in Ubuntu due to chaos with linker conflicts in
libc6-i386 vs. libc6:i386, and wished that we'd had this in advance.

How about something like this for a C version?  I'd appreciate review as
I'm not entirely familiar with all the internal interfaces here.

diff --git a/src/divertcmd.c b/src/divertcmd.c
index 6d3c8bf..39fa25b 100644
--- a/src/divertcmd.c
+++ b/src/divertcmd.c
@@ -47,6 +47,7 @@
 #include <dpkg/options.h>
 
 #include "filesdb.h"
+#include "main.h"
 
 
 static const char printforhelp[] = N_(
@@ -171,8 +172,10 @@ check_writable_dir(struct file *f)
 }
 
 static bool
-check_rename(struct file *src, struct file *dst)
+check_rename(struct file *src, struct file *dst, struct pkgset *set)
 {
+	struct pkginfo *pkg;
+
 	file_stat(src);
 
 	/* If the source file is not present and we are not going to do
@@ -202,6 +205,24 @@ check_rename(struct file *src, struct file *dst)
 		         "  different file `%s', not allowed"),
 		        dst->name, src->name);
 
+	if (set) {
+		for (pkg = &set->pkg; pkg; pkg = pkg->arch_next) {
+			struct fileinlist *file;
+
+			ensure_packagefiles_available(pkg);
+			file = pkg->clientdata->files;
+			while (file) {
+				if (strcmp (file->namenode->name,
+				            src->name) == 0) {
+					/* Owned by the same package as
+					 * given in --package; don't rename.
+					 */
+					return false;
+				}
+			}
+		}
+	}
+
 	return true;
 }
 
@@ -467,7 +488,7 @@ diversion_add(const char *const *argv)
 	if (opt_verbose > 0)
 		printf(_("Adding '%s'\n"), diversion_describe(contest));
 	if (opt_rename)
-		opt_rename = check_rename(&file_from, &file_to);
+		opt_rename = check_rename(&file_from, &file_to, pkgset);
 	if (!opt_test) {
 		divertdb_write();
 		if (opt_rename)
@@ -576,7 +597,7 @@ diversion_remove(const char *const *argv)
 	altname->camefrom->divert = NULL;
 
 	if (opt_rename)
-		opt_rename = check_rename(&file_to, &file_from);
+		opt_rename = check_rename(&file_to, &file_from, pkgset);
 	if (opt_rename && !opt_test)
 		file_rename(&file_to, &file_from);
 
diff --git a/src/t/100_dpkg_divert.t b/src/t/100_dpkg_divert.t
index 6d62fe5..cbd0436 100644
--- a/src/t/100_dpkg_divert.t
+++ b/src/t/100_dpkg_divert.t
@@ -33,11 +33,12 @@ if (! -x "@dd") {
     exit(0);
 }
 
-plan tests => 251;
+plan tests => 257;
 
 sub cleanup {
     system("rm -rf $tmpdir && mkdir -p $testdir");
-    system("mkdir -p $admindir/updates && touch $admindir/status");
+    system("mkdir -p $admindir/updates && rm -f $admindir/status && touch $admindir/status");
+    system("rm -rf $admindir/info && mkdir -p $admindir/info");
 }
 
 sub install_diversions {
@@ -47,6 +48,27 @@ sub install_diversions {
     close(O);
 }
 
+sub install_filelist {
+    my ($pkg, $arch, @files) = @_;
+    open(L, "> $admindir/info/$pkg.list");
+    for my $file (@files) {
+        print L "$file\n";
+    }
+    close(L);
+    # Only installed packages have their files list considered.
+    open(S, ">> $admindir/status");
+    print S <<EOF;
+Package: $pkg
+Status: install ok installed
+Version: 0
+Architecture: $arch
+Maintainer: dummy
+Description: dummy
+
+EOF
+    close(S);
+}
+
 sub call {
     my ($prog, $args, %opts) = @_;
 
@@ -392,6 +414,25 @@ EOF
 
 cleanup();
 
+note("Adding diversion of file owned by --package");
+
+install_filelist("coreutils", "i386", "$testdir/foo");
+system("cat $admindir/info/coreutils.list >&2");
+install_diversions('');
+system("touch $testdir/foo");
+
+call_divert(['--quiet', '--rename', '--add', '--package', 'coreutils', "$testdir/foo"],
+            expect_stderr => '', expect_stdout => '');
+ok(-e "$testdir/foo", "foo not renamed");
+ok(!-e "$testdir/foo.distrib", "foo renamed");
+diversions_eq(<<EOF);
+$testdir/foo
+$testdir/foo.distrib
+coreutils
+EOF
+
+cleanup();
+
 note("Remove diversions");
 
 install_diversions('');

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Message #17 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Colin Watson <cjwatson@ubuntu.com>, 588077@bugs.debian.org

Cc: Andreas Beckmann <debian@abeckmann.de>, Steve Langasek <steve.langasek@ubuntu.com>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 14:42:02 +0200

[Message part 1 (text/plain, inline)]

Hi,

On Wed, 2012-04-18 at 11:38:20 +0100, Colin Watson wrote:
> On Sun, Jul 04, 2010 at 06:57:16PM +0200, Andreas Beckmann wrote:
> > dpkg-divert --add --rename incorrectly renames an existing file that is
> > owned by the package given with --package. For example see the following
> > commands:
> > 
> > # dpkg -S /usr/bin/users
> > coreutils: /usr/bin/users
> > # ls -la /usr/bin/users*
> > -rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users
> > # dpkg-divert --add --rename --package coreutils --divert /usr/bin/users.diverted /usr/bin/users
> > Adding `diversion of /usr/bin/users to /usr/bin/users.diverted by coreutils'
> > # ls -la /usr/bin/users*
> > -rwxr-xr-x 1 root root 28368 Apr 28 04:05 /usr/bin/users.diverted
> > 
> > Oops, that should not have happened!

> > A patch that adds a check for the owning package of the to be renamed
> > file is attached.

> We ran into this in Ubuntu due to chaos with linker conflicts in
> libc6-i386 vs. libc6:i386, and wished that we'd had this in advance.
> 
> How about something like this for a C version?  I'd appreciate review as
> I'm not entirely familiar with all the internal interfaces here.

I had this one on my list of bugs to look to get fixed for 1.16.3 or
1.16.4 anyway, so let's see:

> diff --git a/src/divertcmd.c b/src/divertcmd.c
> index 6d3c8bf..39fa25b 100644
> --- a/src/divertcmd.c
> +++ b/src/divertcmd.c
> @@ -171,8 +172,10 @@ check_writable_dir(struct file *f)
>  }
>  
>  static bool
> -check_rename(struct file *src, struct file *dst)
> +check_rename(struct file *src, struct file *dst, struct pkgset *set)
>  {
> +	struct pkginfo *pkg;
> +
>  	file_stat(src);

I don't think it seems like a good idea to let the caller insert this
kind of bogus diversion, dpkg itself will try to do the rename on unpack
which would mess the system up anyway (but I've not though about this long
enough). So it would seem better at first sight to just abort on --add.

>  	/* If the source file is not present and we are not going to do
> @@ -202,6 +205,24 @@ check_rename(struct file *src, struct file *dst)
>  		         "  different file `%s', not allowed"),
>  		        dst->name, src->name);
>  
> +	if (set) {
> +		for (pkg = &set->pkg; pkg; pkg = pkg->arch_next) {
> +			struct fileinlist *file;
> +
> +			ensure_packagefiles_available(pkg);
> +			file = pkg->clientdata->files;
> +			while (file) {
> +				if (strcmp (file->namenode->name,
> +				            src->name) == 0) {
> +					/* Owned by the same package as
> +					 * given in --package; don't rename.
> +					 */
> +					return false;
> +				}
> +			}

This loop will only work for packages with exactly one matching file,
or none. :)

In any case, it's better to check which packages own the file instead
of trying to go finding the file in the list of files, which will be
certainly more work.

I'm attaching a preliminary patch I just cooked, but not tested which
would do what I describe above.

thanks,
guillem

[dpkg-divert-owned.patch (text/x-diff, attachment)]

Message #22 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Colin Watson <cjwatson@ubuntu.com>

To: Guillem Jover <guillem@debian.org>

Cc: 588077@bugs.debian.org, Andreas Beckmann <debian@abeckmann.de>, Steve Langasek <steve.langasek@ubuntu.com>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 14:55:48 +0100

On Wed, Apr 18, 2012 at 02:42:02PM +0200, Guillem Jover wrote:
> On Wed, 2012-04-18 at 11:38:20 +0100, Colin Watson wrote:
> > We ran into this in Ubuntu due to chaos with linker conflicts in
> > libc6-i386 vs. libc6:i386, and wished that we'd had this in advance.
> > 
> > How about something like this for a C version?  I'd appreciate review as
> > I'm not entirely familiar with all the internal interfaces here.
> 
> I had this one on my list of bugs to look to get fixed for 1.16.3 or
> 1.16.4 anyway, so let's see:

Great, thanks for the help.

> >  	/* If the source file is not present and we are not going to do
> > @@ -202,6 +205,24 @@ check_rename(struct file *src, struct file *dst)
> >  		         "  different file `%s', not allowed"),
> >  		        dst->name, src->name);
> >  
> > +	if (set) {
> > +		for (pkg = &set->pkg; pkg; pkg = pkg->arch_next) {
> > +			struct fileinlist *file;
> > +
> > +			ensure_packagefiles_available(pkg);
> > +			file = pkg->clientdata->files;
> > +			while (file) {
> > +				if (strcmp (file->namenode->name,
> > +				            src->name) == 0) {
> > +					/* Owned by the same package as
> > +					 * given in --package; don't rename.
> > +					 */
> > +					return false;
> > +				}
> > +			}
> 
> This loop will only work for packages with exactly one matching file,
> or none. :)

Whoops, yes!

> I'm attaching a preliminary patch I just cooked, but not tested which
> would do what I describe above.

This at least passes the test suite in conjunction with the following
test patch.

Steve, I think I missed the specifics of what you wanted to do with
libc6-i386/libc6:i386.  Would dpkg-divert exiting non-zero have been a
problem here?

diff --git a/src/t/100_dpkg_divert.t b/src/t/100_dpkg_divert.t
index 6d62fe5..e4f5079 100644
--- a/src/t/100_dpkg_divert.t
+++ b/src/t/100_dpkg_divert.t
@@ -33,11 +33,12 @@ if (! -x "@dd") {
     exit(0);
 }
 
-plan tests => 251;
+plan tests => 257;
 
 sub cleanup {
     system("rm -rf $tmpdir && mkdir -p $testdir");
-    system("mkdir -p $admindir/updates && touch $admindir/status");
+    system("mkdir -p $admindir/updates && rm -f $admindir/status && touch $admindir/status");
+    system("rm -rf $admindir/info && mkdir -p $admindir/info");
 }
 
 sub install_diversions {
@@ -47,6 +48,27 @@ sub install_diversions {
     close(O);
 }
 
+sub install_filelist {
+    my ($pkg, $arch, @files) = @_;
+    open(L, "> $admindir/info/$pkg.list");
+    for my $file (@files) {
+        print L "$file\n";
+    }
+    close(L);
+    # Only installed packages have their files list considered.
+    open(S, ">> $admindir/status");
+    print S <<EOF;
+Package: $pkg
+Status: install ok installed
+Version: 0
+Architecture: $arch
+Maintainer: dummy
+Description: dummy
+
+EOF
+    close(S);
+}
+
 sub call {
     my ($prog, $args, %opts) = @_;
 
@@ -392,6 +414,23 @@ EOF
 
 cleanup();
 
+note("Adding diversion of file owned by --package");
+
+install_filelist("coreutils", "i386", "$testdir/foo");
+system("cat $admindir/info/coreutils.list >&2");
+install_diversions('');
+system("touch $testdir/foo");
+
+call_divert(['--quiet', '--rename', '--add', '--package', 'coreutils', "$testdir/foo"],
+            expect_failure => 1,
+            expect_stderr_like => qr/owned by same package/,
+            expect_stdout => '');
+ok(-e "$testdir/foo", "foo not renamed");
+ok(!-e "$testdir/foo.distrib", "foo renamed");
+diversions_eq('');
+
+cleanup();
+
 note("Remove diversions");
 
 install_diversions('');

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Message #27 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Colin Watson <cjwatson@ubuntu.com>

To: Guillem Jover <guillem@debian.org>

Cc: 588077@bugs.debian.org, Andreas Beckmann <debian@abeckmann.de>, Steve Langasek <steve.langasek@ubuntu.com>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 15:15:09 +0100

On Wed, Apr 18, 2012 at 02:55:48PM +0100, Colin Watson wrote:
> This at least passes the test suite in conjunction with the following
> test patch.

Er, with the obvious debugging line removed, just to see if you were
paying attention ...

> +system("cat $admindir/info/coreutils.list >&2");

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Message #32 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Colin Watson <cjwatson@ubuntu.com>, 588077@bugs.debian.org

Cc: Andreas Beckmann <debian@abeckmann.de>, Steve Langasek <steve.langasek@ubuntu.com>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 16:17:30 +0200

On Wed, 2012-04-18 at 14:42:02 +0200, Guillem Jover wrote:
> I don't think it seems like a good idea to let the caller insert this
> kind of bogus diversion, dpkg itself will try to do the rename on unpack
> which would mess the system up anyway (but I've not though about this long
> enough). So it would seem better at first sight to just abort on --add.

Hmm thinking about it, this does not make much sense. As such diversion
is legitimate and would be equivalent to one w/o --rename, what might be
questionable is the time it gets inserted, so I guess not doing the
rename might be the right thing to do, I'll ponder about it a bit more
after lunch.

thanks,
guillem

Message #37 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Steve Langasek <steve.langasek@ubuntu.com>

To: Colin Watson <cjwatson@ubuntu.com>, Guillem Jover <guillem@debian.org>

Cc: 588077@bugs.debian.org, Andreas Beckmann <debian@abeckmann.de>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 14:20:56 -0700

[Message part 1 (text/plain, inline)]

Hey there,

On Wed, Apr 18, 2012 at 02:55:48PM +0100, Colin Watson wrote:
> Steve, I think I missed the specifics of what you wanted to do with
> libc6-i386/libc6:i386.  Would dpkg-divert exiting non-zero have been a
> problem here?

Yes, it would.  The problem we're trying to solve is that libc6:i386
currently has a Replaces: on libc6-i386 so that it can take over the PI.
However, this means that if the user later removes libc6:i386, the PI (now
owned by this package) is removed, and packages depending on libc6-i386 now
don't work.

The proposed solution was for libc6:i386 to apply a divert to
/lib/ld-linux.so.2 in its preinst, to preserve any other packages' copies of
this file in a different location so that they can be restored on removal. 
However, libc6:i386 is not always the foreign-arch libc, sometimes it's the
transitively-essential one that everything else depends on.  So to do this,
we *must* be able to tell dpkg-divert "always record the diversion, but move
the file IFF it belongs to a different package."  This is what we expected
from --rename --package, but we hit this bug.

So the expected behavior is:

 - libc6-i386 unpacked, libc6:i386 not yet installed:  record diversion,
   move /lib/ld-linux.so.2
 - libc6-i386 not installed, libc6:i386 not yet installed:  record diversion,
   nothing moves
 - libc6-i386 unpacked, libc6:i386 already installed:  record diversion,
   nothing moves, next upgrade of libc6-i386 redirects to the diverted
   location
 - libc6-i386 not installed, libc6:i386 already installed:  record
   diversion, nothing moves

On Wed, Apr 18, 2012 at 04:17:30PM +0200, Guillem Jover wrote:
> On Wed, 2012-04-18 at 14:42:02 +0200, Guillem Jover wrote:
> > I don't think it seems like a good idea to let the caller insert this
> > kind of bogus diversion, dpkg itself will try to do the rename on unpack
> > which would mess the system up anyway (but I've not though about this long
> > enough). So it would seem better at first sight to just abort on --add.

> Hmm thinking about it, this does not make much sense. As such diversion
> is legitimate and would be equivalent to one w/o --rename, what might be
> questionable is the time it gets inserted, so I guess not doing the
> rename might be the right thing to do, I'll ponder about it a bit more
> after lunch.

Right, this should be entirely legitimate.  Diverting the PI is an extreme
case (and one we've had to work around differently since we can't rely on
dpkg in previous releases to handle this), but it illustrates why it's
important to be consistent in application of diversions regardless of the
installed state of the named package.

-- 
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
Ubuntu Developer                                    http://www.debian.org/
slangasek@ubuntu.com                                     vorlon@debian.org

[signature.asc (application/pgp-signature, inline)]

Message #42 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Steve Langasek <steve.langasek@ubuntu.com>, 588077@bugs.debian.org

Cc: Colin Watson <cjwatson@ubuntu.com>, Andreas Beckmann <debian@abeckmann.de>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Thu, 19 Apr 2012 01:10:14 +0200

[Message part 1 (text/plain, inline)]

On Wed, 2012-04-18 at 14:20:56 -0700, Steve Langasek wrote:
> On Wed, Apr 18, 2012 at 02:55:48PM +0100, Colin Watson wrote:
> > Steve, I think I missed the specifics of what you wanted to do with
> > libc6-i386/libc6:i386.  Would dpkg-divert exiting non-zero have been a
> > problem here?
> 
> Yes, it would.  The problem we're trying to solve is that libc6:i386
> currently has a Replaces: on libc6-i386 so that it can take over the PI.
> However, this means that if the user later removes libc6:i386, the PI (now
> owned by this package) is removed, and packages depending on libc6-i386 now
> don't work.
> 
> The proposed solution was for libc6:i386 to apply a divert to
> /lib/ld-linux.so.2 in its preinst, to preserve any other packages' copies of
> this file in a different location so that they can be restored on removal. 
> However, libc6:i386 is not always the foreign-arch libc, sometimes it's the
> transitively-essential one that everything else depends on.  So to do this,
> we *must* be able to tell dpkg-divert "always record the diversion, but move
> the file IFF it belongs to a different package."  This is what we expected
> from --rename --package, but we hit this bug.
> 
> So the expected behavior is:
> 
>  - libc6-i386 unpacked, libc6:i386 not yet installed:  record diversion,
>    move /lib/ld-linux.so.2

This has the problem that it introduces a time window where there's no
interpreter for i386 on the system.

>  - libc6-i386 not installed, libc6:i386 not yet installed:  record diversion,
>    nothing moves
>  - libc6-i386 unpacked, libc6:i386 already installed:  record diversion,
>    nothing moves, next upgrade of libc6-i386 redirects to the diverted
>    location

And this one (assuming the Replaces are still in place) that if
libc6:i386 gets removed before libc6-i386 is upgraded then there will
be no interpreter left.

>  - libc6-i386 not installed, libc6:i386 already installed:  record
>    diversion, nothing moves

So in any case regardless of the fix for this particular bug, I don't
think dpkg-divert is the right solution for this specific problem. In
this case I think what would make more sense is for Replaces to not
make packages lose track of owned files, just replace them on disk,
which would solve all instances of this problem. But these new
semantics would need to be carefully considered, probably the safest
would be to not expose the shared owning and just track them
internally, but I think there's already a bug related to this.

> On Wed, Apr 18, 2012 at 04:17:30PM +0200, Guillem Jover wrote:
> > On Wed, 2012-04-18 at 14:42:02 +0200, Guillem Jover wrote:
> > > I don't think it seems like a good idea to let the caller insert this
> > > kind of bogus diversion, dpkg itself will try to do the rename on unpack
> > > which would mess the system up anyway (but I've not though about this long
> > > enough). So it would seem better at first sight to just abort on --add.
> 
> > Hmm thinking about it, this does not make much sense. As such diversion
> > is legitimate and would be equivalent to one w/o --rename, what might be
> > questionable is the time it gets inserted, so I guess not doing the
> > rename might be the right thing to do, I'll ponder about it a bit more
> > after lunch.
> 
> Right, this should be entirely legitimate.  Diverting the PI is an extreme
> case (and one we've had to work around differently since we can't rely on
> dpkg in previous releases to handle this), but it illustrates why it's
> important to be consistent in application of diversions regardless of the
> installed state of the named package.

Sure, I agree on the correctness of this fix because dpkg-divert
should be resilient against any kind of state the system might be
in, but I think I'd still question the correctness of such usage.

Attached a new patch.

thanks,
guillem

[dpkg-divert-owned.patch (text/x-diff, attachment)]

Message #47 received at 588077@bugs.debian.org (full text, mbox, reply):

From: Steve Langasek <steve.langasek@ubuntu.com>

To: Guillem Jover <guillem@debian.org>

Cc: 588077@bugs.debian.org, Colin Watson <cjwatson@ubuntu.com>, Andreas Beckmann <debian@abeckmann.de>

Subject: Re: Bug#588077: /usr/bin/dpkg-divert: --add --rename should not rename a file currently owned by --package

Date: Wed, 18 Apr 2012 16:54:02 -0700

[Message part 1 (text/plain, inline)]

On Thu, Apr 19, 2012 at 01:10:14AM +0200, Guillem Jover wrote:
> > So the expected behavior is:

> >  - libc6-i386 unpacked, libc6:i386 not yet installed:  record diversion,
> >    move /lib/ld-linux.so.2

> This has the problem that it introduces a time window where there's no
> interpreter for i386 on the system.

That's not a problem because the only unpacked package providing the PI is
libc6-i386, which cannot possibly be a dependency of any essential packages;
therefore we know that i386 is a foreign arch and it's safe to have a window
where it's missing.

> >  - libc6-i386 not installed, libc6:i386 not yet installed:  record diversion,
> >    nothing moves
> >  - libc6-i386 unpacked, libc6:i386 already installed:  record diversion,
> >    nothing moves, next upgrade of libc6-i386 redirects to the diverted
> >    location

> And this one (assuming the Replaces are still in place) that if
> libc6:i386 gets removed before libc6-i386 is upgraded then there will
> be no interpreter left.

Yep, that's the existing problem we're trying to solve, which would involve
dropping the Replaces.  But the only way to do that would be if we have a
reliable dpkg-divert.

> >  - libc6-i386 not installed, libc6:i386 already installed:  record
> >    diversion, nothing moves

> So in any case regardless of the fix for this particular bug, I don't
> think dpkg-divert is the right solution for this specific problem. In
> this case I think what would make more sense is for Replaces to not
> make packages lose track of owned files, just replace them on disk,
> which would solve all instances of this problem.

That's only helpful if you also make Replaces do an implicit diversion, as
Goswin has argued on list in the past.  There's no reason to believe that
installing the replacing package, then removing it and leaving behind the
files whose names happened to collide with those in the replaced package, is
going to leave you in any better state than if you'd just removed the files
entirely; so it's really only interesting if you're going to stash the
replaced files somewhere that they can be restored when the replacing
package is removed.

> Attached a new patch.

Makes sense to me.

-- 
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
Ubuntu Developer                                    http://www.debian.org/
slangasek@ubuntu.com                                     vorlon@debian.org

[signature.asc (application/pgp-signature, inline)]

Message #52 received at 588077-submitter@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: 588077-submitter@bugs.debian.org

Subject: Bug#588077 marked as pending

Date: Fri, 27 Apr 2012 08:31:46 +0000

tag 588077 pending
thanks

Hello,

Bug #588077 reported by you has been fixed in the Git repository. You can
see the changelog below, and you can check the diff of the fix at:

    http://git.debian.org/?p=dpkg/dpkg.git;a=commitdiff;h=148ed36

---
commit 148ed36543f85473517f6ebcc36c0d738a305776
Author: Guillem Jover <guillem@debian.org>
Date:   Fri Apr 20 04:56:06 2012 +0200

    dpkg-divert: Do not rename files owned by the diverting package
    
    If the file is already owned by the package diverting it, that will
    actually mess up the filesystem for no good reason, just ignore the
    request and issue a message stating so.
    
    Closes: #588077

diff --git a/debian/changelog b/debian/changelog
index 3d9f84d..d1804c0 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -31,6 +31,8 @@ dpkg (1.16.3) UNRELEASED; urgency=low
   * Add x32 support to abitable, ostable and triplettable. Closes: #667037
   * Fix start-stop-daemon to work with relative --exec arguments and --chdir.
     Closes: #669047
+  * Ignore request to rename a file owned by the diverting package on
+    «dpkg-divert --add --rename». Closes: #588077
 
   [ Helge Kreutzmann ]
   * Fix a typo in man/dpkg-buildflags.1.

Message #57 received at 588077-close@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: 588077-close@bugs.debian.org

Subject: Bug#588077: fixed in dpkg 1.16.3

Date: Fri, 27 Apr 2012 08:47:46 +0000

Source: dpkg
Source-Version: 1.16.3

We believe that the bug you reported is fixed in the latest version of
dpkg, which is due to be installed in the Debian FTP archive:

dpkg-dev_1.16.3_all.deb
  to main/d/dpkg/dpkg-dev_1.16.3_all.deb
dpkg_1.16.3.dsc
  to main/d/dpkg/dpkg_1.16.3.dsc
dpkg_1.16.3.tar.bz2
  to main/d/dpkg/dpkg_1.16.3.tar.bz2
dpkg_1.16.3_amd64.deb
  to main/d/dpkg/dpkg_1.16.3_amd64.deb
dselect_1.16.3_amd64.deb
  to main/d/dpkg/dselect_1.16.3_amd64.deb
libdpkg-dev_1.16.3_amd64.deb
  to main/d/dpkg/libdpkg-dev_1.16.3_amd64.deb
libdpkg-perl_1.16.3_all.deb
  to main/d/dpkg/libdpkg-perl_1.16.3_all.deb



A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 588077@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Guillem Jover <guillem@debian.org> (supplier of updated dpkg package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Format: 1.8
Date: Fri, 27 Apr 2012 10:10:10 +0200
Source: dpkg
Binary: libdpkg-dev dpkg dpkg-dev libdpkg-perl dselect
Architecture: source amd64 all
Version: 1.16.3
Distribution: unstable
Urgency: low
Maintainer: Dpkg Developers <debian-dpkg@lists.debian.org>
Changed-By: Guillem Jover <guillem@debian.org>
Description: 
 dpkg       - Debian package management system
 dpkg-dev   - Debian package development tools
 dselect    - Debian package management front-end
 libdpkg-dev - Debian package management static library
 libdpkg-perl - Dpkg perl modules
Closes: 21722 155676 155799 552517 588077 664964 665050 666752 667037 669012 669047 670048
Changes: 
 dpkg (1.16.3) unstable; urgency=low
 .
   [ Guillem Jover ]
   * Do not look for newline beyond the read buffer on dpkg-deb extract.
   * Check update-alternative name and link arguments for all commands.
     Closes: #665050
   * Check all dpkg-divert filename arguments to be absolute and to not
     contain newlines. Closes: #21722
   * Print errors while reading the file list files on a new line instead
     of just after the progress percentage. Closes: #552517
   * Document in dpkg-source(1) that patches for source format “3.0 (quilt)”
     are expected to apply without any fuzz. Closes: #666752
     Based on a patch by Luca Capello <luca@pca.it>.
   * Remove redundant -Wformat-security from default dpkg-buildflags, which
     is already implied by -Werror=format-security. Closes: #664964
     Suggested by Peter Eisentraut <petere@debian.org>.
   * Document in dpkg-query(1) that commands producing multiple paragraphs
     will preserve the order of the packages specified on the argument list.
   * Change start-stop-daemon --exec on GNU/Hurd, FreeBSD, NetBSD, OpenBSD
     and Solaris to check for executables matching device and inode numbers
     instead of filenames.
   * Change start-stop-daemon --name on GNU/Hurd to check the process' argv[1]
     in addition to argv[0], to handle both binaries and interpreted scripts.
     Reported by Mats Erik Andersson <mats.andersson@gisladisker.se>.
   * Handle deb format versions as major.minor integers instead of strings or
     floats, the latter being susceptible to parsing errors depending on the
     current locale (although this was only affecting the old deb format).
   * Ignore the minor format version number for deb-split format, unifying
     the behaviour with the deb format.
   * Add support for an abitable containing arch attribute overrides.
   * Add x32 support to abitable, ostable and triplettable. Closes: #667037
   * Fix start-stop-daemon to work with relative --exec arguments and --chdir.
     Closes: #669047
   * Ignore request to rename a file owned by the diverting package on
     «dpkg-divert --add --rename». Closes: #588077
   * Clarify dpkg-gensymbols(1) by way of examples that architecture wildcards
     are supported in symbols files. Closes: #670048
   * Fix memory leak due to Dpkg::Control objects not being garbage-collected.
     Thanks to Ben Harris <bjh21@cam.ac.uk>. Closes: #669012
   * Compute the md5sum hash on unpack for empty files too, so that these
     can be checked correctly for matching content when installing multiple
     package instances.
   * Generate md5sums files automatically at unpack time if missing from the
     binary package. Closes: #155676, #155799
   * Add missing list and md5sums database file checks to «dpkg --audit».
 .
   [ Helge Kreutzmann ]
   * Fix a typo in man/dpkg-buildflags.1.
 .
   [ Updated dpkg translations ]
   * French (Christian Perrier).
   * German (Sven Joachim).
   * Swedish (Peter Krefting).
 .
   [ Updated dselect translations ]
   * French (Christian Perrier).
   * German (Sven Joachim).
   * Swedish (Peter Krefting).
 .
   [ Updated scripts translations ]
   * French (Christian Perrier).
   * Swedish (Peter Krefting).
 .
   [ Updated scripts translations ]
   * French (Christian Perrier).
   * German (Helge Kreutzmann).
   * Swedish (Peter Krefting).
Checksums-Sha1: 
 bb4963fae9946db782220765afbd3743d1896383 1362 dpkg_1.16.3.dsc
 b82a62c1b5e85adcc947f28a264ef7b7ee8580ca 5599915 dpkg_1.16.3.tar.bz2
 29a3fc00855cd8a0365acb11d473b7bf82bed6d5 640096 libdpkg-dev_1.16.3_amd64.deb
 153ac818bcd482b1d42e8823ed4f9334dd40db20 2354904 dpkg_1.16.3_amd64.deb
 4cf4bb413809e3981900ef03a664891bc180b397 1079334 dselect_1.16.3_amd64.deb
 e2d9f21746f0ba2f803fff16f215eb65b56a6792 1184282 dpkg-dev_1.16.3_all.deb
 475bfac569ad4865bf5c9a134b6d55c3e63a85b7 881242 libdpkg-perl_1.16.3_all.deb
Checksums-Sha256: 
 d49eb619ebe10cfdf0ab13ab59a627518ae57e1b3d793ab05f23f105060a44f6 1362 dpkg_1.16.3.dsc
 8048890ca92a3ca317a4fdd557f8e9b2b3ce560743e8e70813496f9a7096d8d8 5599915 dpkg_1.16.3.tar.bz2
 83571879e30c5ee19f17038adb2b3d88604ddeec0d0bd60623c6a4e303782fa3 640096 libdpkg-dev_1.16.3_amd64.deb
 ccfa31ca47729ed42f79367cfa9e6ae3dc5b76616e1708d190c52a1b0f768b26 2354904 dpkg_1.16.3_amd64.deb
 c86ceea341dca6704dc5b4fe63fc9c262751a9fa654d73930acc691c25dec379 1079334 dselect_1.16.3_amd64.deb
 352173d2e19d4b40c0e596e01a5bc41e4f24e8c2514020e73adc38bba9ef7bb0 1184282 dpkg-dev_1.16.3_all.deb
 a25a757012281ce83dde6a727cf1587f18a670c63186964c3192ffc617b94f50 881242 libdpkg-perl_1.16.3_all.deb
Files: 
 c48022e8aacde7046c5b8b9163fea8cb 1362 admin required dpkg_1.16.3.dsc
 20189e2926ada3dda4f77ef2e36999af 5599915 admin required dpkg_1.16.3.tar.bz2
 525113c10668ca1edeccf81f63300562 640096 libdevel optional libdpkg-dev_1.16.3_amd64.deb
 a7096c5f626d1fa1d5cc5b3d0e94f9c7 2354904 admin required dpkg_1.16.3_amd64.deb
 c276e506628a9c218a8fc25343fef1f7 1079334 admin optional dselect_1.16.3_amd64.deb
 e269f80cd82dc477cfeae69edfa07129 1184282 utils optional dpkg-dev_1.16.3_all.deb
 4251ef6c75be3087e2202f4803d4df7a 881242 perl optional libdpkg-perl_1.16.3_all.deb

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iEYEARECAAYFAk+aV7YACgkQuW9ciZ2SjJu8OwCfTAK3+/za0kD9SwhYkxrsVwwF
0YQAoKt6GKTLul6ylFX21YTBbfegF/Xw
=cztM
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.