diff --git a/CHANGES b/CHANGES index 13f63cb46b..95eb8fc5cd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,20 @@ +2.1-188 | 2012-12-03 14:04:29 -0800 + + * Hook functions now callable with "hook" expression (i.e., hook is + no longer a statement). The return value of the call is an + implicit boolean value of T if all hook handlers ran, or F if one + hook handler exited as a result of a break statement and + potentially prevented other handlers from running. + + Scripts don't need to declare hooks with an explicit return type of bool + (internally, that's assumed), and any values given to (optional) return + statements in handler definitions are just ignored. + + Addresses #918. (Jon Siwek) + + * Clarification in hook documentation. (Jon Siwek) + 2.1-184 | 2012-12-03 13:59:50 -0800 * Slightly fix up file name extraction from Content-Disposition diff --git a/VERSION b/VERSION index 8ba59f33b2..481160aafd 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.1-184 +2.1-188 diff --git a/doc/scripts/builtins.rst b/doc/scripts/builtins.rst index ba786ba0d2..322aafc2cf 100644 --- a/doc/scripts/builtins.rst +++ b/doc/scripts/builtins.rst @@ -505,15 +505,14 @@ The Bro scripting language supports the following built-in types. A hook is another flavor of function that shares characteristics of both a :bro:type:`function` and a :bro:type:`event`. They are like events in that many handler bodies can be defined for the same hook - identifier, they have no return vale, and the order of execution - can be enforced with :bro:attr:`&priority`. They are more like - functions in the way they are invoked/called, because, unlike - events, their execution is immediate and they do not get scheduled - through an event queue. Also, a unique feature of a hook is that - a given hook handler body can short-circuit the execution of - remaining hook handlers simply by exiting from the body as a result - of a ``break`` statement (as opposed to a ``return`` or just - reaching the end of the body). + identifier and the order of execution can be enforced with + :bro:attr:`&priority`. They are more like functions in the way they + are invoked/called, because, unlike events, their execution is + immediate and they do not get scheduled through an event queue. + Also, a unique feature of a hook is that a given hook handler body + can short-circuit the execution of remaining hook handlers simply by + exiting from the body as a result of a ``break`` statement (as + opposed to a ``return`` or just reaching the end of the body). A hook type is declared like:: @@ -549,18 +548,26 @@ The Bro scripting language supports the following built-in types. print "not going to happen", s; } - Note that, although the first (forward) declaration of ``myhook`` as - a hook type isn't strictly required, when it is provided, the - argument types must match. + Note that the first (forward) declaration of ``myhook`` as a hook + type isn't strictly required. Argument types must match for all + hook handlers and any forward declaration of a given hook. - To invoke immediate execution of all hook handler bodies, a ``hook`` - statement must be used: + To invoke immediate execution of all hook handler bodies, they + are called similarly to a function, except preceded by the ``hook`` + keyword: .. code:: bro hook myhook("hi"); - And the output would like like:: + or + + .. code:: bro + + if ( hook myhook("hi") ) + print "all handlers ran"; + + And the output would look like:: priority 10 myhook handler, hi break out of myhook handling, bye @@ -568,6 +575,12 @@ The Bro scripting language supports the following built-in types. Note how the modification to arguments can be seen by remaining hook handlers. + The return value of a hook call is an implicit :bro:type:`bool` + value with ``T`` meaning that all handlers for the hook were + executed and ``F`` meaning that only some of the handlers may have + executed due to one handler body exiting as a result of a ``break`` + statement. + Attributes ---------- diff --git a/src/Expr.cc b/src/Expr.cc index 733e0fe9a5..a910e0d2fb 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4394,6 +4394,13 @@ CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args, bool in_hook) return; } + if ( func_type->AsFuncType()->Flavor() == FUNC_FLAVOR_HOOK && ! in_hook ) + { + func->Error("hook cannot be called directly, use hook operator"); + SetError(); + return; + } + if ( ! func_type->MatchesIndex(args) ) SetError("argument type mismatch in function call"); else @@ -4415,13 +4422,8 @@ CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args, bool in_hook) break; case FUNC_FLAVOR_HOOK: - // It's fine to not have a yield if it's known that the call - // is being done from a hook statement. - if ( ! in_hook ) - { - Error("hook called in expression, use hook statement instead"); - SetError(); - } + Error("hook has no yield type"); + SetError(); break; default: diff --git a/src/Func.cc b/src/Func.cc index 27acce4f04..cf548b2d95 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -349,16 +349,31 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const break; } - if ( flow == FLOW_BREAK && Flavor() == FUNC_FLAVOR_HOOK ) + if ( Flavor() == FUNC_FLAVOR_HOOK ) { - // short-circuit execution of remaining hook handler bodies - break; + // Ignore any return values of hook bodies, final return value + // depends on whether a body returns as a result of break statement. + Unref(result); + result = 0; + + if ( flow == FLOW_BREAK ) + { + // Short-circuit execution of remaining hook handler bodies. + result = new Val(false, TYPE_BOOL); + break; + } } } + if ( Flavor() == FUNC_FLAVOR_HOOK ) + { + if ( ! result ) + result = new Val(true, TYPE_BOOL); + } + // Warn if the function returns something, but we returned from // the function without an explicit return, or without a value. - if ( FType()->YieldType() && FType()->YieldType()->Tag() != TYPE_VOID && + else if ( FType()->YieldType() && FType()->YieldType()->Tag() != TYPE_VOID && (flow != FLOW_RETURN /* we fell off the end */ || ! result /* explicit return with no result */) && ! f->HasDelayed() ) diff --git a/src/SerialTypes.h b/src/SerialTypes.h index a18c9bcc65..c47ff19298 100644 --- a/src/SerialTypes.h +++ b/src/SerialTypes.h @@ -165,7 +165,6 @@ SERIAL_STMT(EVENT_BODY_LIST, 16) SERIAL_STMT(INIT_STMT, 17) SERIAL_STMT(NULL_STMT, 18) SERIAL_STMT(WHEN_STMT, 19) -SERIAL_STMT(HOOK_STMT, 20) #define SERIAL_TYPE(name, val) SERIAL_CONST(name, val, BRO_TYPE) SERIAL_TYPE(BRO_TYPE, 1) diff --git a/src/Stmt.cc b/src/Stmt.cc index 0a5ae16ef6..c65b44a9bd 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -23,7 +23,7 @@ const char* stmt_name(BroStmtTag t) "print", "event", "expr", "if", "when", "switch", "for", "next", "break", "return", "add", "delete", "list", "bodylist", - "", "hook", + "", "null", }; @@ -933,52 +933,6 @@ bool EventStmt::DoUnserialize(UnserialInfo* info) return event_expr != 0; } -HookStmt::HookStmt(CallExpr* arg_e) : ExprStmt(STMT_HOOK, arg_e) - { - call_expr = arg_e; - } - -Val* HookStmt::Exec(Frame* f, stmt_flow_type& flow) const - { - RegisterAccess(); - - Val* ret = call_expr->Eval(f); - Unref(ret); - - flow = FLOW_NEXT; - - return 0; - } - -TraversalCode HookStmt::Traverse(TraversalCallback* cb) const - { - TraversalCode tc = cb->PreStmt(this); - HANDLE_TC_STMT_PRE(tc); - - // call expr is stored in base class's "e" field. - tc = e->Traverse(cb); - HANDLE_TC_STMT_PRE(tc); - - tc = cb->PostStmt(this); - HANDLE_TC_STMT_POST(tc); - } - -IMPLEMENT_SERIAL(HookStmt, SER_HOOK_STMT); - -bool HookStmt::DoSerialize(SerialInfo* info) const - { - DO_SERIALIZE(SER_HOOK_STMT, ExprStmt); - return call_expr->Serialize(info); - } - -bool HookStmt::DoUnserialize(UnserialInfo* info) - { - DO_UNSERIALIZE(ExprStmt); - - call_expr = (CallExpr*) Expr::Unserialize(info, EXPR_CALL); - return call_expr != 0; - } - ForStmt::ForStmt(id_list* arg_loop_vars, Expr* loop_expr) : ExprStmt(STMT_FOR, loop_expr) { @@ -1378,7 +1332,10 @@ ReturnStmt::ReturnStmt(Expr* arg_e) : ExprStmt(STMT_RETURN, arg_e) } else if ( ! e ) - Error("return statement needs expression"); + { + if ( ft->Flavor() != FUNC_FLAVOR_HOOK ) + Error("return statement needs expression"); + } else (void) check_and_promote_expr(e, yt); @@ -1990,7 +1947,6 @@ int same_stmt(const Stmt* s1, const Stmt* s2) case STMT_RETURN: case STMT_EXPR: case STMT_EVENT: - case STMT_HOOK: { const ExprStmt* e1 = (const ExprStmt*) s1; const ExprStmt* e2 = (const ExprStmt*) s2; diff --git a/src/Stmt.h b/src/Stmt.h index 68bb8d6425..7c3b42609b 100644 --- a/src/Stmt.h +++ b/src/Stmt.h @@ -286,24 +286,6 @@ protected: EventExpr* event_expr; }; -class HookStmt : public ExprStmt { -public: - HookStmt(CallExpr* e); - - Val* Exec(Frame* f, stmt_flow_type& flow) const; - - TraversalCode Traverse(TraversalCallback* cb) const; - -protected: - friend class Stmt; - - HookStmt() { call_expr = 0; } - - DECLARE_SERIAL(HookStmt); - - CallExpr* call_expr; -}; - class ForStmt : public ExprStmt { public: ForStmt(id_list* loop_vars, Expr* loop_expr); diff --git a/src/StmtEnums.h b/src/StmtEnums.h index fa5b70389d..f431e3fea1 100644 --- a/src/StmtEnums.h +++ b/src/StmtEnums.h @@ -15,7 +15,7 @@ typedef enum { STMT_RETURN, STMT_ADD, STMT_DELETE, STMT_LIST, STMT_EVENT_BODY_LIST, - STMT_INIT, STMT_HOOK, + STMT_INIT, STMT_NULL #define NUM_STMTS (int(STMT_NULL) + 1) } BroStmtTag; diff --git a/src/Var.cc b/src/Var.cc index 9c4fb5b978..68be18b55e 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -294,12 +294,12 @@ void add_type(ID* id, BroType* t, attr_list* attr, int /* is_event */) void begin_func(ID* id, const char* module_name, function_flavor flavor, int is_redef, FuncType* t) { - if ( flavor == FUNC_FLAVOR_EVENT || flavor == FUNC_FLAVOR_HOOK ) + if ( flavor == FUNC_FLAVOR_EVENT ) { const BroType* yt = t->YieldType(); if ( yt && yt->Tag() != TYPE_VOID ) - id->Error("event/hook cannot yield a value", t); + id->Error("event cannot yield a value", t); t->ClearYieldType(flavor); } diff --git a/src/parse.y b/src/parse.y index b4eee1a56c..0385c1c4e1 100644 --- a/src/parse.y +++ b/src/parse.y @@ -32,6 +32,7 @@ %token TOK_NO_TEST +%nonassoc TOK_HOOK %left ',' '|' %right '=' TOK_ADD_TO TOK_REMOVE_FROM %right '?' ':' @@ -56,7 +57,6 @@ %type pattern %type expr init anonymous_function %type event -%type hook %type stmt stmt_list func_body for_head %type type opt_type enum_body %type func_hdr func_params @@ -119,6 +119,7 @@ extern const char* g_curr_debug_error; #define YYLTYPE yyltype +static int in_hook = 0; int in_init = 0; int in_record = 0; bool resolving_global_ID = false; @@ -212,7 +213,6 @@ static std::list* concat_opt_docs (std::list* pre, Val* val; RE_Matcher* re; Expr* expr; - CallExpr* call_expr; EventExpr* event_expr; Stmt* stmt; ListExpr* list; @@ -517,7 +517,16 @@ expr: | expr '(' opt_expr_list ')' { set_location(@1, @4); - $$ = new CallExpr($1, $3); + $$ = new CallExpr($1, $3, in_hook > 0); + } + + | TOK_HOOK { ++in_hook; } expr + { + --in_hook; + set_location(@1, @3); + if ( $3->Tag() != EXPR_CALL ) + $3->Error("not a valid hook call expression"); + $$ = $3; } | expr TOK_HAS_FIELD TOK_ID @@ -875,7 +884,7 @@ type: | TOK_HOOK '(' formal_args ')' { set_location(@1, @3); - $$ = new FuncType($3, 0, FUNC_FLAVOR_HOOK); + $$ = new FuncType($3, base_type(TYPE_BOOL), FUNC_FLAVOR_HOOK); } | TOK_FILE TOK_OF type @@ -1209,6 +1218,8 @@ func_hdr: } | TOK_HOOK def_global_id func_params { + $3->ClearYieldType(FUNC_FLAVOR_HOOK); + $3->SetYieldType(base_type(TYPE_BOOL)); begin_func($2, current_module.c_str(), FUNC_FLAVOR_HOOK, 0, $3); $$ = $3; @@ -1372,14 +1383,6 @@ stmt: brofiler.AddStmt($$); } - | TOK_HOOK hook ';' opt_no_test - { - set_location(@1, @4); - $$ = new HookStmt($2); - if ( ! $4 ) - brofiler.AddStmt($$); - } - | TOK_IF '(' expr ')' stmt { set_location(@1, @4); @@ -1533,14 +1536,6 @@ event: } ; -hook: - expr '(' opt_expr_list ')' - { - set_location(@1, @4); - $$ = new CallExpr($1, $3, true); - } - ; - case_list: case_list case { $1->append($2); } diff --git a/testing/btest/Baseline/language.hook/out b/testing/btest/Baseline/language.hook/out index 10688acc3b..bef25193b8 100644 --- a/testing/btest/Baseline/language.hook/out +++ b/testing/btest/Baseline/language.hook/out @@ -1,7 +1,18 @@ myhook, &priority=10, [a=1156, b=hello world] +myhook return F +myhook return T myhook, &priority=5, [a=37, b=goobye world] +F myhook3, 8 +T myhook4, 2 myhook4, 1 +T +myhook4, 2 +myhook4, 1 +myhook4 all handlers ran myhook, &priority=10, [a=2, b=it works] +myhook return F +myhook return T myhook, &priority=5, [a=37, b=goobye world] +F diff --git a/testing/btest/Baseline/language.hook_calls/invalid.out b/testing/btest/Baseline/language.hook_calls/invalid.out new file mode 100644 index 0000000000..3412c1900e --- /dev/null +++ b/testing/btest/Baseline/language.hook_calls/invalid.out @@ -0,0 +1,10 @@ +error in ./invalid.bro, line 9: hook cannot be called directly, use hook operator (myhook) +warning in ./invalid.bro, line 9: expression value ignored (myhook(3)) +error in ./invalid.bro, line 10: hook cannot be called directly, use hook operator (myhook) +error in ./invalid.bro, line 11: hook cannot be called directly, use hook operator (myhook) +error in ./invalid.bro, line 12: not a valid hook call expression (2 + 2) +warning in ./invalid.bro, line 12: expression value ignored (2 + 2) +error in ./invalid.bro, line 13: not a valid hook call expression (2 + 2) +error in ./invalid.bro, line 15: hook cannot be called directly, use hook operator (h) +warning in ./invalid.bro, line 15: expression value ignored (h(3)) +error in ./invalid.bro, line 16: hook cannot be called directly, use hook operator (h) diff --git a/testing/btest/Baseline/language.hook_calls/valid.out b/testing/btest/Baseline/language.hook_calls/valid.out new file mode 100644 index 0000000000..c711115315 --- /dev/null +++ b/testing/btest/Baseline/language.hook_calls/valid.out @@ -0,0 +1,42 @@ +myhook(), 3 +other myhook(), 3 +myhook(), 3 +other myhook(), 3 +T +myhook(), 0 +F +----------- +indirect() +myhook(), 3 +other myhook(), 3 +indirect() +myhook(), 3 +other myhook(), 3 +T +----------- +really_indirect() +indirect() +myhook(), 3 +other myhook(), 3 +really_indirect() +indirect() +myhook(), 3 +other myhook(), 3 +T +----------- +myhook(), 3 +other myhook(), 3 +myhook(), 3 +other myhook(), 3 +T +myhook(), 3 +other myhook(), 3 +yes +myhook(), 0 +double yes +----------- +myhook(), 3 +other myhook(), 3 +myhook(), 3 +other myhook(), 3 +T diff --git a/testing/btest/Baseline/language.invalid_hook/out b/testing/btest/Baseline/language.invalid_hook/out deleted file mode 100644 index 167d62ccc8..0000000000 --- a/testing/btest/Baseline/language.invalid_hook/out +++ /dev/null @@ -1 +0,0 @@ -error in /Users/jsiwek/Projects/bro/bro/testing/btest/.tmp/language.invalid_hook/invalid_hook.bro, line 15: hook called in expression, use hook statement instead (myhook(nope)) diff --git a/testing/btest/core/leaks/hook.bro b/testing/btest/core/leaks/hook.bro index eadb406e71..9234184317 100644 --- a/testing/btest/core/leaks/hook.bro +++ b/testing/btest/core/leaks/hook.bro @@ -39,7 +39,25 @@ hook myhook(r: rec) &priority=10 r$b = "goobye world"; # returning from the handler early, is fine, remaining handlers still run. return; - print "ERROR: break statement should return from hook handler body"; + print "ERROR: return statement should return from hook handler body"; + } + +hook myhook(r: rec) &priority=9 + { + print "myhook return F"; + # return value is ignored, remaining handlers still run, final return + # value is whether any hook body returned via break statement + return F; + print "ERROR: return statement should return from hook handler body"; + } + +hook myhook(r: rec) &priority=8 + { + print "myhook return T"; + # return value is ignored, remaining handlers still run, final return + # value is whether any hook body returned via break statement + return T; + print "ERROR: return statement should return from hook handler body"; } # hook function doesn't need a declaration, we can go straight to defining @@ -63,16 +81,20 @@ event new_connection(c: connection) { print "new_connection", c$id; - hook myhook([$a=1156, $b="hello world"]); + print hook myhook([$a=1156, $b="hello world"]); # A hook with no handlers is fine, it's just a no-op. - hook myhook2("nope"); + print hook myhook2("nope"); - hook myhook3(8); - hook myhook4(); + print hook myhook3(8); + print hook myhook4(); + if ( hook myhook4() ) + { + print "myhook4 all handlers ran"; + } # A hook can be treated like other data types and doesn't have to be # invoked directly by name. local h = myhook; - hook h([$a=2, $b="it works"]); + print hook h([$a=2, $b="it works"]); } diff --git a/testing/btest/language/hook.bro b/testing/btest/language/hook.bro index 8f7a85ce95..9c9ab30c18 100644 --- a/testing/btest/language/hook.bro +++ b/testing/btest/language/hook.bro @@ -34,7 +34,25 @@ hook myhook(r: rec) &priority=10 r$b = "goobye world"; # returning from the handler early, is fine, remaining handlers still run. return; - print "ERROR: break statement should return from hook handler body"; + print "ERROR: return statement should return from hook handler body"; + } + +hook myhook(r: rec) &priority=9 + { + print "myhook return F"; + # return value is ignored, remaining handlers still run, final return + # value is whether any hook body returned via break statement + return F; + print "ERROR: return statement should return from hook handler body"; + } + +hook myhook(r: rec) &priority=8 + { + print "myhook return T"; + # return value is ignored, remaining handlers still run, final return + # value is whether any hook body returned via break statement + return T; + print "ERROR: return statement should return from hook handler body"; } # hook function doesn't need a declaration, we can go straight to defining @@ -56,16 +74,20 @@ hook myhook4() &priority=2 event bro_init() { - hook myhook([$a=1156, $b="hello world"]); + print hook myhook([$a=1156, $b="hello world"]); # A hook with no handlers is fine, it's just a no-op. - hook myhook2("nope"); + print hook myhook2("nope"); - hook myhook3(8); - hook myhook4(); + print hook myhook3(8); + print hook myhook4(); + if ( hook myhook4() ) + { + print "myhook4 all handlers ran"; + } # A hook can be treated like other data types and doesn't have to be # invoked directly by name. local h = myhook; - hook h([$a=2, $b="it works"]); + print hook h([$a=2, $b="it works"]); } diff --git a/testing/btest/language/hook_calls.bro b/testing/btest/language/hook_calls.bro new file mode 100644 index 0000000000..41ef6f52ae --- /dev/null +++ b/testing/btest/language/hook_calls.bro @@ -0,0 +1,82 @@ +# @TEST-EXEC: bro -b valid.bro >valid.out +# @TEST-EXEC: btest-diff valid.out +# @TEST-EXEC-FAIL: bro -b invalid.bro > invalid.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff invalid.out + +# hook functions must be called using the "hook" keyword as an operator... + +@TEST-START-FILE valid.bro +hook myhook(i: count) + { + print "myhook()", i; + if ( i == 0 ) break; + } + +hook myhook(i: count) &priority=-1 + { + print "other myhook()", i; + } + +function indirect(): hook(i: count) + { + print "indirect()"; + return myhook; + } + +function really_indirect(): function(): hook(i: count) + { + print "really_indirect()"; + return indirect; + } + +global t: table[count] of hook(i: count) = { + [0] = myhook, +}; + +event bro_init() + { + hook myhook(3); + print hook myhook(3); + print hook myhook(0); + print "-----------"; + hook indirect()(3); + print hook indirect()(3); + print "-----------"; + hook really_indirect()()(3); + print hook really_indirect()()(3); + print "-----------"; + local h = t[0]; + hook h(3); + print hook h(3); + if ( hook h(3) ) + print "yes"; + if ( ! hook h(0) ) + print "double yes"; + print "-----------"; + hook t[0](3); + print hook t[0](3); + } + +@TEST-END-FILE + +@TEST-START-FILE invalid.bro +hook myhook(i: count) + { + print "myhook()", i; + if ( i == 0 ) break; + } + +event bro_init() + { + myhook(3); + print myhook(3); + print myhook(0); + hook 2+2; + print hook 2+2; + local h = myhook; + h(3); + if ( h(3) ) + print "hmm"; + print "done"; + } +@TEST-END-FILE diff --git a/testing/btest/language/invalid_hook.bro b/testing/btest/language/invalid_hook.bro deleted file mode 100644 index 0dbbfd1b6f..0000000000 --- a/testing/btest/language/invalid_hook.bro +++ /dev/null @@ -1,16 +0,0 @@ -# @TEST-EXEC-FAIL: bro -b %INPUT >out 2>&1 -# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out - -global myhook: hook(s: string); - -hook myhook(s: string) - { - print "myhook", s; - } - -event bro_init() - { - # hooks must be invoked with a "hook", statement. They have no return - # value and don't make sense to evaluate as arbitrary expressions. - local r = myhook("nope"); - }