From 56f5cafa985d13ca6fc23f0e896d4e04dc4e214b Mon Sep 17 00:00:00 2001 From: Benjamin Bannier Date: Sun, 29 Jun 2025 11:14:07 +0200 Subject: [PATCH] Prevent SQLite storage backend from serving expired entries The SQLite storage backend implements expiration by hand and garbage collection is done in `DoExpire`. This previously relied exclusively on gets not running within `Storage::expire_interval` of the put, otherwise we would potentially serve expired entries. With this patch we explictly check that entries are not expired before serving them so that the SQLite backend should never serve expired entries. --- src/storage/backend/sqlite/SQLite.cc | 25 +++++++--- .../output | 3 ++ .../storage/sqlite-expiration-implicit.zeek | 50 +++++++++++++++++++ 3 files changed, 70 insertions(+), 8 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output create mode 100644 testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek diff --git a/src/storage/backend/sqlite/SQLite.cc b/src/storage/backend/sqlite/SQLite.cc index 4d7447b77a..5e479f0e0c 100644 --- a/src/storage/backend/sqlite/SQLite.cc +++ b/src/storage/backend/sqlite/SQLite.cc @@ -17,6 +17,11 @@ using namespace std::chrono_literals; +namespace { +// Helper to check whether a database-stored `expire_time` is considered expired. +bool is_expired(double expire_time) { return expire_time != 0 && expire_time < zeek::run_state::network_time; } +} // namespace + namespace zeek::storage::backend::sqlite { OperationResult SQLite::RunPragma(std::string_view name, std::optional value) { @@ -190,7 +195,7 @@ OperationResult SQLite::DoOpen(OpenResultCallback* cb, RecordValPtr options) { "DO UPDATE SET value_str=?, expire_time=?", table_name.c_str()), db), - std::make_pair(util::fmt("select value_str from %s where key_str=?", table_name.c_str()), db), + std::make_pair(util::fmt("select value_str, expire_time from %s where key_str=?", table_name.c_str()), db), std::make_pair(util::fmt("delete from %s where key_str=?", table_name.c_str()), db), std::make_pair( @@ -497,15 +502,19 @@ OperationResult SQLite::Step(sqlite3_stmt* stmt, bool parse_value) { int step_status = sqlite3_step(stmt); if ( step_status == SQLITE_ROW ) { if ( parse_value ) { - auto blob = static_cast(sqlite3_column_blob(stmt, 0)); - size_t blob_size = sqlite3_column_bytes(stmt, 0); + if ( sqlite3_column_type(stmt, 1) != SQLITE_NULL && is_expired(sqlite3_column_double(stmt, 1)) ) + ret = {ReturnCode::KEY_NOT_FOUND, ""}; + else { + auto blob = static_cast(sqlite3_column_blob(stmt, 0)); + size_t blob_size = sqlite3_column_bytes(stmt, 0); - auto val = serializer->Unserialize({blob, blob_size}, val_type); + auto val = serializer->Unserialize({blob, blob_size}, val_type); - if ( val ) - ret = {ReturnCode::SUCCESS, "", val.value()}; - else - ret = {ReturnCode::OPERATION_FAILED, val.error()}; + if ( val ) + ret = {ReturnCode::SUCCESS, "", val.value()}; + else + ret = {ReturnCode::OPERATION_FAILED, val.error()}; + } } else { ret = {ReturnCode::OPERATION_FAILED, "sqlite3_step should not have returned a value"}; diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output new file mode 100644 index 0000000000..0730873d8c --- /dev/null +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-expiration-implicit/output @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +BEFORE, [code=Storage::SUCCESS, error_str=, value=v] +AFTER, [code=Storage::KEY_NOT_FOUND, error_str=, value=] diff --git a/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek b/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek new file mode 100644 index 0000000000..aa9e4d9a98 --- /dev/null +++ b/testing/btest/scripts/base/frameworks/storage/sqlite-expiration-implicit.zeek @@ -0,0 +1,50 @@ +# @TEST-EXEC: zeek %INPUT 2>&1 >output +# @TEST-EXEC: btest-diff output + +@load base/frameworks/storage/sync +@load policy/frameworks/storage/backend/sqlite + +# Manually control the clock. +redef allow_network_time_forward = F; + +redef Storage::expire_interval = 1secs; + +event zeek_init() + { + local opts = Storage::BackendOptions( + $serializer=Storage::STORAGE_SERIALIZER_JSON, + $sqlite=Storage::Backend::SQLite::Options( + $database_path="test.sqlite", + $table_name="testing")); + + local open_res = Storage::Sync::open_backend( + Storage::STORAGE_BACKEND_SQLITE, + opts, + string, + string); + local h = open_res$value; + + local key="k"; + local value="v"; + + # Expire entries well within `Storage::expire_interval` so `DoExpire` does not kick in. + local expire = Storage::expire_interval / 10; + local expire_time = network_time() + expire; + Storage::Sync::put( + h, + Storage::PutArgs( + $key=key, + $value=value, + $expire_time=expire)); + + # The entry we just put in exists. + local get = Storage::Sync::get(h, key); + print "BEFORE", get; + + # Advance time. + set_network_time(network_time() + expire * 2); + + # An expired value does not exist. + get = Storage::Sync::get(h, key); + print "AFTER", get; + }