From 09497f173550b486dc31cf2734566773291f9eb9 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sun, 11 Feb 2024 16:55:23 -0600 Subject: [PATCH 1/3] Improve error handling around the sqlite3 library Avoid potentially using the DB object after it has been closed. --- src/scitokens_cache.cpp | 65 ++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index 944f990..07b949e 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -95,38 +95,53 @@ std::string get_cache_file() { return keycache_file; } -void remove_issuer_entry(sqlite3 *db, const std::string &issuer, +// Remove a given issuer from the database. Starts a new transaction +// if `new_transaction` is true. +// If a failure occurs, then this function returns nonzero and closes +// the database handle. +int remove_issuer_entry(sqlite3 *db, const std::string &issuer, bool new_transaction) { - if (new_transaction) - sqlite3_exec(db, "BEGIN", 0, 0, 0); + int rc; + if (new_transaction) { + if ((rc = sqlite3_exec(db, "BEGIN", 0, 0, 0)) != SQLITE_OK) { + sqlite3_close(db); + return -1; + } + } sqlite3_stmt *stmt; - int rc = sqlite3_prepare_v2(db, "DELETE FROM keycache WHERE issuer = ?", -1, + rc = sqlite3_prepare_v2(db, "DELETE FROM keycache WHERE issuer = ?", -1, &stmt, NULL); if (rc != SQLITE_OK) { sqlite3_close(db); - return; + return -1; } if (sqlite3_bind_text(stmt, 1, issuer.c_str(), issuer.size(), SQLITE_STATIC) != SQLITE_OK) { sqlite3_finalize(stmt); sqlite3_close(db); - return; + return -1; } rc = sqlite3_step(stmt); if (rc != SQLITE_DONE) { sqlite3_finalize(stmt); sqlite3_close(db); - return; + return -1; } sqlite3_finalize(stmt); - if (new_transaction) - sqlite3_exec(db, "COMMIT", 0, 0, 0); + if (new_transaction) { + if ((rc = sqlite3_exec(db, "COMMIT", 0, 0, 0)) != SQLITE_OK) { + sqlite3_close(db); + return -1; + } + } + + return 0; } } // namespace @@ -170,27 +185,35 @@ bool scitokens::Validator::get_public_keys_from_db(const std::string issuer, picojson::value json_obj; auto err = picojson::parse(json_obj, metadata); if (!err.empty() || !json_obj.is()) { - remove_issuer_entry(db, issuer, true); + if (remove_issuer_entry(db, issuer, true) != 0) { + return false; + } sqlite3_close(db); return false; } auto top_obj = json_obj.get(); auto iter = top_obj.find("jwks"); if (iter == top_obj.end() || !iter->second.is()) { - remove_issuer_entry(db, issuer, true); + if (remove_issuer_entry(db, issuer, true) != 0) { + return false; + } sqlite3_close(db); return false; } auto keys_local = iter->second; iter = top_obj.find("expires"); if (iter == top_obj.end() || !iter->second.is()) { - remove_issuer_entry(db, issuer, true); + if (remove_issuer_entry(db, issuer, true) != 0) { + return false; + } sqlite3_close(db); return false; } auto expiry = iter->second.get(); if (now > expiry) { - remove_issuer_entry(db, issuer, true); + if (remove_issuer_entry(db, issuer, true) != 0) { + return false; + } sqlite3_close(db); return false; } @@ -238,9 +261,14 @@ bool scitokens::Validator::store_public_keys(const std::string &issuer, return false; } - sqlite3_exec(db, "BEGIN", 0, 0, 0); + if ((rc = sqlite3_exec(db, "BEGIN", 0, 0, 0)) != SQLITE_OK) { + sqlite3_close(db); + return false; + } - remove_issuer_entry(db, issuer, false); + if (remove_issuer_entry(db, issuer, false) != 0) { + return false; + } sqlite3_stmt *stmt; rc = sqlite3_prepare_v2(db, "INSERT INTO keycache VALUES (?, ?)", -1, &stmt, @@ -270,10 +298,13 @@ bool scitokens::Validator::store_public_keys(const std::string &issuer, sqlite3_close(db); return false; } + sqlite3_finalize(stmt); - sqlite3_exec(db, "COMMIT", 0, 0, 0); + if (sqlite3_exec(db, "COMMIT", 0, 0, 0) != SQLITE_OK) { + sqlite3_close(db); + return false; + } - sqlite3_finalize(stmt); sqlite3_close(db); return true; } From ac93e2d4c6dc923863850884c48a2f81a4f07c7b Mon Sep 17 00:00:00 2001 From: Brian P Bockelman Date: Sun, 11 Feb 2024 20:54:40 -0600 Subject: [PATCH 2/3] Small whitespace fix by linter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens_cache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index 07b949e..a7e4968 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -100,7 +100,7 @@ std::string get_cache_file() { // If a failure occurs, then this function returns nonzero and closes // the database handle. int remove_issuer_entry(sqlite3 *db, const std::string &issuer, - bool new_transaction) { + bool new_transaction) { int rc; if (new_transaction) { From cf5b5ccf1ae23992a39d26643f8b6e4b9ef7b4a0 Mon Sep 17 00:00:00 2001 From: Brian P Bockelman Date: Sun, 11 Feb 2024 20:55:37 -0600 Subject: [PATCH 3/3] Small whitespace fix by linter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/scitokens_cache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scitokens_cache.cpp b/src/scitokens_cache.cpp index a7e4968..12536c1 100644 --- a/src/scitokens_cache.cpp +++ b/src/scitokens_cache.cpp @@ -112,7 +112,7 @@ int remove_issuer_entry(sqlite3 *db, const std::string &issuer, sqlite3_stmt *stmt; rc = sqlite3_prepare_v2(db, "DELETE FROM keycache WHERE issuer = ?", -1, - &stmt, NULL); + &stmt, NULL); if (rc != SQLITE_OK) { sqlite3_close(db); return -1;