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

@@ -190,7 +190,10 @@ 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=? and "
+                                  "(expire_time > ? OR expire_time == 0.0)",
+                                  table_name.c_str()),
+                        db),
          std::make_pair(util::fmt("delete from %s where key_str=?", table_name.c_str()), db),
 
          std::make_pair(
@@ -348,6 +351,11 @@ OperationResult SQLite::DoGet(ResultCallback* cb, ValPtr key) {
         return res;
     }
 
+    if ( auto res = CheckError(sqlite3_bind_double(stmt.get(), 2, run_state::network_time));
+         res.code != ReturnCode::SUCCESS ) {
+        return res;
+    }
+
     return Step(stmt.get(), true);
 }
 

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=<uninitialized>, value=v]
+AFTER, [code=Storage::KEY_NOT_FOUND, error_str=<uninitialized>, value=<uninitialized>]

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;
+	}