diff --git a/NEWS b/NEWS index 8a6c383e01..e74a497837 100644 --- a/NEWS +++ b/NEWS @@ -60,6 +60,13 @@ Breaking Changes value alone, we aren't able to deprecate the original version of the method. This may cause build of packages to fail if they were using this method. +- Conditional directives (``@if``, ``@ifdef``, ``@ifndef``, ``@else`` and ``@endif``) + can not be placed directly following ``if``, ``for`` or ``while`` statements + anymore. This was interpreted non-intuitively and could lead to subtle bugs. + The statement following the directive was placed outside of its intended block. + Placing braces after ``if``, ``for`` or ``while`` should result in the + intended behavior. + New Functionality ----------------- diff --git a/src/Stmt.cc b/src/Stmt.cc index 7d2b8d93c9..0d6a6dfe10 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -158,6 +158,12 @@ const ReturnStmt* Stmt::AsReturnStmt() const return (const ReturnStmt*)this; } +const NullStmt* Stmt::AsNullStmt() const + { + CHECK_TAG(tag, STMT_NULL, "Stmt::AsNullStmt", stmt_name) + return (const NullStmt*)this; + } + bool Stmt::SetLocationInfo(const Location* start, const Location* end) { if ( ! Obj::SetLocationInfo(start, end) ) @@ -1785,6 +1791,8 @@ TraversalCode InitStmt::Traverse(TraversalCallback* cb) const HANDLE_TC_STMT_POST(tc); } +NullStmt::NullStmt(bool arg_is_directive) : Stmt(STMT_NULL), is_directive(arg_is_directive) { } + ValPtr NullStmt::Exec(Frame* /* f */, StmtFlowType& flow) { RegisterAccess(); diff --git a/src/Stmt.h b/src/Stmt.h index 2fd18d225d..7c293e7c5a 100644 --- a/src/Stmt.h +++ b/src/Stmt.h @@ -525,7 +525,7 @@ protected: class NullStmt final : public Stmt { public: - NullStmt() : Stmt(STMT_NULL) { } + NullStmt(bool arg_is_directive = false); ValPtr Exec(Frame* f, StmtFlowType& flow) override; bool IsPure() const override; @@ -536,6 +536,12 @@ public: // Optimization-related: StmtPtr Duplicate() override { return SetSucc(new NullStmt()); } + + // Returns true if this NullStmt represents a directive (@if..., @else, @endif) + bool IsDirective() const { return is_directive; }; + +private: + bool is_directive; }; // A helper class for tracking all of the information associated with diff --git a/src/StmtBase.h b/src/StmtBase.h index d70cf8b0aa..dc7bb375c3 100644 --- a/src/StmtBase.h +++ b/src/StmtBase.h @@ -34,6 +34,7 @@ class ExprStmt; class ForStmt; class IfStmt; class InitStmt; +class NullStmt; class PrintStmt; class ReturnStmt; class StmtList; @@ -92,6 +93,7 @@ public: const WhileStmt* AsWhileStmt() const; const WhenStmt* AsWhenStmt() const; const SwitchStmt* AsSwitchStmt() const; + const NullStmt* AsNullStmt() const; void RegisterAccess() const { diff --git a/src/input.h b/src/input.h index 8b34bef4a6..205ffe734d 100644 --- a/src/input.h +++ b/src/input.h @@ -28,6 +28,7 @@ extern void do_atifdef(const char* id); extern void do_atifndef(const char* id); extern void do_atelse(); extern void do_atendif(); +extern void reject_directive(zeek::detail::Stmt* s); extern void do_doc_token_start(); extern void do_doc_token_stop(); diff --git a/src/parse.y b/src/parse.y index 2e04b653c5..3e98d60944 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1773,6 +1773,7 @@ stmt: | TOK_IF '(' expr ')' stmt { + reject_directive($5); set_location(@1, @4); $$ = new IfStmt({AdoptRef{}, $3}, {AdoptRef{}, $5}, make_intrusive()); script_coverage_mgr.AddStmt($$); @@ -1780,6 +1781,8 @@ stmt: | TOK_IF '(' expr ')' stmt TOK_ELSE stmt { + reject_directive($5); + reject_directive($7); set_location(@1, @4); $$ = new IfStmt({AdoptRef{}, $3}, {AdoptRef{}, $5}, {AdoptRef{}, $7}); script_coverage_mgr.AddStmt($$); @@ -1794,12 +1797,14 @@ stmt: | for_head stmt { + reject_directive($2); $1->AsForStmt()->AddBody({AdoptRef{}, $2}); script_coverage_mgr.AddStmt($1); } | TOK_WHILE '(' expr ')' stmt { + reject_directive($5); $$ = new WhileStmt({AdoptRef{}, $3}, {AdoptRef{}, $5}); script_coverage_mgr.AddStmt($$); } @@ -1906,7 +1911,7 @@ stmt: } | conditional - { $$ = new NullStmt; } + { $$ = new NullStmt(true /* is_directive */); } ; stmt_list: diff --git a/src/scan.l b/src/scan.l index c9397e7431..c34979ddd9 100644 --- a/src/scan.l +++ b/src/scan.l @@ -847,6 +847,14 @@ void do_atendif() --conditional_depth; } +// If the given Stmt is a directive, trigger an error so that its usage is rejected. +void reject_directive(zeek::detail::Stmt* s) + { + if ( s->Tag() == STMT_NULL ) + if ( s->AsNullStmt()->IsDirective() ) + zeek::reporter->Error("incorrect use of directive"); + } + // Be careful to never delete things from this list, as the strings // are referred to (in order to save the locations of tokens and statements, // for error reporting and debugging). diff --git a/testing/btest/Baseline/language.at-if-else-if-no-way/.stderr b/testing/btest/Baseline/language.at-if-else-if-no-way/.stderr new file mode 100644 index 0000000000..064fad0c5e --- /dev/null +++ b/testing/btest/Baseline/language.at-if-else-if-no-way/.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 <...>/at-if-else-if-no-way.zeek, line 12: incorrect use of directive diff --git a/testing/btest/Baseline/language.at-if-else-if-no-way/.stdout b/testing/btest/Baseline/language.at-if-else-if-no-way/.stdout new file mode 100644 index 0000000000..49d861c74c --- /dev/null +++ b/testing/btest/Baseline/language.at-if-else-if-no-way/.stdout @@ -0,0 +1 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. diff --git a/testing/btest/Baseline/language.at-if-reject-2/.stderr b/testing/btest/Baseline/language.at-if-reject-2/.stderr new file mode 100644 index 0000000000..73440016e3 --- /dev/null +++ b/testing/btest/Baseline/language.at-if-reject-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 <...>/at-if-reject.zeek, line 6: incorrect use of directive diff --git a/testing/btest/Baseline/language.at-if-reject-3/.stderr b/testing/btest/Baseline/language.at-if-reject-3/.stderr new file mode 100644 index 0000000000..2920349de8 --- /dev/null +++ b/testing/btest/Baseline/language.at-if-reject-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 <...>/at-if-reject.zeek, line 5: incorrect use of directive diff --git a/testing/btest/Baseline/language.at-if-reject-4/.stderr b/testing/btest/Baseline/language.at-if-reject-4/.stderr new file mode 100644 index 0000000000..2920349de8 --- /dev/null +++ b/testing/btest/Baseline/language.at-if-reject-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 <...>/at-if-reject.zeek, line 5: incorrect use of directive diff --git a/testing/btest/Baseline/language.at-if-reject-5/.stderr b/testing/btest/Baseline/language.at-if-reject-5/.stderr new file mode 100644 index 0000000000..73440016e3 --- /dev/null +++ b/testing/btest/Baseline/language.at-if-reject-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 <...>/at-if-reject.zeek, line 6: incorrect use of directive diff --git a/testing/btest/Baseline/language.at-if-reject/.stderr b/testing/btest/Baseline/language.at-if-reject/.stderr new file mode 100644 index 0000000000..2a5368c7dc --- /dev/null +++ b/testing/btest/Baseline/language.at-if-reject/.stderr @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/at-if-reject.zeek, line 9: incorrect use of directive +error in <...>/at-if-reject.zeek, line 13: syntax error, at or near "else" diff --git a/testing/btest/language/at-if-else-if-no-way.zeek b/testing/btest/language/at-if-else-if-no-way.zeek new file mode 100644 index 0000000000..002cf1c8b1 --- /dev/null +++ b/testing/btest/language/at-if-else-if-no-way.zeek @@ -0,0 +1,14 @@ +# @TEST-DOC: Regression test for #2289 from vpax - previously this printed "There's way this should happen", now it's a syntax error. +# @TEST-EXEC-FAIL: zeek -b %INPUT +# @TEST-EXEC: btest-diff .stdout +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr + +event zeek_init() + { +@if ( T ) + if ( F ) +@else + if ( F ) +@endif + print "There's no way this should happen"; + } diff --git a/testing/btest/language/at-if-reject.zeek b/testing/btest/language/at-if-reject.zeek new file mode 100644 index 0000000000..1e4e1b74d9 --- /dev/null +++ b/testing/btest/language/at-if-reject.zeek @@ -0,0 +1,57 @@ +# @TEST-DOC: Test for #2289 - reject directives appearing as statements +# @TEST-EXEC-FAIL: zeek -b %INPUT +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr + +event zeek_init() + { + if ( F ) + @if ( T ) + print "Bad branch true"; + @else + print "Bad branch false"; + @endif + else + print "That's the right branch"; + } + +@TEST-START-NEXT +event zeek_init() + { + if ( F ) + print "That would be okay"; + else + @if ( T ) + print "That isn't"; + @endif + } + +@TEST-START-NEXT +event zeek_init() + { + local vec = vector(1, 2, 3); + for ( i in vec ) + @if ( T ) + print "Bad branch true"; + @endif + } + +@TEST-START-NEXT +event zeek_init() + { + local i = 10; + while ( --i != 0 ) + @if ( T ) + print "Bad branch true"; + @endif + } + +@TEST-START-NEXT +global cond = T; +event zeek_init() + { + local vec = vector(1, 2, 3); + for ( i in vec ) + @if ( cond ) + print "Bad branch true"; + @endif + }