From b2072c4c5cd5becc5549a9df6789421bed7eece8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:32:31 +0100 Subject: [PATCH 01/16] option.bif: fix crash bug by referencing `Func`, not `Val` The method `ID::AddOptionHandler()` expects to adopt a reference to the `callback` parameter from the caller, but the caller references the containing `Val` instance, not the `Func`. Later, the `ID` destructor will `Unref()` the `Func`, which will quickly underflow the reference counter. The containing `Val` however will have references nobody will ever release (memory leak). --- src/option.bif | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/option.bif b/src/option.bif index 55e3c4a6cf..8c56fe4c38 100644 --- a/src/option.bif +++ b/src/option.bif @@ -205,6 +205,8 @@ function Option::set_change_handler%(ID: string, on_change: any, priority: int & return val_mgr->GetBool(0); } - i->AddOptionHandler(on_change->Ref()->AsFunc(), -priority); + auto* func = on_change->AsFunc(); + Ref(func); + i->AddOptionHandler(func, -priority); return val_mgr->GetBool(1); %} From 7f97f7420303dca8457d7894910a21c40c9cdb79 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:35:15 +0100 Subject: [PATCH 02/16] RuleMatcher: delete PatternSet instances in destructor (memleak) --- src/RuleMatcher.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/RuleMatcher.cc b/src/RuleMatcher.cc index 520d34109f..cc3e59448a 100644 --- a/src/RuleMatcher.cc +++ b/src/RuleMatcher.cc @@ -130,7 +130,10 @@ RuleHdrTest::~RuleHdrTest() for ( int i = 0; i < Rule::TYPES; ++i ) { for ( auto pset : psets[i] ) + { delete pset->re; + delete pset; + } } delete ruleset; From 32786c76457b85be4955023933ff6af9fe3ee1ac Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:37:52 +0100 Subject: [PATCH 03/16] parse.y: fix memory leak after "&derepcated" without string The "TOK_CONSTANT" leaks. --- src/parse.y | 1 + 1 file changed, 1 insertion(+) diff --git a/src/parse.y b/src/parse.y index 6106b04482..fe4910e250 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1363,6 +1363,7 @@ attr: { ODesc d; $3->Describe(&d); + Unref($3); reporter->Error("'&deprecated=%s' must use a string literal", d.Description()); $$ = new Attr(ATTR_DEPRECATED); From 3b09bb9e460a64229ede4f19ad61387a788ff021 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:39:12 +0100 Subject: [PATCH 04/16] Expr: fix two memory leaks in AssignExpr::InitVal() --- src/Expr.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Expr.cc b/src/Expr.cc index 2b4839c79a..64c4f47156 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2480,7 +2480,11 @@ Val* AssignExpr::InitVal(const BroType* t, Val* aggr) const Val* index = op1->InitVal(tt->Indices(), 0); Val* v = op2->InitVal(yt, 0); if ( ! index || ! v ) + { + Unref(index); + Unref(tv); return 0; + } if ( ! tv->ExpandAndInit(index, v) ) { From 2db1c88e4d69ebc47e54863b560c902a2791c766 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 12:32:26 +0100 Subject: [PATCH 05/16] parse.y: hold reference on init_expr for zeekygen::Manager::Redef() The reference is consumed by add_global(), which means we must not use it after the function returns. --- src/parse.y | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/parse.y b/src/parse.y index fe4910e250..9ebf4eda1d 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1099,8 +1099,11 @@ decl: | TOK_REDEF global_id opt_type init_class opt_init opt_attr ';' { + if ($5) + Ref($5); add_global($2, $3, $4, $5, $6, VAR_REDEF); zeekygen_mgr->Redef($2, ::filename, $4, $5); + Unref($5); } | TOK_REDEF TOK_ENUM global_id TOK_ADD_TO '{' From d49e709bfdee983bb9a188306f32c4b90b8019cc Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 12:54:10 +0100 Subject: [PATCH 06/16] Var: add missing references to `init` in add{,_and_assign}_local() The reference to `init` is consumed by make_var() and again by the AssignExpr constructor. --- src/Var.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Var.cc b/src/Var.cc index 971a09cfa9..1d917ad4aa 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -235,6 +235,8 @@ void add_global(ID* id, BroType* t, init_class c, Expr* init, Stmt* add_local(ID* id, BroType* t, init_class c, Expr* init, attr_list* attr, decl_type dt) { + if (init) + Ref(init); make_var(id, t, c, init, attr, dt, 0); if ( init ) @@ -262,6 +264,7 @@ Stmt* add_local(ID* id, BroType* t, init_class c, Expr* init, extern Expr* add_and_assign_local(ID* id, Expr* init, Val* val) { + Ref(init); make_var(id, 0, INIT_FULL, init, 0, VAR_REGULAR, 0); Ref(id); return new AssignExpr(new NameExpr(id), init, 0, val); From 6c263cbce51c8084add864e20805d14614784cb0 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:41:43 +0100 Subject: [PATCH 07/16] Var: fix memory leaks in add_global() and add_local() The `init` parameter always leaks. --- src/Var.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/Var.cc b/src/Var.cc index 1d917ad4aa..59d270b269 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -42,6 +42,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, else if ( dt != VAR_REDEF || init || ! attr ) { id->Error("already defined", init); + Unref(init); return; } } @@ -51,6 +52,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, if ( ! id->Type() ) { id->Error("\"redef\" used but not previously defined"); + Unref(init); return; } @@ -64,6 +66,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, (! init || ! do_init || (! t && ! (t = init_type(init)))) ) { id->Error("already defined", init); + Unref(init); return; } @@ -71,6 +74,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, if ( ! same_type(t, id->Type()) ) { id->Error("redefinition changes type", init); + Unref(init); return; } } @@ -85,6 +89,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, if ( init ) { id->Error("double initialization", init); + Unref(init); return; } @@ -98,6 +103,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, if ( ! init ) { id->Error("no type given"); + Unref(init); return; } @@ -105,6 +111,7 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, if ( ! t ) { id->SetType(error_type()); + Unref(init); return; } } @@ -185,7 +192,10 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, { v = init_val(init, t, aggr); if ( ! v ) + { + Unref(init); return; + } } if ( aggr ) @@ -211,6 +221,8 @@ static void make_var(ID* id, BroType* t, init_class c, Expr* init, id->SetOption(); } + Unref(init); + id->UpdateValAttrs(); if ( t && t->Tag() == TYPE_FUNC && From e32a9a61f6364fa58c9f7f1f7985652819f5ac80 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 18:52:12 +0100 Subject: [PATCH 08/16] Func: fix memory leaks in check_built_in_call() All error branches leak the `fmt_str_arg->Eval(0)` return value. --- src/Func.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Func.cc b/src/Func.cc index cd5062af9b..3b385b11f5 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -828,6 +828,7 @@ bool check_built_in_call(BuiltinFunc* f, CallExpr* call) if ( ! *fmt_str ) { + Unref(fmt_str_val); call->Error("format string ends with bare '%'"); return false; } @@ -839,11 +840,13 @@ bool check_built_in_call(BuiltinFunc* f, CallExpr* call) if ( args.length() != num_fmt + 1 ) { + Unref(fmt_str_val); call->Error("mismatch between format string to fmt() and number of arguments passed"); return false; } } + Unref(fmt_str_val); return true; } From 80d6bbc4dc549ae034bbee4b4dd55be6160507e4 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 19:05:45 +0100 Subject: [PATCH 09/16] parse.y: fix several memory leaks after lookup_ID() lookup_ID() returns a referenced pointer to the caller. Quite a lot of code paths don't release those references. --- src/parse.y | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/parse.y b/src/parse.y index 9ebf4eda1d..7938426433 100644 --- a/src/parse.y +++ b/src/parse.y @@ -688,6 +688,7 @@ expr: { id->Error("undeclared variable"); id->SetType(error_type()); + Ref(id); $$ = new NameExpr(id); } @@ -701,10 +702,14 @@ expr: $$ = new ConstExpr(t->GetVal(intval)); } else + { + Ref(id); $$ = new NameExpr(id); + } if ( id->IsDeprecated() ) reporter->Warning("%s", id->GetDeprecationWarning().c_str()); + Unref(id); } } @@ -1581,6 +1586,7 @@ event: } if ( id->IsDeprecated() ) reporter->Warning("%s", id->GetDeprecationWarning().c_str()); + Unref(id); } $$ = new EventExpr($1, $3); @@ -1632,7 +1638,10 @@ case_type: if ( case_var && case_var->IsGlobal() ) case_var->Error("already a global identifier"); else + { + Unref(case_var); case_var = install_ID(name, current_module.c_str(), false, false); + } add_local(case_var, type, INIT_NONE, 0, 0, VAR_REGULAR); $$ = case_var; @@ -1656,8 +1665,11 @@ for_head: } else + { + Unref(loop_var); loop_var = install_ID($3, current_module.c_str(), false, false); + } id_list* loop_vars = new id_list; loop_vars->push_back(loop_var); From b84693e5463eb01b3be011960a953dfb0da920f7 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 21 Feb 2020 21:08:44 +0100 Subject: [PATCH 10/16] Func: fix memory leaks in get_func_priority() --- src/Func.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Func.cc b/src/Func.cc index 3b385b11f5..639d69f685 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -876,11 +876,13 @@ static int get_func_priority(const attr_list& attrs) if ( ! IsIntegral(v->Type()->Tag()) ) { + Unref(v); a->Error("expression is not of integral type"); continue; } priority = v->InternalInt(); + Unref(v); } return priority; From c074122f1364c23fe4b370a76dc57a72e90384fb Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 12:26:37 +0100 Subject: [PATCH 11/16] zeekygen/IdentifierInfo: fix memory leak in operator=() --- src/zeekygen/IdentifierInfo.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/zeekygen/IdentifierInfo.cc b/src/zeekygen/IdentifierInfo.cc index 5ff5b5dfa3..36f5036f73 100644 --- a/src/zeekygen/IdentifierInfo.cc +++ b/src/zeekygen/IdentifierInfo.cc @@ -165,6 +165,8 @@ IdentifierInfo::Redefinition::operator=(const IdentifierInfo::Redefinition& othe if ( &other == this ) return *this; + Unref(init_expr); + from_script = other.from_script; ic = other.ic; init_expr = other.init_expr; From 46ecbd400e07d6139586314035d1d741956bfe40 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 13:37:51 +0100 Subject: [PATCH 12/16] Expr: fix memory leak in RecordCoerceExpr::InitVal() Caused by commit b1fd1612740d554bf6681888e6920e982ceb5200 --- src/Expr.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 64c4f47156..ec62024553 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3776,12 +3776,10 @@ Val* RecordCoerceExpr::InitVal(const BroType* t, Val* aggr) const { RecordVal* rv = v->AsRecordVal(); RecordVal* ar = rv->CoerceTo(t->AsRecordType(), aggr); + Unref(rv); if ( ar ) - { - Unref(rv); return ar; - } } Error("bad record initializer"); From e557563c69a4be22a71d4402feae8dc942fbb739 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 13:42:25 +0100 Subject: [PATCH 13/16] Expr: fix memory leak in RecordCoerceExpr::Fold() Don't add a second reference if the `rhs` variable was assigned from `def->AttrExpr()->Eval(0)`. Caused by commit af3267acc3fa7b5e7b933bf8de7124202853bfc8 --- src/Expr.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index ec62024553..4463df0432 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3804,8 +3804,7 @@ Val* RecordCoerceExpr::Fold(Val* v) const if ( def ) rhs = def->AttrExpr()->Eval(0); } - - if ( rhs ) + else rhs = rhs->Ref(); assert(rhs || Type()->AsRecordType()->FieldDecl(i)->FindAttr(ATTR_OPTIONAL)); From bcac5d3b17dc218271d4cd704807b270ae6cffaa Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 14:28:31 +0100 Subject: [PATCH 14/16] Type: fix use-after-free bug in init_type() Prior to this, `t` gets assigned from `Expr::InitType()` which returns a referenced `BroType` to the caller (and init_Type() releases the reference later). But `reduce_type()` does not return a referenced `BroType`; so if `reduce_type()` happens to return a different instance, it will be released and maybe destroyed, resulting in a use-after-free bug. --- src/Type.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/Type.cc b/src/Type.cc index 88fffa4724..7bbdbfa6e4 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -2100,7 +2100,19 @@ BroType* init_type(Expr* init) BroType* t = e0->InitType(); if ( t ) + { + BroType *old_t = t; t = reduce_type(t); + /* reduce_type() does not return a referenced pointer, + but we want to own a reference, so create one + here */ + if (t) + Ref(t); + /* reduce_type() does not adopt our reference passed + as parameter, so we need to release it (after the + Ref() call above) */ + Unref(old_t); + } if ( ! t ) return 0; From 68b965c29904702d92a49661d3c7ad54219f8a1f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 15:51:44 +0100 Subject: [PATCH 15/16] parse.y: fix use-after-free bug in open-ended index_slice This code passed the "$1" reference to both `SizeExpr::SizeExpr()` and `IndexExpr::IndexExpr()`. Bug introduced by commit 8515d3aa57254c95a1afad4fc64324b1f2a2755f --- src/parse.y | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parse.y b/src/parse.y index 7938426433..7ac4049943 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1304,7 +1304,7 @@ index_slice: { set_location(@1, @6); Expr* low = $3 ? $3 : new ConstExpr(val_mgr->GetCount(0)); - Expr* high = $5 ? $5 : new SizeExpr($1); + Expr* high = $5 ? $5 : new SizeExpr($1->Ref()); if ( ! IsIntegral(low->Type()->Tag()) || ! IsIntegral(high->Type()->Tag()) ) reporter->Error("slice notation must have integral values as indexes"); From 12b6070b2b7a7f5d29d8c4c728bf76ffedfe095e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 24 Feb 2020 19:50:56 +0100 Subject: [PATCH 16/16] parse.y: fix memory leak in FieldAssignExpr call An unmanaged reference is held on `func_id`, but its `val` reference is now owned by the `FieldAssignExpr` instance. The `ID` instance still feels responsible for releasing the `val` reference, but since nobody ever frees the `ID`, the conflict never causes a crash. --- src/parse.y | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/parse.y b/src/parse.y index 7ac4049943..8a07bdf40f 100644 --- a/src/parse.y +++ b/src/parse.y @@ -514,7 +514,8 @@ expr: } func_body { - $$ = new FieldAssignExpr($2, new ConstExpr(func_id->ID_Val())); + $$ = new FieldAssignExpr($2, new ConstExpr(func_id->ID_Val()->Ref())); + Unref(func_id); } | expr TOK_IN expr