[pacman-dev] test/sync999 - pacman segfault
Hi, Running pactest today showed that sync999 test case no longer works. Eventually, pactest return on investment does not seem null anymore :) It seems the flaw appeared with lib/libalpm/conflict.c:1.15. The same test with conflict.c:1.14 is ok. Here's the log from pactest: flow2: PM_OPT_DBPATH set to 'var/lib/pacman' flow2: PM_OPT_CACHEDIR set to 'var/cache/pacman/pkg' debug: opening database 'local' debug: opening database 'sync' :: Starting local database upgrade... flow1: checking for package replacements debug: loading package cache (infolevel=0x3) for repository 'sync' flow1: checking for package upgrades debug: loading package cache (infolevel=0x3) for repository 'local' debug: CACHE: fetching 'sync/pkg1' flow2: pkg1-1.0-1 elected for upgrade (1.0-1 => 1.0-2) debug: CACHE: fetching 'sync/pkg2' flow2: pkg2-1.0-1 elected for upgrade (1.0-1 => 1.0-2) resolving dependencies... flow1: resolving targets dependencies done. looking for inter-conflicts... flow1: looking for unresolvable dependencies debug: CACHE: fetching 'local/pkg1' debug: CACHE: fetching 'local/pkg2' flow1: looking for conflicts debug: targs vs db: found pkg2 as a conflict for pkg1 debug: targs vs targs: found pkg2 as a conflict for pkg1 debug: db vs targs: found pkg1 as a conflict for pkg2 flow2: package 'pkg1' is conflicting with 'pkg2' debug: CACHE: fetching 'local/pkg2' debug: package 'pkg1' provides its own conflict debug: resolving package 'pkg1' conflict flow2: electing 'pkg2' for removal flow2: removing 'pkg2' from target list flow2: package 'pkg2' is conflicting with 'pkg1' debug: CACHE: fetching 'local/pkg1' When handling conflict pkg1, pkg2 is removed from targets. Then, the library tries to handle pkg2 conflict... but find_pkginsync() returns NULL... I've committed a fix (sync.c:1.66) to skip conflict resolution in such a case. Thoughts? The test suite is ok with it ;) -- Aurelien
On Wed, Feb 22, 2006 at 08:57:25PM +0100, Aurelien Foret <aurelien@archlinux.org> wrote:
It seems the flaw appeared with lib/libalpm/conflict.c:1.15. The same test with conflict.c:1.14 is ok.
(...)
I've committed a fix (sync.c:1.66) to skip conflict resolution in such a case. Thoughts?
i would say if the problem is with the patch that improves conflict.c, then why not fixing conflict.c and adding a workaround to sync.c if possible? the sync.c patch contains two hunk: a workaround for the conflict.c problem and a compile fix (would it be a big problem not to mix compile fixes and workarounds?) if you revert the workaround, and apply this patch: http://frugalware.org/~vmiklos/patches/libpacman-proposed/syncfix.diff i hope the test will pass again (this time without the workaround) something partially unrelated: would it be difficult to add an option to pactest to run "fakeroot libtool gdb --args" instead of "fakeroot" and in that case not to redirect the output? for now i've hardwired that here, but that's not a sollution :) udv / greetings, VMiklos -- Developer of Frugalware Linux, to make things frugal - http://frugalware.org
VMiklos wrote:
i would say if the problem is with the patch that improves conflict.c, then why not fixing conflict.c and adding a workaround to sync.c if possible?
Sure.
the sync.c patch contains two hunk: a workaround for the conflict.c problem and a compile fix (would it be a big problem not to mix compile fixes and workarounds?) if you revert the workaround, and apply this patch:
http://frugalware.org/~vmiklos/patches/libpacman-proposed/syncfix.diff
i hope the test will pass again (this time without the workaround)
Which sync.c patch are you mentioning? If it's my patch (sync.c 1.65-1.66), there's only one hunk There's no compile fix in it, just a fix to avoid a segmentation fault. If it's your patch, there are 3 hunks... You're basically switching "j" and "k", and it fixes the issue by chance because you forgot to also switch "j" and "k" at line 159. With your patch applied, at line 159, j->data has become a (pmpkg_t *) that will for sure never make strcmp return 0... Anyway, I think the issue comes from the _alpm_depmiss_new declarations for CHECK3 that are switching tp->name and info->name. I'm attaching a patch based on yours fixing that point. Thoughts?
something partially unrelated: would it be difficult to add an option to pactest to run "fakeroot libtool gdb --args" instead of "fakeroot" and in that case not to redirect the output? for now i've hardwired that here, but that's not a sollution :)
Added to the TODO. Thanks -- Aurelien
On Thu, Feb 23, 2006 at 11:12:30PM +0100, Aurelien Foret <aurelien@archlinux.org> wrote:
the sync.c patch contains two hunk: a workaround for the conflict.c problem and a compile fix (would it be a big problem not to mix compile fixes and workarounds?) if you revert the workaround, and apply this patch: http://frugalware.org/~vmiklos/patches/libpacman-proposed/syncfix.diff i hope the test will pass again (this time without the workaround)
Which sync.c patch are you mentioning?
ah, sorry, i run cvs diff -u -r 1.65 sync.c, not cvs diff -u -r 1.65 sync.c so i got a patch that included two revisions Judd: would it be a big task to include pacman-lib in the cvsweb interface? :) (<flame>man, after darcs, using centralized version control systems are so annoying ;) </flame>)
If it's my patch (sync.c 1.65-1.66), there's only one hunk There's no compile fix in it, just a fix to avoid a segmentation fault.
If it's your patch, there are 3 hunks... You're basically switching "j" and "k", and it fixes the issue by chance because you forgot to also switch "j" and "k" at line 159. With your patch applied, at line 159, j->data has become a (pmpkg_t *) that will for sure never make strcmp return 0...
Anyway, I think the issue comes from the _alpm_depmiss_new declarations for CHECK3 that are switching tp->name and info->name. I'm attaching a patch based on yours fixing that point.
Thoughts?
seems ok to me udv / greetings, VMiklos -- Developer of Frugalware Linux, to make things frugal - http://frugalware.org
On Thu, Feb 23, 2006 at 11:12:30PM +0100, Aurelien Foret <aurelien@archlinux.org> wrote:
something partially unrelated: would it be difficult to add an option to pactest to run "fakeroot libtool gdb --args" instead of "fakeroot" and in that case not to redirect the output? for now i've hardwired that here, but that's not a sollution :)
Added to the TODO.
if we are at pactest todo's, it would be nice to have one or two test for pacman -D and pacman -T :) udv / greetings, VMiklos -- Developer of Frugalware Linux, to make things frugal - http://frugalware.org
participants (2)
-
Aurelien Foret
-
VMiklos