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
diff --git a/src/divertcmd.c b/src/divertcmd.c
index 6d3c8bf..66c66ba 100644
--- a/src/divertcmd.c
+++ b/src/divertcmd.c
@@ -382,6 +382,31 @@ divertdb_write(void)
free(dbname);
}
+static bool
+diversion_is_owned(struct pkgset *set, struct filenamenode *namenode)
+{
+ struct pkginfo *pkg;
+ struct filepackages_iterator *iter;
+ bool owned = false;
+
+ if (set == NULL)
+ return false;
+
+ for (pkg = &set->pkg; pkg; pkg = pkg->arch_next)
+ ensure_packagefiles_available(pkg);
+
+ iter = filepackages_iter_new(namenode);
+ while ((pkg = filepackages_iter_next(iter))) {
+ if (pkg->set == set) {
+ owned = true;
+ break;
+ }
+ }
+ filepackages_iter_free(iter);
+
+ return owned;
+}
+
static int
diversion_add(const char *const *argv)
{
@@ -449,6 +474,11 @@ diversion_add(const char *const *argv)
diversion_describe(fnn_to->divert));
}
+ /* Check we are not diverting a file owned by the same set. */
+ if (diversion_is_owned(pkgset, fnn_from))
+ ohshit(_("cannot divert file '%s' owned by same package '%s'"),
+ filename, pkgset->name);
+
/* Create new diversion. */
contest = nfmalloc(sizeof(*contest));
altname = nfmalloc(sizeof(*altname));
