pkg/46281: pkgin fails to clean its database properly

[filip%joyent.com@localhost]

>Number:         46281
>Category:       pkg
>Synopsis:       pkgin fails to clean its database properly
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pkg-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 29 12:55:00 +0000 2012
>Originator:     Filip Hajny
>Release:        n/a
>Organization:
Joyent
>Environment:
SunOS af9b008b-434d-4d53-af5c-bb9ee6ea4746 5.11 joyent_20120126T071347Z i86pc 
i386 i86pc
>Description:
In all pkgin 0.5.x versions (i.e. since 2011Q4), there is a serious bug that 
affects *any* day to day operations. This is only manifest on SunOS, or - more 
likely - on non-NetBSD platforms.

When the remote package repository is updated, pkgin needs to synchronize with 
its local SQLite database, using 'pkgin update' (or with 'pkgin -f update' 
regardless of any remote changes). As part of the database sync, the local 
REMOTE_* tables need to be cleaned before new/updated contents is inserted. The 
way this is currently implement in pkgin's summary.c code, is to take the 
struct that holds the database table list, and try to *iterate* through it by 
assuming the strings are laid out consequently:

for (ptbl = __UNCONST(sum.tbl_name), i = 0;
         i < nelms;
         ptbl += ((strlen(ptbl) + 1) * sizeof(char)), i++) {

        if (strstr(ptbl, "_PKG") != NULL)
                continue;

        snprintf(buf, BUFSIZ, DELETE_REMOTE, ptbl, ptbl, repo, ptbl);
        pkgindb_doquery(buf, NULL, NULL);
}

While this works apparently on NetBSD, there is no ground to believe this 
should be so on any platform. On SunOS, the 2nd and further iterations of loop 
end up pulling completely irrelevant strings from memory, which means the 
database cleanup operation is a failure. Even though no real error is echoed 
back to the user, the database is corrupted as stale entries pollute the table 
of package dependencies, requires and provides, and on any subsequent database 
update, this gets worse and worse. If the package count changes in the repo, 
you'll end up getting ridiculous dependencies reported on any install/upgrade 
request, as the indexing on the stale entries is off.

I've been in touch with imil, but it looks like he has had very little time 
available to work on pkgin in 2012, so this remains a problem.

>How-To-Repeat:
Build pkgin>=0.5 on any SunOS platform.
Point it to a package repository.
Run 'pkgin -f update' at least twice.
>Fix:
There is clear advantage to having to loop through a struct, and the list of 
tables is fixed and known anyway. This is a very crude patch, which only seems 
to help stop the cancerous growth of REMOTE_DEPS (the most annoying one), but 
not of the other tables. A better solution is needed.

--- summary.c.orig      2011-10-29 14:06:22.000000000 +0000
+++ summary.c
@@ -536,6 +536,19 @@ delete_remote_tbl(struct Summary sum, ch
                "DELETE FROM REMOTE_PKG WHERE REPOSITORY = '%s';", repo);
        pkgindb_doquery(buf, NULL, NULL);
 }
+static void
+delete_remote_tbl_each(char *ptbl, char *repo)
+{
+       char buf[BUFSIZ];
+
+       snprintf(buf, BUFSIZ, DELETE_REMOTE,
+              ptbl, ptbl, repo, ptbl);
+       pkgindb_doquery(buf, NULL, NULL);
+
+       if (strstr(ptbl, "_DEPS") != NULL)
+       {
+               snprintf(buf, BUFSIZ,
+                       "DELETE FROM REMOTE_PKG WHERE REPOSITORY = '%s';", 
repo);
+               pkgindb_doquery(buf, NULL, NULL);
+       }
+}
 
 static int
 pdb_clean_remote(void *param, int argc, char **argv, char **colname)
@@ -556,7 +569,10 @@ pdb_clean_remote(void *param, int argc, 
        /* did not find argv[0] (db repository) in pkg_repos */
        printf(MSG_CLEANING_DB_FROM_REPO, argv[0]);
 
-       delete_remote_tbl(sumsw[1], argv[0]);
+       delete_remote_tbl_each("REMOTE_DEPS", argv[0]);
+       delete_remote_tbl_each("REMOTE_CONFLICTS", argv[0]);
+       delete_remote_tbl_each("REMOTE_REQUIRES", argv[0]);
+       delete_remote_tbl_each("REMOTE_PROVIDES", argv[0]);
 
        snprintf(query, BUFSIZ,
                "DELETE FROM REPOS WHERE REPO_URL = \'%s\';", argv[0]);
@@ -668,7 +684,10 @@ update_db(int which, char **pkgkeep)
                                printf(MSG_PROCESSING_REMOTE_SUMMARY, *prepos);
 
                                /* delete remote* associated to this repository 
*/
-                               delete_remote_tbl(sumsw[i], *prepos);
+                               delete_remote_tbl_each("REMOTE_DEPS", *prepos);
+                               delete_remote_tbl_each("REMOTE_CONFLICTS", 
*prepos);
+                               delete_remote_tbl_each("REMOTE_REQUIRES", 
*prepos);
+                               delete_remote_tbl_each("REMOTE_PROVIDES", 
*prepos);
                                /* update remote* table for this repository */
                                insert_summary(sumsw[i], summary, *prepos);
                        }