diff --git a/CHANGES b/CHANGES index 9f5288efd5..9395d92f2d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,29 @@ +3.2.0-dev.892 | 2020-07-13 12:10:22 -0700 + + * Fix wrong frame offsets for locals of alternate event/hook prototypes + + Local frame offsets were being assigned based on number of the alternate + prototype's parameters, which may end up having less total parameters + than the canonical prototype, causing the local value to incorrectly + overwrite an event/hook argument value. (Jon Siwek, Corelight) + + * Add deprecation expression to deprecated prototype/parameter messages (Jon Siwek, Corelight) + + * Improve "use of deprecated prototype" warning message + + The location information now points out the place of the deprecated + prototype instead of the location where the ID was initially declared + (which may not itself be a deprecated prototype). (Jon Siwek, Corelight) + + * Emit deprecation warning for use of &deprecated function parameters + + Particularly, this is meant for using &deprecated on canonical + event/hook prototype parameters to encourage users to create handlers + to another, non-deprecated prototype. i.e. for canonical prototypes, + we may not always want to put &deprecated directly on the prototype + itself since that signals deprecation of the ID entirely. (Jon Siwek, Corelight) + 3.2.0-dev.885 | 2020-07-10 11:20:41 -0700 * Add more error checks to shadow log parsing (Jon Siwek, Corelight) diff --git a/VERSION b/VERSION index 4f4873ddb1..efae06b4fc 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.2.0-dev.885 +3.2.0-dev.892 diff --git a/src/Attr.cc b/src/Attr.cc index a32cc89668..c464be6f54 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -40,6 +40,18 @@ Attr::Attr(AttrTag t) void Attr::SetAttrExpr(ExprPtr e) { expr = std::move(e); } +std::string Attr::DeprecationMessage() const + { + if ( tag != ATTR_DEPRECATED ) + return ""; + + if ( ! expr ) + return ""; + + auto ce = static_cast(expr.get()); + return ce->Value()->AsStringVal()->CheckString(); + } + void Attr::Describe(ODesc* d) const { AddTag(d); diff --git a/src/Attr.h b/src/Attr.h index 4439701f86..2c9d278a73 100644 --- a/src/Attr.h +++ b/src/Attr.h @@ -3,6 +3,7 @@ #pragma once #include +#include #include "Obj.h" #include "BroList.h" @@ -73,6 +74,12 @@ public: void Describe(ODesc* d) const override; void DescribeReST(ODesc* d, bool shorten = false) const; + /** + * Returns the deprecation string associated with a &deprecated attribute + * or an empty string if this is not such an attribute. + */ + std::string DeprecationMessage() const; + bool operator==(const Attr& other) const { if ( tag != other.tag ) diff --git a/src/ID.cc b/src/ID.cc index ba184ccaec..1b8c3fef58 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -294,14 +294,7 @@ std::string ID::GetDeprecationWarning() const const auto& depr_attr = GetAttr(ATTR_DEPRECATED); if ( depr_attr ) - { - auto expr = static_cast(depr_attr->GetExpr().get()); - if ( expr ) - { - StringVal* text = expr->Value()->AsStringVal(); - result = text->CheckString(); - } - } + result = depr_attr->DeprecationMessage(); if ( result.empty() ) return fmt("deprecated (%s)", Name()); diff --git a/src/Type.cc b/src/Type.cc index 6a833ddb9e..cb4b1d793f 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -563,7 +563,7 @@ FuncType::FuncType(RecordTypePtr arg_args, offsets[i] = i; } - prototypes.emplace_back(Prototype{false, args, std::move(offsets)}); + prototypes.emplace_back(Prototype{false, "", args, std::move(offsets)}); } TypePtr FuncType::ShallowClone() @@ -1120,14 +1120,7 @@ string RecordType::GetFieldDeprecationWarning(int field, bool has_check) const { string result; if ( const auto& deprecation = decl->GetAttr(zeek::detail::ATTR_DEPRECATED) ) - { - auto expr = static_cast(deprecation->GetExpr().get()); - if ( expr ) - { - StringVal* text = expr->Value()->AsStringVal(); - result = text->CheckString(); - } - } + result = deprecation->DeprecationMessage(); if ( result.empty() ) return fmt("deprecated (%s%s$%s)", GetName().c_str(), has_check ? "?" : "", diff --git a/src/Type.h b/src/Type.h index b5d58a633c..a28a32c45d 100644 --- a/src/Type.h +++ b/src/Type.h @@ -427,7 +427,10 @@ public: */ struct Prototype { bool deprecated; + std::string deprecation_msg; RecordTypePtr args; + // Maps from parameter index in canonical prototype to + // parameter index in this alternate prorotype. std::map offsets; }; diff --git a/src/Var.cc b/src/Var.cc index baa4c0dae7..16e474ea02 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -96,17 +96,27 @@ static bool add_prototype(const zeek::detail::IDPtr& id, zeek::Type* t, return false; } - offsets[i] = o; + offsets[o] = i; } auto deprecated = false; + std::string depr_msg; if ( attrs ) for ( const auto& a : *attrs ) if ( a->Tag() == zeek::detail::ATTR_DEPRECATED ) + { deprecated = true; + depr_msg = a->DeprecationMessage(); + break; + } + + zeek::FuncType::Prototype p; + p.deprecated = deprecated; + p.deprecation_msg = std::move(depr_msg); + p.args = alt_args; + p.offsets = std::move(offsets); - zeek::FuncType::Prototype p{deprecated, alt_args, std::move(offsets)}; canon_ft->AddPrototype(std::move(p)); return true; } @@ -450,7 +460,25 @@ static std::optional func_type_check(const zeek::Func return {}; } - return decl->FindPrototype(*impl->Params()); + auto rval = decl->FindPrototype(*impl->Params()); + + if ( rval ) + for ( auto i = 0; i < rval->args->NumFields(); ++i ) + if ( auto ad = rval->args->FieldDecl(i)->GetAttr(zeek::detail::ATTR_DEPRECATED) ) + { + auto msg = ad->DeprecationMessage(); + + if ( msg.empty() ) + impl->Warn(fmt("use of deprecated parameter '%s'", + rval->args->FieldName(i)), + decl, true); + else + impl->Warn(fmt("use of deprecated parameter '%s': %s", + rval->args->FieldName(i), msg.data()), + decl, true); + } + + return rval; } static bool canonical_arg_types_match(const zeek::FuncType* decl, const zeek::FuncType* impl) @@ -523,7 +551,15 @@ void begin_func(zeek::detail::IDPtr id, const char* module_name, } if ( prototype->deprecated ) - t->Warn("use of deprecated prototype", id.get()); + { + if ( prototype->deprecation_msg.empty() ) + t->Warn(fmt("use of deprecated '%s' prototype", id->Name()), + prototype->args.get(), true); + else + t->Warn(fmt("use of deprecated '%s' prototype: %s", + id->Name(), prototype->deprecation_msg.data()), + prototype->args.get(), true); + } } else { @@ -568,24 +604,54 @@ void begin_func(zeek::detail::IDPtr id, const char* module_name, else id->SetType(t); + const auto& args = t->Params(); + const auto& canon_args = id->GetType()->AsFuncType()->Params(); + zeek::detail::push_scope(std::move(id), std::move(attrs)); - const auto& args = t->Params(); - int num_args = args->NumFields(); - - for ( int i = 0; i < num_args; ++i ) + for ( int i = 0; i < canon_args->NumFields(); ++i ) { - zeek::TypeDecl* arg_i = args->FieldDecl(i); + zeek::TypeDecl* arg_i; + bool hide = false; + + if ( prototype ) + { + auto it = prototype->offsets.find(i); + + if ( it == prototype->offsets.end() ) + { + // Alternate prototype hides this param + hide = true; + arg_i = canon_args->FieldDecl(i); + } + else + { + // Alternate prototype maps this param to another index + arg_i = args->FieldDecl(it->second); + } + } + else + { + if ( i < args->NumFields() ) + arg_i = args->FieldDecl(i); + else + break; + } + auto arg_id = zeek::detail::lookup_ID(arg_i->id, module_name); if ( arg_id && ! arg_id->IsGlobal() ) arg_id->Error("argument name used twice"); - arg_id = zeek::detail::install_ID(arg_i->id, module_name, false, false); - arg_id->SetType(arg_i->type); + const char* local_name = arg_i->id; - if ( prototype ) - arg_id->SetOffset(prototype->offsets[i]); + if ( hide ) + // Note the illegal '-' in hidden name implies we haven't + // clobbered any local variable names. + local_name = fmt("%s-hidden", local_name); + + arg_id = zeek::detail::install_ID(local_name, module_name, false, false); + arg_id->SetType(arg_i->type); } if ( zeek::detail::Attr* depr_attr = find_attr(zeek::detail::current_scope()->Attrs().get(), diff --git a/testing/btest/Baseline/language.alternate-event-hook-prototypes/out b/testing/btest/Baseline/language.alternate-event-hook-prototypes/out index aa70ccc747..66d22d20e4 100644 --- a/testing/btest/Baseline/language.alternate-event-hook-prototypes/out +++ b/testing/btest/Baseline/language.alternate-event-hook-prototypes/out @@ -1,4 +1,4 @@ -warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 68 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 10: use of deprecated prototype (hook(c:count;) : bool and my_hook) +warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 68 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 13: use of deprecated 'my_hook' prototype (hook(c:count;) : bool) my_hook, infinite, 13 my_hook, 13, infinite my_hook, infinite diff --git a/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/hidden-error b/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/hidden-error new file mode 100644 index 0000000000..3e7b9120c4 --- /dev/null +++ b/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/hidden-error @@ -0,0 +1,3 @@ +warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 11 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 7: use of deprecated parameter 'b': Don't use 'b' (event(a:string; b:string; c:string;)) +warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 30 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 9: use of deprecated 'myev' prototype: Don't use this prototype (event(a:string; b:string;)) +error in ./hide.zeek, line 5: unknown identifier b, at or near "b" diff --git a/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/out b/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/out new file mode 100644 index 0000000000..b3686a0b23 --- /dev/null +++ b/testing/btest/Baseline/language.alternate-prototypes-deprecated-args/out @@ -0,0 +1,8 @@ +warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 11 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 7: use of deprecated parameter 'b': Don't use 'b' (event(a:string; b:string; c:string;)) +warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 30 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 9: use of deprecated 'myev' prototype: Don't use this prototype (event(a:string; b:string;)) +myev (canon), one, two, three +myev (new), one, three, [1, 2, 3] +myev (new), one, three, 0 +myev (new), one, three, 1 +myev (new), one, three, 2 +myev (old), one, two diff --git a/testing/btest/language/alternate-prototypes-deprecated-args.zeek b/testing/btest/language/alternate-prototypes-deprecated-args.zeek new file mode 100644 index 0000000000..d6c40425eb --- /dev/null +++ b/testing/btest/language/alternate-prototypes-deprecated-args.zeek @@ -0,0 +1,47 @@ +# @TEST-EXEC: zeek -b %INPUT >out 2>&1 +# +# @TEST-EXEC-FAIL: zeek -b %INPUT hide.zeek >hidden-error 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff hidden-error + +global myev: event(a: string, b: string &deprecated="Don't use 'b'", c: string); +global myev: event(a: string, c: string); +global myev: event(a: string, b: string) &deprecated="Don't use this prototype"; + +event myev(a: string, b: string, c: string) &priority=11 + { + print "myev (canon)", a, b, c; + } + +event myev(a: string, c: string) &priority = 7 + { + local ddd = vector(1,2,3); + print "myev (new)", a, c, ddd; + } + +global eee = vector(1,2,3); + +event myev(a: string, c: string) &priority = 6 + { + for ( o in eee ) + print "myev (new)", a, c, o; + } + +event myev(a: string, b: string) &priority = 5 + { + print "myev (old)", a, b; + } + +event zeek_init() + { + event myev("one", "two", "three"); + } + +@TEST-START-FILE hide.zeek +event myev(a: string, c: string) &priority = 7 + { + local ddd = vector(1,2,3); + print "myev (new)", a, c, ddd; + print b; + } +@TEST-END-FILE