From 171846a37a0561e14625c4e6746daf4b699b50c9 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Aug 2022 18:45:22 +0200 Subject: [PATCH] parse.y/directives: Reject directives as statements Avoid the issue outlined in #2289 where the @if or @else is taken as the statement of an `if`, `for` or `while` by rejecting such constructs. Effectively this means the following scripts are now rejected: # Print's "cond true" with Zeek 5.0 even though the `if ( F )` # should be in effect. if ( F ) @if ( T ) print "cond true"; @else print "cond false"; @endif or # Print's "hello" once with Zeek 5.0 local v = vector( 1, 2, 3 ); for ( i in v ) @if ( T ) print("hello") @endif To make above work as intended, additional braces can be used. if ( T ) { @if ( cond ) print "cond true"; @else print "cond false"; @endif } for ( i in v ) { @if ( T ) print("hello") @endif } --- NEWS | 7 +++ src/Stmt.cc | 8 +++ src/Stmt.h | 8 ++- src/StmtBase.h | 2 + src/input.h | 1 + src/parse.y | 7 ++- src/scan.l | 8 +++ .../language.at-if-else-if-no-way/.stderr | 2 + .../language.at-if-else-if-no-way/.stdout | 1 + .../Baseline/language.at-if-reject-2/.stderr | 2 + .../Baseline/language.at-if-reject-3/.stderr | 2 + .../Baseline/language.at-if-reject-4/.stderr | 2 + .../Baseline/language.at-if-reject-5/.stderr | 2 + .../Baseline/language.at-if-reject/.stderr | 3 + .../btest/language/at-if-else-if-no-way.zeek | 14 +++++ testing/btest/language/at-if-reject.zeek | 57 +++++++++++++++++++ 16 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 testing/btest/Baseline/language.at-if-else-if-no-way/.stderr create mode 100644 testing/btest/Baseline/language.at-if-else-if-no-way/.stdout create mode 100644 testing/btest/Baseline/language.at-if-reject-2/.stderr create mode 100644 testing/btest/Baseline/language.at-if-reject-3/.stderr create mode 100644 testing/btest/Baseline/language.at-if-reject-4/.stderr create mode 100644 testing/btest/Baseline/language.at-if-reject-5/.stderr create mode 100644 testing/btest/Baseline/language.at-if-reject/.stderr create mode 100644 testing/btest/language/at-if-else-if-no-way.zeek create mode 100644 testing/btest/language/at-if-reject.zeek 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 + }