From 6af436aad333c36aafb75a1e642d278ed425398c Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 5 Mar 2021 23:28:57 -0800 Subject: [PATCH] GH-1426: Improve handling of Broker data store creation failures 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. --- auxil/broker | 2 +- scripts/base/frameworks/broker/store.zeek | 8 +- src/broker/Manager.cc | 4 + src/broker/Store.h | 13 ++-- src/broker/store.bif | 16 ++-- .../broker.store.create-failure/zeek.err | 11 +++ .../broker.store.create-failure/zeek.out | 21 +++++ .../btest/broker/store/create-failure.zeek | 77 +++++++++++++++++++ 8 files changed, 138 insertions(+), 14 deletions(-) create mode 100644 testing/btest/Baseline/broker.store.create-failure/zeek.err create mode 100644 testing/btest/Baseline/broker.store.create-failure/zeek.out create mode 100644 testing/btest/broker/store/create-failure.zeek diff --git a/auxil/broker b/auxil/broker index b9f68a1dde..c8d9a8fb1c 160000 --- a/auxil/broker +++ b/auxil/broker @@ -1 +1 @@ -Subproject commit b9f68a1ddebe82db28068c76153b0954c1f7c227 +Subproject commit c8d9a8fb1ce768cfbcf81767a34a15ffd8104d0d 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); + }