Historical bug tracker for the Pacman package manager.
The pacman bug tracker has moved to gitlab:
https://gitlab.archlinux.org/pacman/pacman/-/issues
This tracker remains open for interaction with historical bugs during the transition period. Any new bugs reports will be closed without further action.
The pacman bug tracker has moved to gitlab:
https://gitlab.archlinux.org/pacman/pacman/-/issues
This tracker remains open for interaction with historical bugs during the transition period. Any new bugs reports will be closed without further action.
FS#21345 - Sigsegv in libalpm when alpm_db_get_pkgcache is called while dbpath not set up allready
Attached to Project:
Pacman
Opened by Bruno Widmann (bwid) - Tuesday, 19 October 2010, 18:33 GMT
Last edited by Dan McGee (toofishes) - Monday, 13 December 2010, 22:46 GMT
Opened by Bruno Widmann (bwid) - Tuesday, 19 October 2010, 18:33 GMT
Last edited by Dan McGee (toofishes) - Monday, 13 December 2010, 22:46 GMT
|
DetailsSummary and Info:
I am trying my hand at a small pacman frontent. Thus i copied a few lines of code out of pacman to get going with a small libalpm test program. Seems like i had forgotten about the aalpm_option_set_dbpath line (see backtrace). Not sure if this bugreport is really valid, as my code is incomplete. But maybe there should be even more thorough error handling in the library as there allready is. Steps to Reproduce: Compile attached program with gcc -lalpm alpmtest1.c -o alpmtest1 On execution it will segfault in this line: i = alpm_db_get_pkgcache(db_local); Backtrace: (gdb) l 357 ALPM_LOG_FUNC; 358 359 ASSERT(db != NULL, RET_ERR(PM_ERR_DB_NULL, -1)); 360 361 dbpath = _alpm_db_path(db); 362 dbdir = opendir(dbpath); 363 if(dbdir == NULL) { 364 return(0); 365 } 366 while((ent = readdir(dbdir)) != NULL) { (gdb) bt #0 _alpm_db_populate (db=0x6010f0) at be_files.c:362 #1 0x00007ffff7bbb94a in _alpm_db_load_pkgcache (db=0x6010f0) at cache.c:52 #2 0x00007ffff7bbbacf in _alpm_db_get_pkgcache (db=0x6010f0) at cache.c:90 #3 0x00007ffff7bbf06e in alpm_db_get_pkgcache (db=0x6010f0) at db.c:273 #4 0x0000000000400903 in main () at alpmtest1.c:40 (gdb) print dbpath $3 = 0x0 (gdb) n Program received signal SIGSEGV, Segmentation fault. 0x00007ffff78ebc74 in opendir () from /lib/libc.so.6 
alpmtest1.c
(0.8 KiB)
|
This task depends upon
Closed by Dan McGee (toofishes)
Monday, 13 December 2010, 22:46 GMT
Reason for closing: Fixed
Additional comments about closing: commit 5f36523af9de20a
Monday, 13 December 2010, 22:46 GMT
Reason for closing: Fixed
Additional comments about closing: commit 5f36523af9de20a
I would do the following. Changed the return value on error from 0 to -1, because
that is what the calling function is checking for.
diff -r -u pacman-3.4.1.orig//lib/libalpm/be_files.c pacman-3.4.1//lib/libalpm/be_files.c
--- pacman-3.4.1.orig//lib/libalpm/be_files.c 2010-06-24 15:29:21.000000000 +0200
+++ pacman-3.4.1//lib/libalpm/be_files.c 2010-10-19 22:52:53.866662526 +0200
@@ -359,9 +359,12 @@
ASSERT(db != NULL, RET_ERR(PM_ERR_DB_NULL, -1));
dbpath = _alpm_db_path(db);
+ if(dbpath == NULL) {
+ return(-1);
+ }
dbdir = opendir(dbpath);
if(dbdir == NULL) {
- return(0);
+ return(-1);
}
while((ent = readdir(dbdir)) != NULL) {
const char *name = ent->d_name;
Thanks for the patch, even better if you attach it so that we can actually apply it directly with patch / git apply.
Even better is a git formatted patch.
However your patch made me realize that this code changed in git master (things were moved around there). Since this is not a bugfix (just a welcome improvement), one patch against master would be perfect.
Anyway for such a simple change, I can do that myself if you don't feel like it.
However i have now testet the change also with the "master" version, and it looks as if it's allright (segfault with my program goes away).
I'll attach a git patch. Never made one before, so not sure if it worked out correctly.
When reading that change now, I first thought 'wtf', but looking a bit more, it seems I just wanted to handle the absence of a database as 'empty set' and not as an error.
If that's indeed the new behavior (and it still works as expected), then a comment would be good.
But from what little i understand, i have the following opinions.
dbpath = _alpm_db_path(db);
dbdir = opendir(dbpath);
At the moment there is made a destinction here between "no configured path" and "configure path, but can't be opened"
I think those two situations should result in the same return code from _alpm_local_db_populate (be that 0 or -1).
Because why should it matter that much why _exacty_ the populate of the db failed.
As far as i can tell, the only calling function is _alpm_db_load_pkgcache in db.c,
and atm it checks for a return code of -1. In case of 0 it proceeds as normal,
and sets db->pkgcache_loaded.
If return(0) in case of failed opendir in db_populate is the correct thing to do, then i guess that
case should be checked for so that db->pkgcache_loaded is not set in that case.
The filesystem directory dbpath not existing is different, it can be interpreted as 'its a fresh system, there is just nothing installed yet, the dir will be created when we actually install something'
Well, the segfault of my incomplete test-program goes away with just the first check (for dbpath == NULL), so i would be happy if you could put that in.