diff --git a/CHANGES b/CHANGES index 225fb4cd88..f42b3a8d33 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,36 @@ +3.2.0-dev.69 | 2020-02-19 18:40:58 -0800 + + * Fix various reference counting issues in Assign() implementations/callers (Jon Siwek, Corelight) + + * Fix memory leak in Input stream creation when using &type_column (Max Kellermann) + + * Fix two memory leaks in BloomFilterVal::Merge() (Max Kellermann) + + * Fix memory leaks in script debugger (Max Kellermann) + + * scan.l: fix missing Unref() in do_atif() (Max Kellermann) + + * Fix parse-time memory leak in RecordConstructorExpr::InitVal() (Max Kellermann) + + * Fix memory leaks in AssignExpr::EvalIntoAggregate() error conditions (Max Kellermann) + + * Fix memory leaks in CondExpr::Eval() error conditions (Max Kellermann) + + Also fixes reference counting issue for vector-based conditionals. + + * Fix memory leaks in BoolExpr::Eval() error conditions (Max Kellermann) + + * Fix various memory leaks in Assign() error conditions (Max Kellermann) + + * Fix memory leaks in BinaryExpr::Eval() error conditions (Max Kellermann) + + * Fix potential future memory leak in HTTP analyzer (Max Kellermann) + + 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. + 3.2.0-dev.53 | 2020-02-18 12:12:28 -0800 * Make DNS NSEC3 parsing more resilient to introducing a memory leak diff --git a/VERSION b/VERSION index a47090db7c..0db350991e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.2.0-dev.53 +3.2.0-dev.69 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(); 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 diff --git a/src/Expr.cc b/src/Expr.cc index 9aff73cdb1..6170c1e495 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -18,6 +18,7 @@ #include "digest.h" #include "module_util.h" #include "DebugLogger.h" +#include "IntrusivePtr.h" #include "broker/Data.h" @@ -101,8 +102,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"); } @@ -494,6 +496,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; } @@ -1036,17 +1040,17 @@ Val* IncrExpr::Eval(Frame* f) const else v_vec->Assign(i, 0); } - op->Assign(f, v_vec); + op->Assign(f, v_vec->Ref()); + return v; } else { - Val* old_v = v; - op->Assign(f, v = DoSingleEval(f, old_v)); - Unref(old_v); + auto new_v = DoSingleEval(f, v); + Unref(v); + op->Assign(f, new_v->Ref()); + return new_v; } - - return v->Ref(); } int IncrExpr::IsPure() const @@ -1312,8 +1316,8 @@ Val* AddToExpr::Eval(Frame* f) const if ( result ) { - op1->Assign(f, result); - return result->Ref(); + op1->Assign(f, result->Ref()); + return result; } else return 0; @@ -1407,8 +1411,8 @@ Val* RemoveFromExpr::Eval(Frame* f) const if ( result ) { - op1->Assign(f, result); - return result->Ref(); + op1->Assign(f, result->Ref()); + return result; } else return 0; @@ -1628,7 +1632,10 @@ Val* BoolExpr::Eval(Frame* f) const } if ( ! scalar_v || ! vector_v ) + { + Unref(v1); return 0; + } VectorVal* result = 0; @@ -1654,13 +1661,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; } @@ -1984,11 +1996,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(); @@ -1996,6 +2015,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; } @@ -2007,13 +2029,18 @@ 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); + result->Assign(i, v ? v->Ref() : nullptr); + } else result->Assign(i, 0); } + Unref(v1); + Unref(v2); + Unref(v3); + return result; } @@ -2315,8 +2342,15 @@ Val* AssignExpr::Eval(Frame* f) const if ( v ) { - op1->Assign(f, v); - return val ? val->Ref() : v->Ref(); + op1->Assign(f, v->Ref()); + + if ( val ) + { + Unref(v); + return val->Ref(); + } + + return v; } else return 0; @@ -2377,7 +2411,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"); @@ -2500,10 +2538,7 @@ Val* IndexSliceAssignExpr::Eval(Frame* f) const Val* v = op2->Eval(f); if ( v ) - { op1->Assign(f, v); - Unref(v); - } return 0; } @@ -2666,7 +2701,10 @@ Val* IndexExpr::Eval(Frame* f) const for ( unsigned int i = 0; i < v_v2->Size(); ++i ) { if ( v_v2->Lookup(i)->AsBool() ) - v_result->Assign(v_result->Size() + 1, v_v1->Lookup(i)); + { + auto a = v_v1->Lookup(i); + v_result->Assign(v_result->Size() + 1, a ? a->Ref() : nullptr); + } } } else @@ -2676,7 +2714,10 @@ Val* IndexExpr::Eval(Frame* f) const // Probably only do this if *all* are negative. v_result->Resize(v_v2->Size()); for ( unsigned int i = 0; i < v_v2->Size(); ++i ) - v_result->Assign(i, v_v1->Lookup(v_v2->Lookup(i)->CoerceToInt())); + { + auto a = v_v1->Lookup(v_v2->Lookup(i)->CoerceToInt()); + v_result->Assign(i, a ? a->Ref() : nullptr); + } } } else @@ -2726,7 +2767,10 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const result->Resize(sub_length); for ( int idx = first; idx < last; idx++ ) - result->Assign(idx - first, vect->Lookup(idx)->Ref()); + { + auto a = vect->Lookup(idx); + result->Assign(idx - first, a ? a->Ref() : nullptr); + } } return result; @@ -2782,8 +2826,10 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const return 0; } -void IndexExpr::Assign(Frame* f, Val* v) +void IndexExpr::Assign(Frame* f, Val* arg_v) { + IntrusivePtr v{arg_v, false}; + if ( IsError() ) return; @@ -2800,6 +2846,11 @@ void IndexExpr::Assign(Frame* f, Val* v) return; } + // Hold an extra reference to 'arg_v' in case the ownership transfer to + // the table/vector goes wrong and we still want to obtain diagnostic info + // from the original value after the assignment already unref'd. + IntrusivePtr v_extra{arg_v, true}; + switch ( v1->Type()->Tag() ) { case TYPE_VECTOR: { @@ -2822,8 +2873,10 @@ void IndexExpr::Assign(Frame* f, Val* v) for ( auto idx = 0u; idx < v_vect->Size(); idx++, first++ ) v1_vect->Insert(first, v_vect->Lookup(idx)->Ref()); } - else if ( ! v1_vect->Assign(v2, v) ) + else if ( ! v1_vect->Assign(v2, v.detach()) ) { + v = v_extra; + if ( v ) { ODesc d; @@ -2842,8 +2895,10 @@ void IndexExpr::Assign(Frame* f, Val* v) } case TYPE_TABLE: - if ( ! v1->AsTableVal()->Assign(v2, v) ) + if ( ! v1->AsTableVal()->Assign(v2, v.detach()) ) { + v = v_extra; + if ( v ) { ODesc d; @@ -2947,7 +3002,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 ) @@ -2956,6 +3014,8 @@ void FieldExpr::Assign(Frame* f, Val* v) r->Assign(field, v); Unref(r); } + else + Unref(v); } void FieldExpr::Delete(Frame* f) @@ -3098,12 +3158,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"); @@ -4886,7 +4944,7 @@ void ListExpr::Assign(Frame* f, Val* v) loop_over_list(exprs, i) exprs[i]->Assign(f, (*lv->Vals())[i]->Ref()); - Unref(lv); + Unref(v); } TraversalCode ListExpr::Traverse(TraversalCallback* cb) const 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; } 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); 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); } } 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)