Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   ArchLinux Pacman Development (http://www.linux-archive.org/archlinux-pacman-development/)
-   -   chk_filedifference is buggy!! (http://www.linux-archive.org/archlinux-pacman-development/52663-chk_filedifference-buggy.html)

Nagy Gabor 02-11-2008 01:34 PM

chk_filedifference is buggy!!
 
Part II [the real problem].

----- Tovább*tott levél Nagy Gabor <ngaba@bibl.u-szeged.hu> c*mről -----
Dátum: Sun, 10 Feb 2008 14:40:42 +0100
Feladó: Nagy Gabor <ngaba@bibl.u-szeged.hu>
Viszontválasz c*m: Nagy Gabor <ngaba@bibl.u-szeged.hu>
Tartalom: [BUG] chk_filedifference is buggy!!
C*mzett: Xavier <shiningxc@gmail.com>

Hi!

This is the next part of my previous e-mail.
After some investigation I figured out that chk_filedifference is the reason
of the bug:
The different pB==NULL handling is a good indicator of the bug (which is
totally needless in bugfree chk_filedifference): almost the same happens here.
The problem is the "while(pA && pB)"; if we reach the end of pB first, then
the remaining pA is cut down...
As an illustration I attached an other failing pactest file, which also
demonstrates this clearly (and tests a now not checked case).

Bye
----- Vége a tovább*tott üzenetnek -----


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/
self.description = "Upgrade a package with a filesystem conflict"

p = pmpkg("dummy", "2.0-1")
p.files = ["bin/dummy", "usr/share/file"]
self.addpkg(p)

lp = pmpkg("dummy", "1.0-1")
lp.files = ["bin/dummy"]
self.addpkg2db("local", lp)

self.filesystem = ["usr/share/file"]

self.args = "-U %s" % p.filename()

self.addrule("PACMAN_RETCODE=1")
self.addrule("PKG_VERSION=dummy|1.0-1")
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 02-11-2008 02:50 PM

chk_filedifference is buggy!!
 
On Feb 11, 2008 8:34 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
> Part II [the real problem].
>
> Hi!
>
> This is the next part of my previous e-mail.
> After some investigation I figured out that chk_filedifference is the reason
> of the bug:
> The different pB==NULL handling is a good indicator of the bug (which is
> totally needless in bugfree chk_filedifference): almost the same happens here.
> The problem is the "while(pA && pB)"; if we reach the end of pB first, then
> the remaining pA is cut down...
> As an illustration I attached an other failing pactest file, which also
> demonstrates this clearly (and tests a now not checked case).

Nagy, we've discussed this before in the past. *Please* do not just
point out problems with the expectation that we will fix it. I know
you are a very competent coder, and I'd appreciate you contributing
solutions rather than inferring our code is shit. Saying things like
"totally needless" does not start things off on the right foot with
Aaron and I, who have really tried hard to clean up some questionable
code. We do make mistakes.

diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
index c093705..9ef5c9f 100644
--- a/lib/libalpm/conflict.c
+++ b/lib/libalpm/conflict.c
@@ -279,6 +279,12 @@ static alpm_list_t
*chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
}
}
}
+ /* ensure we have completely emptied pA */
+ while(pA) {
+ const char *strA = pA->data;
+ ret = alpm_list_add(ret, strdup(strA));
+ pA = pA->next;
+ }

return(ret);
}

This patch fixes the revised upgrade040 and 041 pactests, as well as
upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
that issue? I'm not familiar enough with the whole program flow there
yet.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Nagy Gabor 02-11-2008 03:05 PM

chk_filedifference is buggy!!
 
> Nagy, we've discussed this before in the past. *Please* do not just
> point out problems with the expectation that we will fix it. I know
> you are a very competent coder, and I'd appreciate you contributing
> solutions rather than inferring our code is shit. Saying things like
> "totally needless" does not start things off on the right foot with
> Aaron and I, who have really tried hard to clean up some questionable
> code. We do make mistakes.

Oh, I didn't want to say, that your code is shit (however: serious). Instead,
that function is quite tricky, and not easy to understand at all (that's why I
didn't provide a patch yet...: 1. I'm also quite tired/ill nowadays => I don't
trust on myself now; 2. I want to do a bigger rewrite of that part)

The word "totally needless" referred to the following: there is (should be) no
difference between empty and n-element list, so that pB==NULL check is not
needed <- the incorrect handling of pB==NULL was a special case of this BUG.

> diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> index c093705..9ef5c9f 100644
> --- a/lib/libalpm/conflict.c
> +++ b/lib/libalpm/conflict.c
> @@ -279,6 +279,12 @@ static alpm_list_t
> *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
> }
> }
> }
> + /* ensure we have completely emptied pA */
> + while(pA) {
> + const char *strA = pA->data;
> + ret = alpm_list_add(ret, strdup(strA));
> + pA = pA->next;
> + }
>
> return(ret);
> }
>

This seems OK, but the previous code filters out directories, so this is needed
here too (this was missed in the old pB==NULL handler part, too).

> This patch fixes the revised upgrade040 and 041 pactests, as well as
> upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
> that issue? I'm not familiar enough with the whole program flow there
> yet.
>

Yes, for fixing upgrade046.py bigger efforts are needed (however, that is
probably rarer issue). The problem is, when you use --force, the
_alpm_db_find_fileconflicts is not called at all. And this file relocation
"stuff" (skip_remove) is computed in that function; so this computation is not
done at all with --force.

Bye


----------------------------------------------------
SZTE Egyetemi Knyvtr - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 02-11-2008 03:23 PM

chk_filedifference is buggy!!
 
2008/2/11 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
> > Nagy, we've discussed this before in the past. *Please* do not just
> > point out problems with the expectation that we will fix it. I know
> > you are a very competent coder, and I'd appreciate you contributing
> > solutions rather than inferring our code is shit. Saying things like
> > "totally needless" does not start things off on the right foot with
> > Aaron and I, who have really tried hard to clean up some questionable
> > code. We do make mistakes.
>
> Oh, I didn't want to say, that your code is shit (however: serious). Instead,
> that function is quite tricky, and not easy to understand at all (that's why I
> didn't provide a patch yet...: 1. I'm also quite tired/ill nowadays => I don't
> trust on myself now; 2. I want to do a bigger rewrite of that part)
>
> The word "totally needless" referred to the following: there is (should be) no
> difference between empty and n-element list, so that pB==NULL check is not
> needed <- the incorrect handling of pB==NULL was a special case of this BUG.
>
> > diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> > index c093705..9ef5c9f 100644
> > --- a/lib/libalpm/conflict.c
> > +++ b/lib/libalpm/conflict.c
> > @@ -279,6 +279,12 @@ static alpm_list_t
> > *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
> > }
> > }
> > }
> > + /* ensure we have completely emptied pA */
> > + while(pA) {
> > + const char *strA = pA->data;
> > + ret = alpm_list_add(ret, strdup(strA));
> > + pA = pA->next;
> > + }
> >
> > return(ret);
> > }
> >
>
> This seems OK, but the previous code filters out directories, so this is needed
> here too (this was missed in the old pB==NULL handler part, too).

Ahh, you are correct. If you would have said this originally, then I
would understand why this problem was not easy at first sight. :)

> > This patch fixes the revised upgrade040 and 041 pactests, as well as
> > upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
> > that issue? I'm not familiar enough with the whole program flow there
> > yet.
> >
>
> Yes, for fixing upgrade046.py bigger efforts are needed (however, that is
> probably rarer issue). The problem is, when you use --force, the
> _alpm_db_find_fileconflicts is not called at all. And this file relocation
> "stuff" (skip_remove) is computed in that function; so this computation is not
> done at all with --force.

Yeah, interesting. We may have to rework this somehow...hmm.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Nagy Gabor 02-11-2008 04:56 PM

chk_filedifference is buggy!!
 
Idzs Dan McGee <dpmcgee@gmail.com>:

> 2008/2/11 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
> > > Nagy, we've discussed this before in the past. *Please* do not just
> > > point out problems with the expectation that we will fix it. I know
> > > you are a very competent coder, and I'd appreciate you contributing
> > > solutions rather than inferring our code is shit. Saying things like
> > > "totally needless" does not start things off on the right foot with
> > > Aaron and I, who have really tried hard to clean up some questionable
> > > code. We do make mistakes.
> >
> > Oh, I didn't want to say, that your code is shit (however: serious).
> Instead,
> > that function is quite tricky, and not easy to understand at all (that's
> why I
> > didn't provide a patch yet...: 1. I'm also quite tired/ill nowadays => I
> don't
> > trust on myself now; 2. I want to do a bigger rewrite of that part)
> >
> > The word "totally needless" referred to the following: there is (should be)
> no
> > difference between empty and n-element list, so that pB==NULL check is not
> > needed <- the incorrect handling of pB==NULL was a special case of this
> BUG.
> >
> > > diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> > > index c093705..9ef5c9f 100644
> > > --- a/lib/libalpm/conflict.c
> > > +++ b/lib/libalpm/conflict.c
> > > @@ -279,6 +279,12 @@ static alpm_list_t
> > > *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
> > > }
> > > }
> > > }
> > > + /* ensure we have completely emptied pA */
> > > + while(pA) {
> > > + const char *strA = pA->data;
> > > + ret = alpm_list_add(ret, strdup(strA));
> > > + pA = pA->next;
> > > + }
> > >
> > > return(ret);
> > > }
> > >
> >
> > This seems OK, but the previous code filters out directories, so this is
> needed
> > here too (this was missed in the old pB==NULL handler part, too).
>
> Ahh, you are correct. If you would have said this originally, then I
> would understand why this problem was not easy at first sight. :)
>
> > > This patch fixes the revised upgrade040 and 041 pactests, as well as
> > > upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
> > > that issue? I'm not familiar enough with the whole program flow there
> > > yet.
> > >
> >
> > Yes, for fixing upgrade046.py bigger efforts are needed (however, that is
> > probably rarer issue). The problem is, when you use --force, the
> > _alpm_db_find_fileconflicts is not called at all. And this file relocation
> > "stuff" (skip_remove) is computed in that function; so this computation is
> not
> > done at all with --force.
>
> Yeah, interesting. We may have to rework this somehow...hmm.
>
A possible hotfix:
We may call _alpm_db_find_fileconflicts() with --force too (which is in fact a
fileconflict PLUS filerelocation check), and simply ignore its reported
fileconflict errors.

One more small but must-mention issue:
See upgrade041.py and conflict.c:428 and consider the following case: user
deleted /usr/share/file by hand [this is not forbidden, however not suggested],
then this filerelocation simply won't be detected and the issue appear again.

Long run:
Fixing these issues is not easy at all (I mean cleaning up this part: moving
filerelocation stuff to add.c and fix these issues), I'm afraid of a possible
appearing slowdown (see the last issue for example).
On the other hand, transaction rollback will eliminate this whole filerelocation
computation, since then we will do the "final step" (the REAL removal and and
install) at transaction level, not in "package level", so we will just see:
file.OLD-PID, file.NEW-PID at the end or something like this. And this fact is
quite unmotivating :-P
Transaction rollback can also easily fix trans001.py and fileconflict00?.py so
this is just an other reason for implementing it (however, that will be
difficult too ;-P and personally I'm not a big fan of that, but I must admit
that it is needed.)

Bye


----------------------------------------------------
SZTE Egyetemi Knyvtr - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 02-11-2008 06:20 PM

chk_filedifference is buggy!!
 
2008/2/11 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
> Idzs Dan McGee <dpmcgee@gmail.com>:
> > 2008/2/11 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
> > > This seems OK, but the previous code filters out directories, so this is
> > needed
> > > here too (this was missed in the old pB==NULL handler part, too).
> >
> > Ahh, you are correct. If you would have said this originally, then I
> > would understand why this problem was not easy at first sight. :)

Revised patch:

diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
index c093705..3442902 100644
--- a/lib/libalpm/conflict.c
+++ b/lib/libalpm/conflict.c
@@ -251,10 +251,7 @@ static alpm_list_t
*chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
alpm_list_t *ret = NULL;
alpm_list_t *pA = filesA, *pB = filesB;

- if(pB == NULL) {
- return(alpm_list_strdup(pA));
- }
-
+ /* if both filesA and filesB have entries, do this loop */
while(pA && pB) {
const char *strA = pA->data;
const char *strB = pB->data;
@@ -279,6 +276,15 @@ static alpm_list_t
*chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
}
}
}
+ /* ensure we have completely emptied pA */
+ while(pA) {
+ const char *strA = pA->data;
+ /* skip directories */
+ if(strA[strlen(strA)-1] != '/') {
+ ret = alpm_list_add(ret, strdup(strA));
+ }
+ pA = pA->next;
+ }

return(ret);
}

> A possible hotfix:
> We may call _alpm_db_find_fileconflicts() with --force too (which is in fact a
> fileconflict PLUS filerelocation check), and simply ignore its reported
> fileconflict errors.
>
> One more small but must-mention issue:
> See upgrade041.py and conflict.c:428 and consider the following case: user
> deleted /usr/share/file by hand [this is not forbidden, however not suggested],
> then this filerelocation simply won't be detected and the issue appear again.
>
> Long run:
> Fixing these issues is not easy at all (I mean cleaning up this part: moving
> filerelocation stuff to add.c and fix these issues), I'm afraid of a possible
> appearing slowdown (see the last issue for example).
> On the other hand, transaction rollback will eliminate this whole filerelocation
> computation, since then we will do the "final step" (the REAL removal and and
> install) at transaction level, not in "package level", so we will just see:
> file.OLD-PID, file.NEW-PID at the end or something like this. And this fact is
> quite unmotivating :-P
> Transaction rollback can also easily fix trans001.py and fileconflict00?.py so
> this is just an other reason for implementing it (however, that will be
> difficult too ;-P and personally I'm not a big fan of that, but I must admit
> that it is needed.)

My take:
* When a user uses --force, they should know what they are doing. So
I'm less concerned about fixing that situation than the normal,
not-forced case.
* The above patch fixes the original issue, correct? Where
chk_fileconflicts was not always producing the correct list.

-Dan
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev


All times are GMT. The time now is 01:21 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.