diff --git a/CHANGES b/CHANGES index a1fa31387c..b1d7694b7f 100644 --- a/CHANGES +++ b/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) diff --git a/VERSION b/VERSION index 2a1e8990f3..1790536e02 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.1.0-dev.286 +4.1.0-dev.288 diff --git a/auxil/broker b/auxil/broker index b9f68a1dde..c6779fb4dc 160000 --- a/auxil/broker +++ b/auxil/broker @@ -1 +1 @@ -Subproject commit b9f68a1ddebe82db28068c76153b0954c1f7c227 +Subproject commit c6779fb4dc5a636eb75e14f6306548bbc0386767 diff --git a/scripts/base/frameworks/broker/store.zeek b/scripts/base/frameworks/broker/store.zeek index 83aff4b6bb..8a685da396 100644 --- a/scripts/base/frameworks/broker/store.zeek +++ b/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, diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index 98abfbe56f..b33ec50bb9 100644 --- a/src/broker/Manager.cc +++ b/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; diff --git a/src/broker/Store.h b/src/broker/Store.h index fb9a516fac..8e73900a47 100644 --- a/src/broker/Store.h +++ b/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 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 diff --git a/src/broker/store.bif b/src/broker/store.bif index fa0db96f3e..87f614f373 100644 --- a/src/broker/store.bif +++ b/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(h); } + { + auto rval = dynamic_cast(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(); } 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(); } 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(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 diff --git a/testing/btest/Baseline/broker.store.create-failure/zeek.err b/testing/btest/Baseline/broker.store.create-failure/zeek.err new file mode 100644 index 0000000000..5ae6b0affd --- /dev/null +++ b/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 diff --git a/testing/btest/Baseline/broker.store.create-failure/zeek.out b/testing/btest/Baseline/broker.store.create-failure/zeek.out new file mode 100644 index 0000000000..dcf0911820 --- /dev/null +++ b/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=]] +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=]] +c1 keys result: [status=Broker::FAILURE, result=[data=]] +m2 keys result: [status=Broker::FAILURE, result=[data=]] +c2 keys result: [status=Broker::FAILURE, result=[data=]] +c1 timeout diff --git a/testing/btest/broker/store/create-failure.zeek b/testing/btest/broker/store/create-failure.zeek new file mode 100644 index 0000000000..57f56b815c --- /dev/null +++ b/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); + }