Merge remote-tracking branch 'origin/topic/awelzel/2440-break-next-us…

…age-validation'

* origin/topic/awelzel/2440-break-next-usage-validation:
  parse.y: Traverse AST post parsing to detect break/next usage issues

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)

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
 ---------------------
 

VERSION

@@ -1 +1 @@
-5.2.0-dev.171
+5.2.0-dev.173

src/CMakeLists.txt

@@ -331,6 +331,7 @@ set(MAIN_SRCS
     Scope.cc
     ScriptCoverageManager.cc
     ScriptProfile.cc
+    ScriptValidation.cc
     SerializationFormat.cc
     SmithWaterman.cc
     Stats.cc

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<const StmtTag, int> stmt_depths;
+	int hook_depth = 0;
+	};
+
+void script_validation()
+	{
+	zeek::detail::BreakNextScriptValidation bn_cb;
+	zeek::detail::traverse_all(&bn_cb);
+	}
+	}

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();
+	}

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<detail::Attributes>(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);

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

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

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

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

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

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

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

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

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

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;
+	}