From bf41a7e7f9e54fe85cf4a939c201d4fed83f83bf Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Tue, 22 Oct 2019 16:42:23 +0300 Subject: [PATCH 1/3] Fix a titanitic memory leak in the sqlite backend The rest of rpm expects to get a pointer to a temporary data area containing the header blob, which is then copied on header import. Seems my recollection of this was the exact opposite, so the sqlite backend was pushing back malloced copies of the blobs that nobody would then free. Oops, leaks, leaks, leaks, sunk. There's no need for sqlite to be different in this regard, but during Packages iteration the implementation used a temporary cursor for the actual header, causing the data to go away before reaching the upper layers which necessiated dup'ing the data, that nobody would free. Refactor to eliminate the temporary cursor from the picture to fix the mismatch of expectations and thus the leak. --- lib/backend/sqlite.c | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/backend/sqlite.c b/lib/backend/sqlite.c index 9de3e46318..b0d9d0c32e 100644 --- a/lib/backend/sqlite.c +++ b/lib/backend/sqlite.c @@ -28,7 +28,7 @@ static int sqlexec(sqlite3 *sdb, const char *fmt, ...); static int dbiCursorResult(dbiCursor dbc) { int rc = sqlite3_errcode(dbc->sdb); - int err = (rc != SQLITE_OK && rc != SQLITE_DONE); + int err = (rc != SQLITE_OK && rc != SQLITE_DONE && rc != SQLITE_ROW); if (err) { rpmlog(RPMLOG_ERR, "%s: %d: %s\n", sqlite3_sql(dbc->stmt), sqlite3_errcode(dbc->sdb), sqlite3_errmsg(dbc->sdb)); @@ -396,6 +396,19 @@ static rpmRC sqlite_pkgdbDel(dbiIndex dbi, dbiCursor dbc, unsigned int hdrNum) return dbiCursorResult(dbc); } +static rpmRC sqlite_stepPkg(dbiCursor dbc, unsigned char **hdrBlob, unsigned int *hdrLen) +{ + int rc = sqlite3_step(dbc->stmt); + + if (rc == SQLITE_ROW) { + if (hdrLen) + *hdrLen = sqlite3_column_bytes(dbc->stmt, 1); + if (hdrBlob) + *hdrBlob = (void *) sqlite3_column_blob(dbc->stmt, 1); + } + return rc; +} + static rpmRC sqlite_pkgdbByKey(dbiIndex dbi, dbiCursor dbc, unsigned int hdrNum, unsigned char **hdrBlob, unsigned int *hdrLen) { int rc = dbiCursorPrep(dbc, "SELECT hnum, blob from '%q' where hnum=?", @@ -404,13 +417,8 @@ static rpmRC sqlite_pkgdbByKey(dbiIndex dbi, dbiCursor dbc, unsigned int hdrNum, if (!rc) rc = dbiCursorBindPkg(dbc, hdrNum, NULL, 0); - if (!rc) { - while ((rc = sqlite3_step(dbc->stmt)) == SQLITE_ROW) { - *hdrLen = sqlite3_column_bytes(dbc->stmt, 1); - *hdrBlob = memcpy(xmalloc(*hdrLen), - sqlite3_column_blob(dbc->stmt, 1), *hdrLen); - } - } + if (!rc) + rc = sqlite_stepPkg(dbc, hdrBlob, hdrLen); return dbiCursorResult(dbc); } @@ -420,18 +428,13 @@ static rpmRC sqlite_pkgdbIter(dbiIndex dbi, dbiCursor dbc, { int rc = RPMRC_OK; if (dbc->stmt == NULL) { - rc = dbiCursorPrep(dbc, "SELECT hnum from '%q'", dbi->dbi_file); + rc = dbiCursorPrep(dbc, "SELECT hnum, blob from '%q'", dbi->dbi_file); } if (!rc) - rc = sqlite3_step(dbc->stmt); + rc = sqlite_stepPkg(dbc, hdrBlob, hdrLen); - if (rc == SQLITE_ROW) { - int hnum = sqlite3_column_int(dbc->stmt, 0); - dbiCursor kc = dbiCursorInit(dbi, 0); - rc = sqlite_pkgdbByKey(dbi, kc, hnum, hdrBlob, hdrLen); - dbiCursorFree(dbi, kc); - } else if (rc == SQLITE_DONE) { + if (rc == SQLITE_DONE) { rc = RPMRC_NOTFOUND; } else { rc = dbiCursorResult(dbc); From e720addd0b5293e140a3f01868315bfd4e791a85 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Wed, 23 Oct 2019 11:52:02 +0300 Subject: [PATCH 2/3] Cache prepared SQL statements for future use Preparing SQL statements from textual representation is relatively expensive and we do it a lot. Add a refcounted hash table for reusing the statements, which roughly halves the time of rpm -Va, other operations will naturally benefit too. db_cache should really be a private member someplace in the sqlite backend but there's no place to hang it, so without bigger changes that are out of scope here, the only option is adding a new member to the rpmdb struct itself. --- lib/backend/dbi.h | 1 + lib/backend/sqlite.c | 40 +++++++++++++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/lib/backend/dbi.h b/lib/backend/dbi.h index a618ea2c30..3fc9345c94 100644 --- a/lib/backend/dbi.h +++ b/lib/backend/dbi.h @@ -63,6 +63,7 @@ struct rpmdb_s { /* dbenv and related parameters */ void * db_dbenv; /*!< Backend private handle */ + void * db_cache; /*!< Backend private cache handle */ struct dbConfig_s cfg; int db_remove_env; diff --git a/lib/backend/sqlite.c b/lib/backend/sqlite.c index b0d9d0c32e..77c0307dcb 100644 --- a/lib/backend/sqlite.c +++ b/lib/backend/sqlite.c @@ -8,11 +8,21 @@ #include "debug.h" +#define HASHTYPE stmtHash +#define HTKEYTYPE const char * +#define HTDATATYPE sqlite3_stmt * +#include "lib/rpmhash.H" +#include "lib/rpmhash.C" +#undef HASHTYPE +#undef HTKEYTYPE +#undef HTDATATYPE + static const int sleep_ms = 50; struct dbiCursor_s { sqlite3 *sdb; sqlite3_stmt *stmt; + stmtHash stcache; const char *fmt; int flags; rpmTagVal tag; @@ -38,10 +48,26 @@ static int dbiCursorResult(dbiCursor dbc) static int dbiCursorReset(dbiCursor dbc) { - int rc = sqlite3_finalize(dbc->stmt); dbc->stmt = NULL; dbc->fmt = NULL; - return rc; + return 0; +} + +static sqlite3_stmt *cachedStmt(dbiCursor dbc, const char *cmd) +{ + sqlite3_stmt *stmt = NULL; + sqlite3_stmt **sts = NULL; + if (stmtHashGetEntry(dbc->stcache, cmd, &sts, NULL, NULL)) { + stmt = sts[0]; + sqlite3_reset(stmt); + sqlite3_clear_bindings(stmt); + } else { + if (sqlite3_prepare_v2(dbc->sdb, cmd, -1, &stmt, NULL) == SQLITE_OK) { + stmtHashAddEntry(dbc->stcache, xstrdup(cmd), stmt); + } + } + + return stmt; } static int dbiCursorPrep(dbiCursor dbc, const char *fmt, ...) @@ -64,7 +90,7 @@ static int dbiCursorPrep(dbiCursor dbc, const char *fmt, ...) } if (dbc->stmt == NULL) { - sqlite3_prepare_v2(dbc->sdb, cmd, -1, &dbc->stmt, NULL); + dbc->stmt = cachedStmt(dbc, cmd); } else { sqlite3_reset(dbc->stmt); } @@ -137,6 +163,10 @@ static int sqlite_init(rpmdb rdb, const char * dbhome) } sqlite3_busy_timeout(sdb, sleep_ms); + rdb->db_cache = stmtHashCreate(31, rstrhash, strcmp, + (stmtHashFreeKey)rfree, + (stmtHashFreeData)sqlite3_finalize); + if (rdb->db_flags & RPMDB_FLAG_REBUILD) { sqlexec(sdb, "PRAGMA journal_mode = OFF"); sqlexec(sdb, "PRAGMA locking_mode = EXCLUSIVE"); @@ -163,6 +193,7 @@ static int sqlite_fini(rpmdb rdb) if (sqlite3_db_readonly(sdb, NULL) == 0) { sqlexec(sdb, "PRAGMA optimize"); } + rdb->db_cache = stmtHashFree(rdb->db_cache); int xx = sqlite3_close(sdb); rc = (xx != SQLITE_OK); } @@ -323,6 +354,7 @@ static dbiCursor sqlite_CursorInit(dbiIndex dbi, unsigned int flags) dbc->sdb = dbi->dbi_db; dbc->flags = flags; dbc->tag = rpmTagGetValue(dbi->dbi_file); + dbc->stcache = dbi->dbi_rpmdb->db_cache; if (rpmTagGetClass(dbc->tag) == RPM_STRING_CLASS) { dbc->ctype = SQLITE_TEXT; } else { @@ -338,8 +370,6 @@ static dbiCursor sqlite_CursorFree(dbiIndex dbi, dbiCursor dbc) if (dbc) { if (dbc->flags & DBC_WRITE) sqlexec(dbc->sdb, "RELEASE '%s'", dbi->dbi_file); - if (dbc->stmt) - sqlite3_finalize(dbc->stmt); free(dbc); } return NULL; From 6fbe247ac6df713311cd83038f0800aedc89e086 Mon Sep 17 00:00:00 2001 From: Panu Matilainen Date: Wed, 23 Oct 2019 12:44:47 +0300 Subject: [PATCH 3/3] Handle setting db_descr centrally from the backend name Now that we can, set db_descr centrally on database open instead of relying on backends to do it (and forget, or leak memory, as has been the case). Also don't bother mallocing, the name of the backend is quite enough. With backends knowing their own names we could probably eliminate db_descr entirely but leaving that for another rainy day, it's possible there are code paths that assume it being set to something. --- lib/backend/db3.c | 4 ---- lib/backend/dbi.h | 2 +- lib/backend/lmdb.c | 4 ---- lib/backend/sqlite.c | 1 - lib/rpmdb.c | 3 +-- 5 files changed, 2 insertions(+), 12 deletions(-) diff --git a/lib/backend/db3.c b/lib/backend/db3.c index ab2f11f61e..ff0fe4305e 100644 --- a/lib/backend/db3.c +++ b/lib/backend/db3.c @@ -415,10 +415,6 @@ static int db_init(rpmdb rdb, const char * dbhome) if (rdb->db_dbenv != NULL) { rdb->db_opens++; return 0; - } else { - /* On first call, set backend description to something... */ - free(rdb->db_descr); - rasprintf(&rdb->db_descr, "db%u", DB_VERSION_MAJOR); } /* diff --git a/lib/backend/dbi.h b/lib/backend/dbi.h index 3fc9345c94..b2b9717c4e 100644 --- a/lib/backend/dbi.h +++ b/lib/backend/dbi.h @@ -49,7 +49,7 @@ struct rpmdb_s { int db_flags; int db_mode; /*!< open mode */ int db_perms; /*!< open permissions */ - char * db_descr; /*!< db backend description (for error msgs) */ + const char * db_descr; /*!< db backend description (for error msgs) */ struct dbChk_s * db_checked;/*!< headerCheck()'ed package instances */ rpmdb db_next; int db_opens; diff --git a/lib/backend/lmdb.c b/lib/backend/lmdb.c index 801f50e54b..badd317c95 100644 --- a/lib/backend/lmdb.c +++ b/lib/backend/lmdb.c @@ -137,10 +137,6 @@ static int db_init(rpmdb rdb, const char * dbhome) if (rdb->db_dbenv != NULL) { rdb->db_opens++; return 0; - } else { - /* On first call, set backend description to something... */ - free(rdb->db_descr); - rdb->db_descr = xstrdup("lmdb"); } MDB_dbi maxdbs = 32; diff --git a/lib/backend/sqlite.c b/lib/backend/sqlite.c index 77c0307dcb..180faab817 100644 --- a/lib/backend/sqlite.c +++ b/lib/backend/sqlite.c @@ -173,7 +173,6 @@ static int sqlite_init(rpmdb rdb, const char * dbhome) } rdb->db_dbenv = sdb; - rdb->db_descr = xstrdup("sqlite"); } rdb->db_opens++; diff --git a/lib/rpmdb.c b/lib/rpmdb.c index b97274e7b3..9cd50e7d93 100644 --- a/lib/rpmdb.c +++ b/lib/rpmdb.c @@ -408,7 +408,6 @@ int rpmdbClose(rpmdb db) db->db_fullpath = _free(db->db_fullpath); db->db_checked = dbChkFree(db->db_checked); db->db_indexes = _free(db->db_indexes); - db->db_descr = _free(db->db_descr); if (next) { *prev = next->db_next; @@ -473,7 +472,6 @@ static rpmdb newRpmdb(const char * root, const char * home, db->db_tags = dbiTags; db->db_ndbi = sizeof(dbiTags) / sizeof(rpmDbiTag); db->db_indexes = xcalloc(db->db_ndbi, sizeof(*db->db_indexes)); - db->db_descr = xstrdup("unknown db"); db->nrefs = 0; return rpmdbLink(db); } @@ -508,6 +506,7 @@ static int openDatabase(const char * prefix, /* Just the primary Packages database opened here */ rc = pkgdbOpen(db, db->db_flags, NULL); + db->db_descr = (rc == 0) ? db->db_ops->name : "unknown db"; } if (rc || justCheck || dbp == NULL)