Re: [pacman-dev] [PATCH] Handle null pkgcache for db_populate, refactor hash

On Mon, Feb 07, 2011 at 09:44:39AM -0600, Dan McGee wrote:

On Mon, Feb 7, 2011 at 8:59 AM, Pang Yan Han wrote:

...

In sync_db_populate() and local_db_populate(), a NULL db->pkgcache is not handled. This not only involves unnecessary operations, but also potentially merge sorting db->pkgcache->list which does not even exist in the case that db->pkgcache is NULL. I'm not following the "unnecessary operations" part of this- do you simply mean we would continue instead of bailing if the function returned NULL? The only case that happens in would be if we ran out of memory (MALLOC, CALLOC) or if we exceeded the maximum size.

Hi yes I meant that the function will continue instead of bailing if db->pkgcache happens to be NULL.

...

This may lead to serious issues since alpm_list_msort calls alpm_list_nth which will dereference invalid pointers. Thus, db->pkgcache is checked during local/sync db_populate calls.

For this purpose, PM_ERR_DB_PKGCACHE_NULL error is added to _pmerrno_t and taken care of in alpm_strerror(). -1 on adding another error code that means nothing to a front-end user. PM_ERR_MEMORY is already set in two of the three NULL exit cases; we should set that in the third as well as greater than maximum size might as well be a memory issue.

So I guess it's ok to set pmerrno to PM_ERR_MEMORY?

...

In addition, common code between _alpm_pkghash_add() and _alpm_pkghash_add_sorted() is refactored out to _alpm_pkghash_add_pkg(). The 2 functions are now wrappers around _alpm_pkghash_add_pkg().

This belongs in a seperate patch, and I would prefer to not expose the internal function, mark it static, and remove the need for a prototype by pushing the implementors below the internal function.

Sure thing, I think it's neater that way too. Thanks!

...

Signed-off-by: Pang Yan Han --- Please give me advice on this as to whether it needs tweaking or if it won't be accepted. Thanks!

 lib/libalpm/alpm.h     |    1 +  lib/libalpm/be_local.c |    4 ++++  lib/libalpm/be_sync.c  |    8 +++++++-  lib/libalpm/db.c       |    1 +  lib/libalpm/error.c    |    2 ++  lib/libalpm/pkghash.c  |   46 +++++++++++++++-------------------------------  lib/libalpm/pkghash.h  |    1 +  7 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7fec293..57c68bd 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -501,6 +501,7 @@ enum _pmerrno_t {        PM_ERR_DB_NOT_FOUND,        PM_ERR_DB_WRITE,        PM_ERR_DB_REMOVE, +       PM_ERR_DB_PKGCACHE_NULL,        /* Servers */        PM_ERR_SERVER_BAD_URL,        PM_ERR_SERVER_NONE, diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index c5b2498..c7980f9 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -393,6 +393,10 @@ static int local_db_populate(pmdb_t *db)

       /* initialize hash at 50% full */        db->pkgcache = _alpm_pkghash_create(est_count * 2); +       if(db->pkgcache == NULL){ +               closedir(dbdir); +               RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1); +       }

       while((ent = readdir(dbdir)) != NULL) {                const char *name = ent->d_name; diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 4ad045c..853ad30 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -234,6 +234,9 @@ static int sync_db_populate(pmdb_t *db)

       /* initialize hash at 66% full */        db->pkgcache = _alpm_pkghash_create(est_count * 3 / 2); +       if(db->pkgcache == NULL) { +               RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1); +       }

       while(archive_read_next_header(archive, &entry) == ARCHIVE_OK) {                const struct stat *st; @@ -348,6 +351,9 @@ static int sync_db_read(pmdb_t *db, struct archive *archive,        if(likely_pkg && strcmp(likely_pkg->name, pkgname) == 0) {                pkg = likely_pkg;        } else { +               if(db->pkgcache == NULL) { +                       RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1); +               }                pkg = _alpm_pkghash_find(db->pkgcache, pkgname);        }        if(pkg == NULL) { @@ -459,10 +465,10 @@ pmdb_t *_alpm_db_register_sync(const char *treename)        _alpm_log(PM_LOG_DEBUG, "registering sync database '%s'\n", treename);

       db = _alpm_db_new(treename, 0); -       db->ops = &sync_db_ops;        if(db == NULL) {                RET_ERR(PM_ERR_DB_CREATE, NULL);        } +       db->ops = &sync_db_ops;

       handle->dbs_sync = alpm_list_add(handle->dbs_sync, db);        return(db); diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index 02f8282..d43a16b 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -370,6 +370,7 @@ pmdb_t *_alpm_db_new(const char *treename, int is_local)        CALLOC(db, 1, sizeof(pmdb_t), RET_ERR(PM_ERR_MEMORY, NULL));        STRDUP(db->treename, treename, RET_ERR(PM_ERR_MEMORY, NULL));        db->is_local = is_local; +       db->pkgcache_loaded = 0;

       return(db);  } diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c index 2cc6685..5b48c89 100644 --- a/lib/libalpm/error.c +++ b/lib/libalpm/error.c @@ -81,6 +81,8 @@ const char SYMEXPORT *alpm_strerror(int err)                        return _("could not update database");                case PM_ERR_DB_REMOVE:                        return _("could not remove database entry"); +               case PM_ERR_DB_PKGCACHE_NULL: +                       return _("could not initialize database package cache");                /* Servers */                case PM_ERR_SERVER_BAD_URL:                        return _("invalid url for server"); diff --git a/lib/libalpm/pkghash.c b/lib/libalpm/pkghash.c index 5480527..7006888 100644 --- a/lib/libalpm/pkghash.c +++ b/lib/libalpm/pkghash.c @@ -150,6 +150,16 @@ static pmpkghash_t *rehash(pmpkghash_t *oldhash)

 pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg)  { +       return(_alpm_pkghash_add_pkg(hash, pkg, 0)); +} + +pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg) +{ +       return(_alpm_pkghash_add_pkg(hash, pkg, 1)); +} + +pmpkghash_t *_alpm_pkghash_add_pkg(pmpkghash_t *hash, pmpkg_t *pkg, int sorted) +{        alpm_list_t *ptr;        size_t position;

@@ -173,40 +183,13 @@ pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg)        ptr->prev = ptr;

       hash->hash_table[position] = ptr; -       hash->list = alpm_list_join(hash->list, ptr); -       hash->entries += 1; - -       return(hash); -} - -pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg) -{ -       if(!hash) { -               return(_alpm_pkghash_add(hash, pkg)); -       } - -       alpm_list_t *ptr; -       size_t position; - -       if((hash->entries + 1) / MAX_HASH_LOAD > hash->buckets) { -               hash = rehash(hash); +       if(!sorted){ +               hash->list = alpm_list_join(hash->list, ptr); +       }else{ +               hash->list = alpm_list_mmerge(hash->list, ptr, _alpm_pkg_cmp);        }

-       position = get_hash_position(pkg->name_hash, hash); - -       ptr = calloc(1, sizeof(alpm_list_t)); -       if(ptr == NULL) { -               return(hash); -       } - -       ptr->data = pkg; -       ptr->next = NULL; -       ptr->prev = ptr; - -       hash->hash_table[position] = ptr; -       hash->list = alpm_list_mmerge(hash->list, ptr, _alpm_pkg_cmp);        hash->entries += 1; -        return(hash);  }

@@ -344,3 +327,4 @@ pmpkg_t *_alpm_pkghash_find(pmpkghash_t *hash, const char *name)

       return(NULL);  } +/* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/pkghash.h b/lib/libalpm/pkghash.h index a6c1db7..d63c6f0 100644 --- a/lib/libalpm/pkghash.h +++ b/lib/libalpm/pkghash.h @@ -47,6 +47,7 @@ pmpkghash_t *_alpm_pkghash_create(size_t size);

 pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg);  pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg); +pmpkghash_t *_alpm_pkghash_add_pkg(pmpkghash_t *hash, pmpkg_t *pkg, int sorted);  pmpkghash_t *_alpm_pkghash_remove(pmpkghash_t *hash, pmpkg_t *pkg, pmpkg_t **data);

 void _alpm_pkghash_free(pmpkghash_t *hash); -- 1.7.4