Merge remote-tracking branch 'origin/topic/jsiwek/improve-broker-store-creation'

* origin/topic/jsiwek/improve-broker-store-creation:
  GH-1426: Improve handling of Broker data store creation failures

CHANGES

@@ -1,4 +1,15 @@
 
+4.1.0-dev.288 | 2021-03-08 12:29:14 -0800
+
+  * GH-1426: Improve handling of Broker data store creation failures (Jon Siwek, Corelight)
+
+    Broker::create_master() and Broker::create_clone() now return
+    a valid value even when there's a failure to open the backend database
+    (e.g. SQLite filesystem error).  In that case, the returned value can
+    still be passed into other data store operations, but they'll fail
+    immediately with an error.  Broker::is_closed() can now also be used to
+    determine whether the data store creation calls failed.
+
 4.1.0-dev.286 | 2021-03-08 11:24:38 +0000
 
   * Allow non-TCP based protocols to use SSL analyzer. (Keith Jones)

VERSION

@@ -1 +1 @@
-4.1.0-dev.286
+4.1.0-dev.288

auxil/broker

@@ -1 +1 @@
-Subproject commit b9f68a1ddebe82db28068c76153b0954c1f7c227
+Subproject commit c6779fb4dc5a636eb75e14f6306548bbc0386767

scripts/base/frameworks/broker/store.zeek

@@ -78,7 +78,9 @@ export {
 	##
 	## options: tunes how some storage backends operate.
 	##
-	## Returns: a handle to the data store.
+	## Returns: a handle to the data store for which a subsequent call to
+	##          :zeek:see:`Broker::is_closed` will return true if the store
+	##          could not be created/opened.
 	global create_master: function(name: string, b: BackendType &default = MEMORY,
 	                               options: BackendOptions &default = BackendOptions()): opaque of Broker::Store;
 
@@ -111,7 +113,9 @@ export {
 	##                           acknowledged by the master.  A negative/zero
 	##                           value indicates that commands never buffer.
 	##
-	## Returns: a handle to the data store.
+	## Returns: a handle to the data store for which a subsequent call to
+	##          :zeek:see:`Broker::is_closed` will return true if the store
+	##          could not be created/opened.
 	global create_clone: function(name: string,
 	                              resync_interval: interval &default = default_clone_resync_interval,
 	                              stale_interval: interval &default = default_clone_stale_interval,

src/broker/Manager.cc

@@ -1746,6 +1746,10 @@ bool Manager::CloseStore(const string& name)
 		++i;
 		}
 
+	s->second->have_store = false;
+	s->second->store_pid = {};
+	s->second->proxy = {};
+	s->second->store = {};
 	Unref(s->second);
 	data_stores.erase(s);
 	return true;

src/broker/Store.h

@@ -114,8 +114,12 @@ private:
  */
 class StoreHandleVal : public OpaqueVal {
 public:
+	StoreHandleVal()
+		: OpaqueVal(Broker::detail::opaque_of_store_handle)
+		{}
+
 	StoreHandleVal(broker::store s)
-		: OpaqueVal(Broker::detail::opaque_of_store_handle), store{s}, proxy{store}, store_pid{store.frontend_id()}
+		: OpaqueVal(Broker::detail::opaque_of_store_handle), store{std::move(s)}, proxy{store}, store_pid{store.frontend_id()}, forward_to{}, have_store{true}
 		{ }
 
 	void ValDescribe(ODesc* d) const override;
@@ -125,16 +129,13 @@ public:
 	broker::publisher_id store_pid;
 	// Zeek table that events are forwarded to.
 	TableValPtr forward_to;
+	bool have_store = false;
 
 protected:
 
 	IntrusivePtr<Val> DoClone(CloneState* state) override
 		{ return { NewRef{}, this }; }
 
-	StoreHandleVal()
-		: OpaqueVal(Broker::detail::opaque_of_store_handle)
-		{}
-
 	DECLARE_OPAQUE_VALUE(StoreHandleVal)
 };
 
@@ -145,4 +146,4 @@ broker::backend to_backend_type(BifEnum::Broker::BackendType type);
 broker::backend_options to_backend_options(broker::backend backend,
                                            RecordVal* options);
 
-} // namespace zeek::Broker
+} // namespace zeek::Broker::detail

src/broker/store.bif

@@ -8,7 +8,10 @@
 #include "zeek/Trigger.h"
 
 static zeek::Broker::detail::StoreHandleVal* to_store_handle(zeek::Val* h)
-	{ return dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h); }
+	{
+	auto rval = dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h);
+	return rval && rval->have_store ? rval : nullptr;
+	}
 %%}
 
 module Broker;
@@ -41,7 +44,7 @@ function Broker::__create_master%(id: string, b: BackendType,
 	if ( ! store )
 		{
 		zeek::emit_builtin_error(zeek::util::fmt("Could not create Broker master store '%s'", name));
-		return nullptr;
+		return make_intrusive<zeek::Broker::detail::StoreHandleVal>();
 		}
 
 	return store;
@@ -66,7 +69,7 @@ function Broker::__create_clone%(id: string, resync_interval: interval,
 	if ( ! store )
 		{
 		zeek::emit_builtin_error(zeek::util::fmt("Could not create clone of Broker store '%s'", name));
-		return nullptr;
+		return make_intrusive<zeek::Broker::detail::StoreHandleVal>();
 		}
 
 	return store;
@@ -75,12 +78,15 @@ function Broker::__create_clone%(id: string, resync_interval: interval,
 function Broker::__is_closed%(h: opaque of Broker::Store%): bool
 	%{
 	zeek::Broker::Manager::ScriptScopeGuard ssg;
-	auto handle = to_store_handle(h);
+	auto handle = dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h);
 
 	if ( ! handle )
 		zeek::detail::emit_builtin_exception("invalid Broker store handle", h);
 
-	return zeek::val_mgr->Bool(broker_mgr->LookupStore(handle->store.name()));
+	if ( ! handle->have_store )
+		return zeek::val_mgr->True();
+
+	return zeek::val_mgr->Bool(! broker_mgr->LookupStore(handle->store.name()));
 	%}
 
 function Broker::__close%(h: opaque of Broker::Store%): bool

testing/btest/Baseline/broker.store.create-failure/zeek.err

@@ -0,0 +1,11 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/create-failure.zeek, line 63: Failed to attach master store backend_failure: (Broker::create_master(../fail, Broker::SQLITE, (coerce [] to Broker::BackendOptions)))
+error in <...>/create-failure.zeek, line 63: Could not create Broker master store '../fail' (Broker::create_master(../fail, Broker::SQLITE, (coerce [] to Broker::BackendOptions)))
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 27: invalid Broker store handle (Broker::close(m1) and broker::store::{})
+error in <...>/create-failure.zeek, line 33: invalid Broker store handle (Broker::close(c2) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+received termination signal

testing/btest/Baseline/broker.store.create-failure/zeek.out

@@ -0,0 +1,21 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+T
+F
+F
+F
+m1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+m2 keys result: [status=Broker::SUCCESS, result=[data=broker::data{{}}]]
+c2 keys result: [status=Broker::SUCCESS, result=[data=broker::data{{}}]]
+T
+F
+F
+F
+T
+T
+T
+T
+m1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+m2 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c2 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c1 timeout

testing/btest/broker/store/create-failure.zeek

@@ -0,0 +1,77 @@
+# @TEST-EXEC: mkdir fail.sqlite
+# @TEST-EXEC: btest-bg-run zeek "BROKER_FILE_VERBOSITY=error BROKER_CONSOLE_VERBOSITY=quiet zeek -b %INPUT >out 2>err"
+# @TEST-EXEC: btest-bg-wait 60
+# @TEST-EXEC: btest-diff zeek/out
+# @TEST-EXEC: TEST_DIFF_CANONIFIER="$SCRIPTS/diff-remove-abspath $SCRIPTS/diff-sort" btest-diff zeek/err
+
+redef exit_only_after_terminate = T;
+
+global c = 0;
+global m1: opaque of Broker::Store;
+global m2: opaque of Broker::Store;
+global c1: opaque of Broker::Store;
+global c2: opaque of Broker::Store;
+
+global check_it: function(name: string, s: opaque of Broker::Store);
+
+function check_terminate_conditions()
+	{
+	++c;
+	if ( c == 4 )
+		{
+		print Broker::is_closed(m1);
+		print Broker::is_closed(m2);
+		print Broker::is_closed(c1);
+		print Broker::is_closed(c2);
+		# Failed to originally open m1, so should raise an error.
+		Broker::close(m1);
+		Broker::close(m2);
+		Broker::close(c1);
+		# Closing c2 is an error since it's actually referring to m2, already
+		# closed above (i.e. making a clone of a master store that already lives
+		# in the same process, just returns a handle to the master).
+		Broker::close(c2);
+		print Broker::is_closed(m1);
+		print Broker::is_closed(m2);
+		print Broker::is_closed(c1);
+		print Broker::is_closed(c2);
+		check_it("m1", m1);
+		check_it("c1", c1);
+		check_it("m2", m2);
+		check_it("c2", c2);
+		}
+	else if ( c == 8 )
+		terminate();
+	}
+
+function check_it(name: string, s: opaque of Broker::Store)
+	{
+	when ( local r = Broker::keys(s) )
+		{
+		check_terminate_conditions();
+		print fmt("%s keys result: %s", name, r);
+		}
+	timeout 1sec
+		{
+		check_terminate_conditions();
+		print fmt("%s timeout", name);
+		}
+	}
+
+event zeek_init()
+	{
+	m1 = Broker::create_master("../fail", Broker::SQLITE);
+	m2 = Broker::create_master("ok");
+	c1 = Broker::create_clone("../fail");
+	c2 = Broker::create_clone("ok");
+
+	print Broker::is_closed(m1);
+	print Broker::is_closed(m2);
+	print Broker::is_closed(c1);
+	print Broker::is_closed(c2);
+
+	check_it("m1", m1);
+	check_it("c1", c1);
+	check_it("m2", m2);
+	check_it("c2", c2);
+	}