diff --git a/CHANGES b/CHANGES index dd351b1134..67314647e7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,18 @@ +8.1.0-dev.87 | 2025-08-18 14:39:10 -0400 + + * Fix parameter attributes pretending to be records (Evan Typanski, Corelight) + + Parameters relied on is_record for a couple of validations, but they are + not records and should not be treated as such. This way we can validate + &optional better. + + * Only allow `&optional` in records (Evan Typanski, Corelight) + + There was some confusing behavior with &optional and locals, so this + should get rid of that by making it an error. However, there is a case + where function parameters are still allowed to have &optional - this is + because there are checks for &default in parameters as well. + 8.1.0-dev.83 | 2025-08-18 09:40:41 -0700 * Add `record_type_to_vector` deprecation to NEWS (Evan Typanski, Corelight) diff --git a/NEWS b/NEWS index 5b4c6a615e..f6d562464d 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,9 @@ We would like to thank ... for their contributions to this release. Breaking Changes ---------------- +- The ``&optional`` script attribute will now error when applied to anything that's + not a record field. Previously, this would have surprising behavior. + New Functionality ----------------- diff --git a/VERSION b/VERSION index 007f2e0849..4a5391d846 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -8.1.0-dev.83 +8.1.0-dev.87 diff --git a/src/Attr.cc b/src/Attr.cc index 52c7c461b6..9a8aa3396a 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -166,13 +166,9 @@ detail::TraversalCode Attr::Traverse(detail::TraversalCallback* cb) const { HANDLE_TC_ATTR_POST(tc); } -Attributes::Attributes(TypePtr t, bool arg_in_record, bool is_global) - : Attributes(std::vector{}, std::move(t), arg_in_record, is_global) {} - -Attributes::Attributes(std::vector a, TypePtr t, bool arg_in_record, bool is_global) : type(std::move(t)) { +Attributes::Attributes(std::vector a, TypePtr t, bool in_record, bool is_global, bool is_param) + : type(std::move(t)), in_record(in_record), global_var(is_global), is_param(is_param) { attrs.reserve(a.size()); - in_record = arg_in_record; - global_var = is_global; SetLocationInfo(&start_location, &end_location); @@ -297,10 +293,10 @@ bool Attributes::CheckAttr(Attr* a) { case ATTR_IS_USED: break; case ATTR_OPTIONAL: - if ( global_var ) - return AttrError("&optional is not valid for global variables"); + if ( ! in_record ) + return AttrError("&optional is only valid for record fields"); - if ( in_record && Find(ATTR_DEFAULT) ) + if ( Find(ATTR_DEFAULT) ) return AttrError("Using &default and &optional together results in &default behavior"); break; @@ -331,7 +327,7 @@ bool Attributes::CheckAttr(Attr* a) { return AttrError("&default and &default_insert cannot be used together"); std::string err_msg; - if ( ! check_default_attr(a, type, global_var, in_record, err_msg) && ! err_msg.empty() ) + if ( ! check_default_attr(a, type, global_var, in_record || is_param, err_msg) && ! err_msg.empty() ) return AttrError(err_msg.c_str()); break; } @@ -344,7 +340,7 @@ bool Attributes::CheckAttr(Attr* a) { return AttrError("Using &default and &optional together results in &default behavior"); std::string err_msg; - if ( ! check_default_attr(a, type, global_var, in_record, err_msg) && ! err_msg.empty() ) + if ( ! check_default_attr(a, type, global_var, in_record || is_param, err_msg) && ! err_msg.empty() ) return AttrError(err_msg.c_str()); break; } @@ -586,7 +582,7 @@ bool Attributes::operator==(const Attributes& other) const { return true; } -bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_record, std::string& err_msg) { +bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool record_like, std::string& err_msg) { ASSERT(a->Tag() == ATTR_DEFAULT || a->Tag() == ATTR_DEFAULT_INSERT); std::string aname = attr_name(a->Tag()); // &default is allowed for global tables, since it's used in @@ -598,7 +594,7 @@ bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_r const auto& atype = a->GetExpr()->GetType(); - if ( type->Tag() != TYPE_TABLE || (type->IsSet() && ! in_record) ) { + if ( type->Tag() != TYPE_TABLE || (type->IsSet() && ! record_like) ) { if ( same_type(atype, type) ) // Ok. return true; @@ -628,7 +624,7 @@ bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_r TableType* tt = type->AsTableType(); const auto& ytype = tt->Yield(); - if ( ! in_record ) { // &default applies to the type itself. + if ( ! record_like ) { // &default applies to the type itself. if ( same_type(atype, ytype) ) return true; diff --git a/src/Attr.h b/src/Attr.h index e44de83b20..15cb471c20 100644 --- a/src/Attr.h +++ b/src/Attr.h @@ -108,8 +108,14 @@ protected: // Manages a collection of attributes. class Attributes final : public Obj { public: - Attributes(std::vector a, TypePtr t, bool in_record, bool is_global); - Attributes(TypePtr t, bool in_record, bool is_global); + Attributes(std::vector a, TypePtr t, bool in_record, bool is_global, bool is_param); + + Attributes(TypePtr t, bool in_record, bool is_global) + : Attributes(std::vector{}, std::move(t), in_record, is_global, false) {} + + Attributes(std::vector a, TypePtr t, bool in_record, bool is_global) + : Attributes(a, std::move(t), in_record, is_global, false) {} + ~Attributes() override = default; @@ -142,6 +148,7 @@ protected: bool in_record; bool global_var; + bool is_param; }; // Checks whether default attribute "a" is compatible with the given type. diff --git a/src/parse.y b/src/parse.y index 0220e64615..ea4fc7ae26 100644 --- a/src/parse.y +++ b/src/parse.y @@ -257,11 +257,11 @@ static void extend_record(ID* id, std::unique_ptr fields, std::u } } -static AttributesPtr make_attributes(std::vector* attrs, TypePtr t, bool in_record, bool is_global) { +static AttributesPtr make_attributes(std::vector* attrs, TypePtr t, bool in_record, bool is_param) { if ( ! attrs ) return nullptr; - auto rval = make_intrusive(std::move(*attrs), std::move(t), in_record, is_global); + auto rval = make_intrusive(std::move(*attrs), std::move(t), in_record, /*is_global=*/false, is_param); delete attrs; return rval; } @@ -1296,7 +1296,7 @@ type_decl: TOK_ID ':' type opt_attr ';' { set_location(@1, @4); - auto attrs = make_attributes($4, {NewRef{}, $3}, in_record > 0, false); + auto attrs = make_attributes($4, {NewRef{}, $3}, in_record > 0, /*is_param=*/false); $$ = new TypeDecl($1, {AdoptRef{}, $3}, std::move(attrs)); if ( in_record > 0 && cur_decl_type_id ) @@ -1327,7 +1327,7 @@ formal_args_decl: TOK_ID ':' type opt_attr { set_location(@1, @4); - auto attrs = make_attributes($4, {NewRef{}, $3}, true, false); + auto attrs = make_attributes($4, {NewRef{}, $3}, /*in_record=*/false, /*is_param=*/true); $$ = new TypeDecl($1, {AdoptRef{}, $3}, std::move(attrs)); } ; diff --git a/testing/btest/Baseline/language.attr-default-global-set-error/out b/testing/btest/Baseline/language.attr-default-global-set-error/out index 431b2d445b..683c0c0db5 100644 --- a/testing/btest/Baseline/language.attr-default-global-set-error/out +++ b/testing/btest/Baseline/language.attr-default-global-set-error/out @@ -2,5 +2,5 @@ error in <...>/attr-default-global-set-error.zeek, line 4: &default is not valid for global variables except for tables (&default=0) error in <...>/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=10) error in <...>/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=9) -error in <...>/attr-default-global-set-error.zeek, line 9: &optional is not valid for global variables (&optional) +error in <...>/attr-default-global-set-error.zeek, line 9: &optional is only valid for record fields (&optional) error in <...>/attr-default-global-set-error.zeek, line 10: &default is not valid for global variables except for tables (&default=set()) diff --git a/testing/btest/Baseline/language.invalid-optional-attr/out b/testing/btest/Baseline/language.invalid-optional-attr/out new file mode 100644 index 0000000000..b9e20e582c --- /dev/null +++ b/testing/btest/Baseline/language.invalid-optional-attr/out @@ -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 <...>/invalid-optional-attr.zeek, line 5: &optional is only valid for record fields (&optional) +error in <...>/invalid-optional-attr.zeek, line 8: &optional is only valid for record fields (&optional) +error in <...>/invalid-optional-attr.zeek, line 11: &optional is only valid for record fields (&optional) diff --git a/testing/btest/language/invalid-optional-attr.zeek b/testing/btest/language/invalid-optional-attr.zeek new file mode 100644 index 0000000000..33d6903f91 --- /dev/null +++ b/testing/btest/language/invalid-optional-attr.zeek @@ -0,0 +1,12 @@ +# @TEST-EXEC-FAIL: zeek -b %INPUT >out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out + +# Invalid on globals +global a: int &optional; + +# Invalid on parameters +function f(a: int &optional) + { + # Invalid in locals + local b: int &optional; + }