Passing the `id_list` pointer to `BroFunc` transfers ownership of the
contained `ID` instances, because `~BroFunc()` unreferences them.
Therefore, we need to increase the reference counters for each
`BroFunc` instance to fix the use-after-free bug.
Closes https://github.com/zeek/zeek/issues/845
Only one instance of base_type() getting a NewRef instead of AdoptRef
fixed in merge. All other changes are superficial formatting and
factoring.
* 'leaks' of https://github.com/MaxKellermann/zeek: (22 commits)
Stmt: use class IntrusivePtr
Stmt: remove unused default constructors and `friend` declarations
Val: remove unimplemented prototype recover_val()
Val: cast_value_to_type() returns IntrusivePtr
Val: use IntrusivePtr in check_and_promote()
Val: use nullptr instead of 0
zeekygen: use class IntrusivePtr
ID: use class IntrusivePtr
Expr: use class IntrusivePtr
Var: copy Location to stack, to fix use-after-free crash bug
Scope: lookup_ID() and install_ID() return IntrusivePtr<ID>
Scope: delete duplicate locals
EventRegistry: automatically delete EventHandlers
main: destroy event_registry after iosource_mgr
zeekygen/IdentifierInfo: delete duplicate fields
main: free the global scope in terminate_bro()
Scope: pop_scope() returns IntrusivePtr<>
Scope: unref all inits in destructor
Var: pass IntrusivePtr to add_global(), add_local() etc.
plugin/ComponentManager: hold a reference to the EnumType
...
* origin/topic/timw/cleanup:
Expr: use fmt instead of sprintf
Expr: other minor initialization cleanup
Expr: use List::empty()
Expr: Convert a bunch of methods returning ints to return bools
IPAddr: minor cleanup
PriorityQueue: initialization cleanup
IP: Cleanup initialization, make a few functions consistent with others
- Various minor code formatting/styling during the merge
* 'leaks' of https://github.com/MaxKellermann/zeek:
parse.y: fix memory leak in FieldAssignExpr call
parse.y: fix use-after-free bug in open-ended index_slice
Type: fix use-after-free bug in init_type()
Expr: fix memory leak in RecordCoerceExpr::Fold()
Expr: fix memory leak in RecordCoerceExpr::InitVal()
zeekygen/IdentifierInfo: fix memory leak in operator=()
Func: fix memory leaks in get_func_priority()
parse.y: fix several memory leaks after lookup_ID()
Func: fix memory leaks in check_built_in_call()
Var: fix memory leaks in add_global() and add_local()
Var: add missing references to `init` in add{,_and_assign}_local()
parse.y: hold reference on init_expr for zeekygen::Manager::Redef()
Expr: fix two memory leaks in AssignExpr::InitVal()
parse.y: fix memory leak after "&derepcated" without string
RuleMatcher: delete PatternSet instances in destructor (memleak)
option.bif: fix crash bug by referencing `Func`, not `Val`
Minor whitespace fixes during merge.
* 'smart_ptr' of https://github.com/MaxKellermann/zeek:
OpaqueVal: remove misplaced `virtual` keywords
CompHash: use class IntrusivePtr for the `type` field
IntrusivePtr: replace the "add_ref" parameter with tag structs
IntrusivePtr: remove reset(), nobody uses it
IntrusivePtr: remove ordering operators
IntrusivePtr: rename detach() to release()
IntrusivePtr: move nullptr initializer to field declaration
Updated the Ref() to happen inline with Assign() call for clarity.
* 'expr_missing_ref' of https://github.com/MaxKellermann/zeek:
Expr: add missing reference in AssignExpr::InitVal()
The one reference returned by `op2->InitVal()` is given to
`aggr_r->Assign()` and returned to the caller, which may result in a
use-after-free crash bug. This patch adds the missing reference.
Closes https://github.com/zeek/zeek/issues/805
The merge commit fixes reference counting issues introduced with the
changes to ListExpr::Assign() and IndexExpr::Assign(), but then also
several other pre-existing reference counting confusions in other
Assign() implementations/calls, some which were now directly observable
via new crashing behavior, others just from a cursory code audit.
* 'memleaks' of https://github.com/MaxKellermann/zeek:
input/Manager: fix memory leak in UnrollRecordType()
OpaqueVal: fix two memory leaks in BloomFilterVal::Merge()
DbgBreakpoint: fix memory leak
DebugCmds: fix memory leak
scan.l: fix crash bug in do_atif()
Expr: fix memory leak in RecordConstructorExpr::InitVal()
Expr: fix memory leaks in AssignExpr::EvalIntoAggregate()
Expr: fix memory leaks in CondExpr::Eval()
Expr: fix several memory leaks in BoolExpr::Eval()
Expr: fix various memory leaks in Assign()
Expr: fix memory leaks in BinaryExpr::Eval()
analyzer/protocol/http: fix potential memory leak
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!
Only 1% build time speedup, but still, it declutters the headers a bit.
Before this patch:
2565.17user 141.83system 2:25.46elapsed 1860%CPU (0avgtext+0avgdata 1489076maxresident)k
72576inputs+9130920outputs (1667major+49400430minor)pagefaults 0swaps
After this patch:
2537.19user 142.94system 2:26.90elapsed 1824%CPU (0avgtext+0avgdata 1434268maxresident)k
16240inputs+8887152outputs (1931major+48728888minor)pagefaults 0swaps
The Zeek code base has very inconsistent #includes. Many sources
included a few headers, and those headers included other headers, and
in the end, nearly everything is included everywhere, so missing
#includes were never noticed. Another side effect was a lot of header
bloat which slows down the build.
First step to fix it: in each source file, its own header should be
included first to verify that each header's includes are correct, and
none is missing.
After adding the missing #includes, I replaced lots of #includes
inside headers with class forward declarations. In most headers,
object pointers are never referenced, so declaring the function
prototypes with forward-declared classes is just fine.
This patch speeds up the build by 19%, because each compilation unit
gets smaller. Here are the "time" numbers for a fresh build (with a
warm page cache but without ccache):
Before this patch:
3144.94user 161.63system 3:02.87elapsed 1808%CPU (0avgtext+0avgdata 2168608maxresident)k
760inputs+12008400outputs (1511major+57747204minor)pagefaults 0swaps
After this patch:
2565.17user 141.83system 2:25.46elapsed 1860%CPU (0avgtext+0avgdata 1489076maxresident)k
72576inputs+9130920outputs (1667major+49400430minor)pagefaults 0swaps
- Adds new trigger namespace
- Adds trigger::Manager class as a new IOSource for keeping track of triggers and integrating them into the loop. Previously the loop relied on the event manager Drain() method to process all triggers on every loop, but now that the loop actively waits for events to occur, triggers would not fire when they needed to. Adding them as part of the loop ensures they're checked.
For example, circular references between a lambda function the frame
it's stored within and/or its closure could cause memory leaks.
This also fixes other various reference-count ownership issues that
could lead to memory errors.
There may still be some potential/undiscovered issues because the "outer
ID" finding logic doesn't look quite right as the AST traversal descends
within nested lambdas and considers their locals as "outer", but
possibly the other logic for locating values in closures or cloning
closures just works around that behavior.
* origin/topic/timw/deprecate-int-types:
Deprecate the internal int/uint types in favor of the cstdint types they were based on
Merge adjustments:
* A bpf type mistakenly got replaced (inside an unlikely #ifdef)
* Did a few substitutions that got missed (likely due to
pre-processing out of DEBUG macros)
* origin/topic/timw/easy-pdict-replacements:
Cleanups related to PDict -> std::map replacements
Remove other simple uses of PDict
Protocols: Remove uses of PDict
g_dbgfilemaps: Remove uses of PDict
Scope: remove uses of PDict
DFA: remove uses of PDict
EventRegistry: remove uses of PDict
The results of std::hash<std::string> may vary depending on platform.
E.g. test suite failed on macOS due to Linux generating different lambda
function names.
