diff --git a/CHANGES b/CHANGES index 444929c41d..051b822154 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,24 @@ + +3.2.0-dev.899 | 2020-07-14 00:02:05 +0000 + + * Improve Broker store API's handling of invalid arguments + + * Some methods mistakenly returned a bool instead of QueryResult + when passed an invalid `opaque of Broker::Store` handle. + + * Now generates a runtime exception for store_name() and is_closed() + calls that pass an invalid `opaque of Broker::Store` handle as any + returned value can't be reasonably used in any subsequent logic. + + * Descriptions of any invalid arguments are now given in the error + message. (Jon Siwek, Corelight) + + * Add zeek::detail::emit_builtin_exception() functions + + These work like zeek::emit_builtin_error(), but also throw an InterpreterException (Jon Siwek, Corelight) + + * GH-1024: fix crash on passing wrong types to Broker store API (Jon Siwek, Corelight) + 3.2.0-dev.894 | 2020-07-13 12:12:17 -0700 * GH-1019: deprecate icmp_conn params for ICMP events diff --git a/VERSION b/VERSION index 00f989a6c4..3f1e5d2b23 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.2.0-dev.894 +3.2.0-dev.899 diff --git a/src/Func.cc b/src/Func.cc index 97272efd1b..99cd652891 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -788,31 +788,51 @@ function_ingredients::function_ingredients(zeek::detail::ScopePtr scope, zeek::d this->body = std::move(body); } -} // namespace detail - -void emit_builtin_error(const char* msg) - { - emit_builtin_error(msg, zeek::ValPtr{}); - } - -void emit_builtin_error(const char* msg, zeek::ValPtr arg) - { - emit_builtin_error(msg, arg.get()); - } - -void emit_builtin_error(const char* msg, Obj* arg) +static void emit_builtin_error_common(const char* msg, Obj* arg, bool unwind) { auto emit = [=](const zeek::detail::CallExpr* ce) { if ( ce ) - ce->Error(msg, arg); + { + if ( unwind ) + { + if ( arg ) + { + ODesc d; + arg->Describe(&d); + reporter->ExprRuntimeError(ce, "%s (%s), during call:", msg, + d.Description()); + } + else + reporter->ExprRuntimeError(ce, "%s", msg); + } + else + ce->Error(msg, arg); + } else - reporter->Error(msg, arg); + { + if ( arg ) + { + if ( unwind ) + reporter->RuntimeError(arg->GetLocationInfo(), "%s", msg); + else + arg->Error(msg); + } + else + { + if ( unwind ) + reporter->RuntimeError(nullptr, "%s", msg); + else + reporter->Error("%s", msg); + } + } }; if ( zeek::detail::call_stack.empty() ) { + // Shouldn't happen unless someone (mistakenly) calls builtin_error() + // from somewhere that's not even evaluating script-code. emit(nullptr); return; } @@ -866,6 +886,39 @@ void emit_builtin_error(const char* msg, Obj* arg) emit(last_call.call); } +void emit_builtin_exception(const char* msg) + { + emit_builtin_error_common(msg, nullptr, true); + } + +void emit_builtin_exception(const char* msg, const zeek::ValPtr& arg) + { + emit_builtin_error_common(msg, arg.get(), true); + } + +void emit_builtin_exception(const char* msg, Obj* arg) + { + emit_builtin_error_common(msg, arg, true); + } + +} // namespace detail + + +void emit_builtin_error(const char* msg) + { + zeek::detail::emit_builtin_error_common(msg, nullptr, false); + } + +void emit_builtin_error(const char* msg, const zeek::ValPtr& arg) + { + zeek::detail::emit_builtin_error_common(msg, arg.get(), false); + } + +void emit_builtin_error(const char* msg, Obj* arg) + { + zeek::detail::emit_builtin_error_common(msg, arg, false); + } + } // namespace zeek void builtin_error(const char* msg) @@ -873,7 +926,7 @@ void builtin_error(const char* msg) zeek::emit_builtin_error(msg); } -void builtin_error(const char* msg, zeek::ValPtr arg) +void builtin_error(const char* msg, const zeek::ValPtr& arg) { zeek::emit_builtin_error(msg, arg); } diff --git a/src/Func.h b/src/Func.h index 2f3d703690..50d39607b5 100644 --- a/src/Func.h +++ b/src/Func.h @@ -275,13 +275,17 @@ extern std::vector call_stack; // This is set to true after the built-in functions have been initialized. extern bool did_builtin_init; +extern void emit_builtin_exception(const char* msg); +extern void emit_builtin_exception(const char* msg, const zeek::ValPtr& arg); +extern void emit_builtin_exception(const char* msg, Obj* arg); + } // namespace detail extern std::string render_call_stack(); // These methods are used by BIFs, so they're in the public namespace. extern void emit_builtin_error(const char* msg); -extern void emit_builtin_error(const char* msg, zeek::ValPtr); +extern void emit_builtin_error(const char* msg, const zeek::ValPtr&); extern void emit_builtin_error(const char* msg, Obj* arg); } // namespace zeek diff --git a/src/broker/store.bif b/src/broker/store.bif index 8b4b955724..8bca7e1144 100644 --- a/src/broker/store.bif +++ b/src/broker/store.bif @@ -20,6 +20,9 @@ static broker::optional prepare_expiry(double e) return ts; } + +static bro_broker::StoreHandleVal* to_store_handle(zeek::Val* h) + { return dynamic_cast(h); } %%} module Broker; @@ -87,58 +90,54 @@ function Broker::__create_clone%(id: string, resync_interval: interval, function Broker::__is_closed%(h: opaque of Broker::Store%): bool %{ bro_broker::Manager::ScriptScopeGuard ssg; + auto handle = to_store_handle(h); - if ( ! h ) - { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); - } + if ( ! handle ) + zeek::detail::emit_builtin_exception("invalid Broker store handle", h); - auto handle = static_cast(h); return zeek::val_mgr->Bool(broker_mgr->LookupStore(handle->store.name())); %} function Broker::__close%(h: opaque of Broker::Store%): bool %{ bro_broker::Manager::ScriptScopeGuard ssg; + auto handle = to_store_handle(h); - if ( ! h ) + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); + zeek::emit_builtin_error("invalid Broker store handle", h); + return val_mgr->False(); } - auto handle = static_cast(h); return zeek::val_mgr->Bool(broker_mgr->CloseStore(handle->store.name())); %} function Broker::__store_name%(h: opaque of Broker::Store%): string %{ - if ( ! h ) - { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->EmptyString(); - } + auto handle = to_store_handle(h); - auto handle = static_cast(h); - return zeek::make_intrusive(handle->store.name()); + if ( ! handle ) + zeek::detail::emit_builtin_exception("invalid Broker store handle", h); + + return make_intrusive(handle->store.name()); %} function Broker::__exists%(h: opaque of Broker::Store, k: any%): Broker::QueryResult %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); + zeek::emit_builtin_error("invalid Broker store handle", h); + return bro_broker::query_result(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return bro_broker::query_result(); } @@ -172,18 +171,19 @@ function Broker::__exists%(h: opaque of Broker::Store, function Broker::__get%(h: opaque of Broker::Store, k: any%): Broker::QueryResult %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); + zeek::emit_builtin_error("invalid Broker store handle", h); + return bro_broker::query_result(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return bro_broker::query_result(); } @@ -217,25 +217,26 @@ function Broker::__get%(h: opaque of Broker::Store, function Broker::__put_unique%(h: opaque of Broker::Store, k: any, v: any, e: interval%): Broker::QueryResult %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); + zeek::emit_builtin_error("invalid Broker store handle", h); + return bro_broker::query_result(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto val = bro_broker::val_to_data(v); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return bro_broker::query_result(); } if ( ! val ) { - zeek::emit_builtin_error("invalid Broker data conversion for value argument"); + zeek::emit_builtin_error("invalid Broker data conversion for value argument", v); return bro_broker::query_result(); } @@ -271,18 +272,19 @@ function Broker::__put_unique%(h: opaque of Broker::Store, function Broker::__get_index_from_value%(h: opaque of Broker::Store, k: any, i: any%): Broker::QueryResult %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); + zeek::emit_builtin_error("invalid Broker store handle", h); + return bro_broker::query_result(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return bro_broker::query_result(); } @@ -290,7 +292,7 @@ function Broker::__get_index_from_value%(h: opaque of Broker::Store, if ( ! index ) { - zeek::emit_builtin_error("invalid Broker data conversion for index argument"); + zeek::emit_builtin_error("invalid Broker data conversion for index argument", i); return bro_broker::query_result(); } @@ -324,13 +326,13 @@ function Broker::__get_index_from_value%(h: opaque of Broker::Store, function Broker::__keys%(h: opaque of Broker::Store%): Broker::QueryResult %{ - if ( ! h ) - { - zeek::emit_builtin_error("invalid Broker store handle"); - return zeek::val_mgr->False(); - } + auto handle = to_store_handle(h); - auto handle = static_cast(h); + if ( ! handle ) + { + zeek::emit_builtin_error("invalid Broker store handle", h); + return bro_broker::query_result(); + } auto trigger = frame->GetTrigger(); @@ -362,25 +364,26 @@ function Broker::__keys%(h: opaque of Broker::Store%): Broker::QueryResult function Broker::__put%(h: opaque of Broker::Store, k: any, v: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto val = bro_broker::val_to_data(v); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! val ) { - zeek::emit_builtin_error("invalid Broker data conversion for value argument"); + zeek::emit_builtin_error("invalid Broker data conversion for value argument", v); return zeek::val_mgr->False(); } @@ -390,18 +393,19 @@ function Broker::__put%(h: opaque of Broker::Store, function Broker::__erase%(h: opaque of Broker::Store, k: any%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } @@ -412,25 +416,26 @@ function Broker::__erase%(h: opaque of Broker::Store, k: any%): bool function Broker::__increment%(h: opaque of Broker::Store, k: any, a: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto amount = bro_broker::val_to_data(a); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! amount ) { - zeek::emit_builtin_error("invalid Broker data conversion for amount argument"); + zeek::emit_builtin_error("invalid Broker data conversion for amount argument", a); return zeek::val_mgr->False(); } @@ -442,25 +447,26 @@ function Broker::__increment%(h: opaque of Broker::Store, k: any, a: any, function Broker::__decrement%(h: opaque of Broker::Store, k: any, a: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto amount = bro_broker::val_to_data(a); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! amount ) { - zeek::emit_builtin_error("invalid Broker data conversion for amount argument"); + zeek::emit_builtin_error("invalid Broker data conversion for amount argument", a); return zeek::val_mgr->False(); } @@ -471,25 +477,26 @@ function Broker::__decrement%(h: opaque of Broker::Store, k: any, a: any, function Broker::__append%(h: opaque of Broker::Store, k: any, s: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto str = bro_broker::val_to_data(s); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! str ) { - zeek::emit_builtin_error("invalid Broker data conversion for str argument"); + zeek::emit_builtin_error("invalid Broker data conversion for str argument", s); return zeek::val_mgr->False(); } @@ -500,25 +507,26 @@ function Broker::__append%(h: opaque of Broker::Store, k: any, s: any, function Broker::__insert_into_set%(h: opaque of Broker::Store, k: any, i: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto idx = bro_broker::val_to_data(i); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! idx ) { - zeek::emit_builtin_error("invalid Broker data conversion for index argument"); + zeek::emit_builtin_error("invalid Broker data conversion for index argument", i); return zeek::val_mgr->False(); } @@ -530,32 +538,33 @@ function Broker::__insert_into_set%(h: opaque of Broker::Store, k: any, i: any, function Broker::__insert_into_table%(h: opaque of Broker::Store, k: any, i: any, v: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto idx = bro_broker::val_to_data(i); auto val = bro_broker::val_to_data(v); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! idx ) { - zeek::emit_builtin_error("invalid Broker data conversion for index argument"); + zeek::emit_builtin_error("invalid Broker data conversion for index argument", i); return zeek::val_mgr->False(); } if ( ! val ) { - zeek::emit_builtin_error("invalid Broker data conversion for value argument"); + zeek::emit_builtin_error("invalid Broker data conversion for value argument", v); return zeek::val_mgr->False(); } @@ -567,25 +576,26 @@ function Broker::__insert_into_table%(h: opaque of Broker::Store, k: any, function Broker::__remove_from%(h: opaque of Broker::Store, k: any, i: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto idx = bro_broker::val_to_data(i); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! idx ) { - zeek::emit_builtin_error("invalid Broker data conversion for index argument"); + zeek::emit_builtin_error("invalid Broker data conversion for index argument", i); return zeek::val_mgr->False(); } @@ -597,25 +607,26 @@ function Broker::__remove_from%(h: opaque of Broker::Store, k: any, i: any, function Broker::__push%(h: opaque of Broker::Store, k: any, v: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); auto val = bro_broker::val_to_data(v); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } if ( ! val ) { - zeek::emit_builtin_error("invalid Broker data conversion for value argument"); + zeek::emit_builtin_error("invalid Broker data conversion for value argument", v); return zeek::val_mgr->False(); } @@ -625,18 +636,19 @@ function Broker::__push%(h: opaque of Broker::Store, k: any, v: any, function Broker::__pop%(h: opaque of Broker::Store, k: any, e: interval%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); auto key = bro_broker::val_to_data(k); if ( ! key ) { - zeek::emit_builtin_error("invalid Broker data conversion for key argument"); + zeek::emit_builtin_error("invalid Broker data conversion for key argument", k); return zeek::val_mgr->False(); } @@ -646,14 +658,14 @@ function Broker::__pop%(h: opaque of Broker::Store, k: any, e: interval%): bool function Broker::__clear%(h: opaque of Broker::Store%): bool %{ - if ( ! h ) + auto handle = to_store_handle(h); + + if ( ! handle ) { - zeek::emit_builtin_error("invalid Broker store handle"); + zeek::emit_builtin_error("invalid Broker store handle", h); return zeek::val_mgr->False(); } - auto handle = static_cast(h); - handle->store.clear(); return zeek::val_mgr->True(); %} diff --git a/testing/btest/Baseline/broker.store.invalid-handle/out b/testing/btest/Baseline/broker.store.invalid-handle/out new file mode 100644 index 0000000000..01aac30529 --- /dev/null +++ b/testing/btest/Baseline/broker.store.invalid-handle/out @@ -0,0 +1,3 @@ +expression error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/broker.store.invalid-handle/invalid-handle.zeek, line 18: invalid Broker store handle (0), during call: (Broker::is_closed(a)) +error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/broker.store.invalid-handle/invalid-handle.zeek, line 6: invalid Broker store handle (Broker::keys(a) and 0) +keys, [status=Broker::FAILURE, result=[data=]] diff --git a/testing/btest/broker/store/invalid-handle.zeek b/testing/btest/broker/store/invalid-handle.zeek new file mode 100644 index 0000000000..c97669af60 --- /dev/null +++ b/testing/btest/broker/store/invalid-handle.zeek @@ -0,0 +1,34 @@ +# @TEST-EXEC: zeek -b %INPUT >out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out + +function print_keys(a: any) + { + when ( local s = Broker::keys(a) ) + { + print "keys", s; + } + timeout 2sec + { + print fmt(""); + } + } + +function checkit(a: any) + { + if ( Broker::is_closed(a) ) + print "this shouldn't get printed"; + else + print "this shouldn't get printed either"; + } + +global a: int = 0; + +event zeek_init() &priority=10 + { + checkit(a); + } + +event zeek_init() + { + print_keys(a); + }