From 36d4b25ac03dfe909f0b470f8b8542e4b8972960 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Mon, 10 Feb 2020 23:01:44 +0100 Subject: [PATCH 01/12] analyzer/protocol/http: fix potential memory leak This isn't really a memory leak because ParseRequest() never fails, but if it one day "learns" to fail, the `request_method` allocation will leak. --- src/analyzer/protocol/http/HTTP.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index 56359a177e..d11308e5a6 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -1264,14 +1264,14 @@ int HTTP_Analyzer::HTTP_RequestLine(const char* line, const char* end_of_line) if ( rest == end_of_method ) goto error; - request_method = new StringVal(end_of_method - line, line); - if ( ! ParseRequest(rest, end_of_line) ) { reporter->AnalyzerError(this, "HTTP ParseRequest failed"); return -1; } + request_method = new StringVal(end_of_method - line, line); + Conn()->Match(Rule::HTTP_REQUEST, (const u_char*) unescaped_URI->AsString()->Bytes(), unescaped_URI->AsString()->Len(), true, true, true, true); From 593ebc1d629105dea652e44cc6b47db773724f72 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:59:12 +0100 Subject: [PATCH 02/12] Expr: fix memory leaks in BinaryExpr::Eval() --- src/Expr.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Expr.cc b/src/Expr.cc index 9aff73cdb1..6fdcad8f0d 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -494,6 +494,8 @@ Val* BinaryExpr::Eval(Frame* f) const if ( v_op1->Size() != v_op2->Size() ) { + Unref(v1); + Unref(v2); RuntimeError("vector operands are of different sizes"); return 0; } From 6ce1081b38dc4cdc982a78c8931ab97e403b9d79 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 11:15:16 +0100 Subject: [PATCH 03/12] Expr: fix various memory leaks in Assign() --- src/Expr.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Expr.cc b/src/Expr.cc index 6fdcad8f0d..2aab130e13 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -101,8 +101,9 @@ void Expr::EvalIntoAggregate(const BroType* /* t */, Val* /* aggr */, Internal("Expr::EvalIntoAggregate called"); } -void Expr::Assign(Frame* /* f */, Val* /* v */) +void Expr::Assign(Frame* /* f */, Val* v) { + Unref(v); Internal("Expr::Assign called"); } @@ -2787,11 +2788,17 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const void IndexExpr::Assign(Frame* f, Val* v) { if ( IsError() ) + { + Unref(v); return; + } Val* v1 = op1->Eval(f); if ( ! v1 ) + { + Unref(v); return; + } Val* v2 = op2->Eval(f); @@ -2799,6 +2806,7 @@ void IndexExpr::Assign(Frame* f, Val* v) { Unref(v1); Unref(v2); + Unref(v); return; } @@ -2873,6 +2881,7 @@ void IndexExpr::Assign(Frame* f, Val* v) Unref(v1); Unref(v2); + Unref(v); } void IndexExpr::ExprDescribe(ODesc* d) const @@ -2949,7 +2958,10 @@ int FieldExpr::CanDel() const void FieldExpr::Assign(Frame* f, Val* v) { if ( IsError() ) + { + Unref(v); return; + } Val* op_v = op->Eval(f); if ( op_v ) @@ -2958,6 +2970,8 @@ void FieldExpr::Assign(Frame* f, Val* v) r->Assign(field, v); Unref(r); } + else + Unref(v); } void FieldExpr::Delete(Frame* f) @@ -4889,6 +4903,7 @@ void ListExpr::Assign(Frame* f, Val* v) exprs[i]->Assign(f, (*lv->Vals())[i]->Ref()); Unref(lv); + Unref(v); } TraversalCode ListExpr::Traverse(TraversalCallback* cb) const From 862f48da45e6543f3e0efbd9200d1bd1fc3e83d7 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 11:56:26 +0100 Subject: [PATCH 04/12] Expr: fix several memory leaks in BoolExpr::Eval() --- src/Expr.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Expr.cc b/src/Expr.cc index 2aab130e13..94e7076564 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -1631,7 +1631,10 @@ Val* BoolExpr::Eval(Frame* f) const } if ( ! scalar_v || ! vector_v ) + { + Unref(v1); return 0; + } VectorVal* result = 0; @@ -1657,13 +1660,18 @@ Val* BoolExpr::Eval(Frame* f) const // Only case remaining: both are vectors. Val* v2 = op2->Eval(f); if ( ! v2 ) + { + Unref(v1); return 0; + } VectorVal* vec_v1 = v1->AsVectorVal(); VectorVal* vec_v2 = v2->AsVectorVal(); if ( vec_v1->Size() != vec_v2->Size() ) { + Unref(v1); + Unref(v2); RuntimeError("vector operands have different sizes"); return 0; } From 65c4f34385cd316a59befe766f1f5fcec051d0dd Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 12:11:48 +0100 Subject: [PATCH 05/12] Expr: fix memory leaks in CondExpr::Eval() No code path had any cleanup code, leaking all the local references. More weird was however the result building code: it took elements from one of the existing vectors without referencing them, and passed them to VectorVal::Assign() which assumes that the caller-owned reference is now owned by that VectorVal. Even in the successful code path, no references were freed. Everything was wrong with this method! --- src/Expr.cc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 94e7076564..235fa15593 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -1995,11 +1995,18 @@ Val* CondExpr::Eval(Frame* f) const Val* v2 = op2->Eval(f); if ( ! v2 ) + { + Unref(v1); return 0; + } Val* v3 = op3->Eval(f); if ( ! v3 ) + { + Unref(v1); + Unref(v2); return 0; + } VectorVal* cond = v1->AsVectorVal(); VectorVal* a = v2->AsVectorVal(); @@ -2007,6 +2014,9 @@ Val* CondExpr::Eval(Frame* f) const if ( cond->Size() != a->Size() || a->Size() != b->Size() ) { + Unref(v1); + Unref(v2); + Unref(v3); RuntimeError("vectors in conditional expression have different sizes"); return 0; } @@ -2018,13 +2028,19 @@ Val* CondExpr::Eval(Frame* f) const { Val* local_cond = cond->Lookup(i); if ( local_cond ) - result->Assign(i, - local_cond->IsZero() ? - b->Lookup(i) : a->Lookup(i)); + { + Val *v = local_cond->IsZero() ? b->Lookup(i) : a->Lookup(i); + ::Ref(v); + result->Assign(i, v); + } else result->Assign(i, 0); } + Unref(v1); + Unref(v2); + Unref(v3); + return result; } From 5468fae8e0d02016593eb9e7568a09232a8e3c38 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 13:04:48 +0100 Subject: [PATCH 06/12] Expr: fix memory leaks in AssignExpr::EvalIntoAggregate() --- src/Expr.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Expr.cc b/src/Expr.cc index 235fa15593..9fd5856d01 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2404,7 +2404,11 @@ void AssignExpr::EvalIntoAggregate(const BroType* t, Val* aggr, Frame* f) const Val* index = op1->Eval(f); Val* v = check_and_promote(op2->Eval(f), t->YieldType(), 1); if ( ! index || ! v ) + { + Unref(index); + Unref(v); return; + } if ( ! tv->Assign(index, v) ) RuntimeError("type clash in table assignment"); From 0a0884edb433db295ef3ea44a2d64604b5d72d9b Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 13:13:43 +0100 Subject: [PATCH 07/12] Expr: fix memory leak in RecordConstructorExpr::InitVal() --- src/Expr.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 9fd5856d01..7d3794f04e 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3142,12 +3142,10 @@ Val* RecordConstructorExpr::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 bb1390caaa8e656fecefc2df824ac97e0795f6dd Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:19:23 +0100 Subject: [PATCH 08/12] scan.l: fix crash bug in do_atif() This is really a memory leak because the Unref() call is missing. But since this usually returns a "stock" object (`ValManager::b_true` or `ValManager::b_false`), nothing really leaks. But eventually, the reference counter will overflow to `INT_MAX`, leading to a crash in bad_ref(). --- src/scan.l | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/scan.l b/src/scan.l index ccc501860c..d41bcb601a 100644 --- a/src/scan.l +++ b/src/scan.l @@ -762,6 +762,8 @@ void do_atif(Expr* expr) if_stack.push_back(current_depth); BEGIN(IGNORE); } + + Unref(val); } void do_atifdef(const char* id) From 37e7e914dcf6263a1c90b0beddd2db09432b84e6 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:25:58 +0100 Subject: [PATCH 09/12] DebugCmds: fix memory leak --- src/DebugCmds.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/DebugCmds.cc b/src/DebugCmds.cc index 255d36f060..cc7d37bcb2 100644 --- a/src/DebugCmds.cc +++ b/src/DebugCmds.cc @@ -570,6 +570,7 @@ int dbg_cmd_print(DebugCmd cmd, const vector& args) { ODesc d; val->Describe(&d); + Unref(val); debug_msg("%s\n", d.Description()); } else From 19fd51a35b6be40fb8784791612fd2f589583ea9 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:22:51 +0100 Subject: [PATCH 10/12] DbgBreakpoint: fix memory leak --- src/DbgBreakpoint.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/DbgBreakpoint.cc b/src/DbgBreakpoint.cc index a1ef53d6ec..e26a8bb050 100644 --- a/src/DbgBreakpoint.cc +++ b/src/DbgBreakpoint.cc @@ -260,6 +260,7 @@ BreakCode DbgBreakpoint::HasHit() if ( ! IsIntegral(yes->Type()->Tag()) && ! IsBool(yes->Type()->Tag()) ) { + Unref(yes); PrintHitMsg(); debug_msg("Breakpoint condition should return an integral type"); return bcHitAndDelete; @@ -267,7 +268,12 @@ BreakCode DbgBreakpoint::HasHit() yes->CoerceToInt(); if ( yes->IsZero() ) + { + Unref(yes); return bcNoHit; + } + + Unref(yes); } int repcount = GetRepeatCount(); From 36127131159e87c91fe053767f190c49abef056e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:13:11 +0100 Subject: [PATCH 11/12] OpaqueVal: fix two memory leaks in BloomFilterVal::Merge() --- src/OpaqueVal.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpaqueVal.cc b/src/OpaqueVal.cc index d289d452f6..460fea1945 100644 --- a/src/OpaqueVal.cc +++ b/src/OpaqueVal.cc @@ -855,6 +855,7 @@ BloomFilterVal* BloomFilterVal::Merge(const BloomFilterVal* x, if ( ! copy->Merge(y->bloom_filter) ) { + delete copy; reporter->Error("failed to merge Bloom filter"); return 0; } @@ -863,6 +864,7 @@ BloomFilterVal* BloomFilterVal::Merge(const BloomFilterVal* x, if ( x->Type() && ! merged->Typify(x->Type()) ) { + Unref(merged); reporter->Error("failed to set type on merged Bloom filter"); return 0; } From 51970c256b159a2bb16eb70b3200878533bf2bf9 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 19 Feb 2020 10:08:39 +0100 Subject: [PATCH 12/12] input/Manager: fix memory leak in UnrollRecordType() The `Val*` return by Expr::Eval() is never freed. Note that it must be freed after the `Field` object has been created, because the `secondary` variable points to data inside the `Val*`. --- src/input/Manager.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/input/Manager.cc b/src/input/Manager.cc index ac2e6fb7cf..c5e56f3830 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -973,6 +973,7 @@ bool Manager::UnrollRecordType(vector *fields, const RecordType *rec, { string name = nameprepend + rec->FieldName(i); const char* secondary = 0; + Val* c = nullptr; TypeTag ty = rec->FieldType(i)->Tag(); TypeTag st = TYPE_VOID; bool optional = false; @@ -988,7 +989,7 @@ bool Manager::UnrollRecordType(vector *fields, const RecordType *rec, { // we have an annotation for the second column - Val* c = rec->FieldDecl(i)->FindAttr(ATTR_TYPE_COLUMN)->AttrExpr()->Eval(0); + c = rec->FieldDecl(i)->FindAttr(ATTR_TYPE_COLUMN)->AttrExpr()->Eval(0); assert(c); assert(c->Type()->Tag() == TYPE_STRING); @@ -1000,6 +1001,7 @@ bool Manager::UnrollRecordType(vector *fields, const RecordType *rec, optional = true; Field* field = new Field(name.c_str(), secondary, ty, st, optional); + Unref(c); fields->push_back(field); } }