diff --git a/CHANGES b/CHANGES index 8fc720d1d6..1ed0f5af6b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5.2.0-dev.173 | 2022-11-02 10:45:34 +0100 + + * GH-2440: parse.y: Traverse AST post parsing to detect break/next + usage issues (Arne Welzel, Corelight) + 5.2.0-dev.171 | 2022-11-01 07:47:41 -0700 * Func: Do not crash on va_args confusion for script funcs (Arne Welzel, Corelight) diff --git a/NEWS b/NEWS index 64c1ce2c1f..22fc74a36c 100644 --- a/NEWS +++ b/NEWS @@ -97,6 +97,10 @@ New Functionality zeek -i af_packet::eth0 +- Usage of ``break`` and ``next`` statements is now validated. It was previously + possible to place these outside of ``for``, ``while`` or ``switch`` + statements without any error indication. + Changed Functionality --------------------- diff --git a/VERSION b/VERSION index d205acf587..276bbfaeb6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -5.2.0-dev.171 +5.2.0-dev.173 diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 172b1ea3e2..3e7b0439f3 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -331,6 +331,7 @@ set(MAIN_SRCS Scope.cc ScriptCoverageManager.cc ScriptProfile.cc + ScriptValidation.cc SerializationFormat.cc SmithWaterman.cc Stats.cc diff --git a/src/ScriptValidation.cc b/src/ScriptValidation.cc new file mode 100644 index 0000000000..73942246ea --- /dev/null +++ b/src/ScriptValidation.cc @@ -0,0 +1,97 @@ +#include "zeek/ScriptValidation.h" + +#include "zeek/Func.h" +#include "zeek/Reporter.h" +#include "zeek/Stmt.h" +#include "zeek/Traverse.h" + +namespace zeek::detail + { + +// Validate context of break and next statement usage. +class BreakNextScriptValidation : public TraversalCallback + { +public: + TraversalCode PreStmt(const Stmt* stmt) + { + if ( ! StmtIsRelevant(stmt) ) + return TC_CONTINUE; + + stmt_depths[stmt->Tag()] += 1; + + if ( stmt->Tag() == STMT_BREAK && ! BreakStmtIsValid() ) + { + zeek::reporter->PushLocation(stmt->GetLocationInfo()); + zeek::reporter->Error("break statement used outside of for, while or " + "switch statement and not within a hook"); + zeek::reporter->PopLocation(); + } + + if ( stmt->Tag() == STMT_NEXT && ! NextStmtIsValid() ) + { + zeek::reporter->PushLocation(stmt->GetLocationInfo()); + zeek::reporter->Error("next statement used outside of for or while statement"); + zeek::reporter->PopLocation(); + } + + return TC_CONTINUE; + } + + TraversalCode PostStmt(const Stmt* stmt) + { + if ( ! StmtIsRelevant(stmt) ) + return TC_CONTINUE; + + --stmt_depths[stmt->Tag()]; + + assert(stmt_depths[stmt->Tag()] >= 0); + + return TC_CONTINUE; + } + + TraversalCode PreFunction(const zeek::Func* func) + { + if ( func->Flavor() == zeek::FUNC_FLAVOR_HOOK ) + ++hook_depth; + + assert(hook_depth <= 1); + + return TC_CONTINUE; + } + + TraversalCode PostFunction(const zeek::Func* func) + { + if ( func->Flavor() == zeek::FUNC_FLAVOR_HOOK ) + --hook_depth; + + assert(hook_depth >= 0); + + return TC_CONTINUE; + } + +private: + bool StmtIsRelevant(const Stmt* stmt) + { + StmtTag tag = stmt->Tag(); + return tag == STMT_FOR || tag == STMT_WHILE || tag == STMT_SWITCH || tag == STMT_BREAK || + tag == STMT_NEXT; + } + + bool BreakStmtIsValid() + { + return hook_depth > 0 || stmt_depths[STMT_FOR] > 0 || stmt_depths[STMT_WHILE] > 0 || + stmt_depths[STMT_SWITCH] > 0; + } + + bool NextStmtIsValid() { return stmt_depths[STMT_FOR] > 0 || stmt_depths[STMT_WHILE] > 0; } + + std::unordered_map stmt_depths; + int hook_depth = 0; + }; + +void script_validation() + { + zeek::detail::BreakNextScriptValidation bn_cb; + zeek::detail::traverse_all(&bn_cb); + } + } diff --git a/src/ScriptValidation.h b/src/ScriptValidation.h new file mode 100644 index 0000000000..22c9cb3b4a --- /dev/null +++ b/src/ScriptValidation.h @@ -0,0 +1,12 @@ +// See the file "COPYING" in the main distribution directory for copyright. +#pragma once + +namespace zeek::detail + { + +/** + * Run extra validations on the parsed AST after everything is initialized + * and report any errors via zeek::reporter->Error(). + */ +void script_validation(); + } diff --git a/src/parse.y b/src/parse.y index e8b2afc50a..9aa09bf799 100644 --- a/src/parse.y +++ b/src/parse.y @@ -99,6 +99,7 @@ #include "zeek/Scope.h" #include "zeek/Reporter.h" #include "zeek/ScriptCoverageManager.h" +#include "zeek/ScriptValidation.h" #include "zeek/zeekygen/Manager.h" #include "zeek/module_util.h" #include "zeek/IntrusivePtr.h" @@ -217,8 +218,8 @@ static void parse_redef_record_field(ID* id, const char* field, InitClass ic, if ( ! decl->attrs ) if ( ic == INIT_EXTRA ) decl->attrs = make_intrusive(decl->type, - true /* in_record */, - false /* is_global */); + true /* in_record */, + false /* is_global */); for ( const auto& attr : *attrs ) { @@ -399,6 +400,11 @@ zeek: else stmts = $3; + // Do some further validation on the parsed AST unless + // we already know there were errors. + if ( zeek::reporter->Errors() == 0 ) + zeek::detail::script_validation(); + // Any objects creates from here on out should not // have file positions associated with them. set_location(no_location); diff --git a/testing/btest/Baseline/language.next-break-context-errors-2/.stderr b/testing/btest/Baseline/language.next-break-context-errors-2/.stderr new file mode 100644 index 0000000000..0ba4e18c15 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-2/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 3: break statement used outside of for, while or switch statement and not within a hook diff --git a/testing/btest/Baseline/language.next-break-context-errors-3/.stderr b/testing/btest/Baseline/language.next-break-context-errors-3/.stderr new file mode 100644 index 0000000000..1a80274fec --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-3/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 3: next statement used outside of for or while statement diff --git a/testing/btest/Baseline/language.next-break-context-errors-4/.stderr b/testing/btest/Baseline/language.next-break-context-errors-4/.stderr new file mode 100644 index 0000000000..0ba4e18c15 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-4/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 3: break statement used outside of for, while or switch statement and not within a hook diff --git a/testing/btest/Baseline/language.next-break-context-errors-5/.stderr b/testing/btest/Baseline/language.next-break-context-errors-5/.stderr new file mode 100644 index 0000000000..b6579d9546 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-5/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 4: break statement used outside of for, while or switch statement and not within a hook diff --git a/testing/btest/Baseline/language.next-break-context-errors-6/.stderr b/testing/btest/Baseline/language.next-break-context-errors-6/.stderr new file mode 100644 index 0000000000..4525ed2cfd --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-6/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 7: next statement used outside of for or while statement diff --git a/testing/btest/Baseline/language.next-break-context-errors-7/.stderr b/testing/btest/Baseline/language.next-break-context-errors-7/.stderr new file mode 100644 index 0000000000..2029212d54 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-7/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 5: next statement used outside of for or while statement diff --git a/testing/btest/Baseline/language.next-break-context-errors-8/.stderr b/testing/btest/Baseline/language.next-break-context-errors-8/.stderr new file mode 100644 index 0000000000..626dedb520 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-8/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 6: next statement used outside of for or while statement diff --git a/testing/btest/Baseline/language.next-break-context-errors-9/.stderr b/testing/btest/Baseline/language.next-break-context-errors-9/.stderr new file mode 100644 index 0000000000..768ecfd690 --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors-9/.stderr @@ -0,0 +1,4 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 6: next statement used outside of for or while statement +error in <...>/next-break-context-errors.zeek, line 11: break statement used outside of for, while or switch statement and not within a hook +error in <...>/next-break-context-errors.zeek, line 16: next statement used outside of for or while statement diff --git a/testing/btest/Baseline/language.next-break-context-errors/.stderr b/testing/btest/Baseline/language.next-break-context-errors/.stderr new file mode 100644 index 0000000000..4525ed2cfd --- /dev/null +++ b/testing/btest/Baseline/language.next-break-context-errors/.stderr @@ -0,0 +1,2 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/next-break-context-errors.zeek, line 7: next statement used outside of for or while statement diff --git a/testing/btest/language/next-break-context-errors.zeek b/testing/btest/language/next-break-context-errors.zeek new file mode 100644 index 0000000000..122f96d2d6 --- /dev/null +++ b/testing/btest/language/next-break-context-errors.zeek @@ -0,0 +1,85 @@ +# @TEST-DOC: Check break and next usage within for, while, switch and hooks. + +# @TEST-EXEC-FAIL: zeek -b %INPUT +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr +function f() + { + next; + } + +event zeek_init() { f(); }; + +@TEST-START-NEXT +function f() + { + break; + } + +event zeek_init() { f(); }; + +@TEST-START-NEXT +event zeek_init() + { + next; + } + +@TEST-START-NEXT +event zeek_init() + { + break; + } + +@TEST-START-NEXT +event zeek_init() + { + if ( T ) + break; + } + +@TEST-START-NEXT +event zeek_init() + { + local history = "Sr"; + switch history { + case "S": + print history; + next; + break; + } + } + +@TEST-START-NEXT +global the_hook: hook(c: count); + +hook the_hook(c: count) + { + next; + } + +@TEST-START-NEXT +global the_hook: hook(c: count); + +hook the_hook(c: count) + { + if ( T ) + next; + } + +@TEST-START-NEXT +# Should report 3 errors. +global the_hook: hook(c: count); + +hook the_hook(c: count) + { + next; + } + +event zeek_init() + { + break; + } + +event zeek_init() + { + next; + }