diff --git a/CHANGES b/CHANGES index 44f3368797..31ef36dd72 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,15 @@ +4.1.0-dev.537 | 2021-04-20 14:34:48 -0700 + + * Restore support for vectors with holes and improve test cases (Vern Paxson, Corelight) + + There's no longer breaking behavior changes to how vector-holes work from + previous Zeek versions. + + * Fix cloning/copying vectors that contain holes (Vern Paxson, Corelight) + + * fix using ++/-- to vectors that contain holes (Vern Paxson, Corelight) + 4.1.0-dev.526 | 2021-04-16 16:03:06 -0700 * GH-1506: Fix Broker unserialization of set/table function indices (Jon Siwek, Corelight) diff --git a/NEWS b/NEWS index fc08c2ae48..58d705863c 100644 --- a/NEWS +++ b/NEWS @@ -52,19 +52,6 @@ New Functionality Changed Functionality --------------------- -- The ``in`` operator does not any more track whether a given vector element - has been assigned-to. It now simply equates to "is this index between zero - and the size-of-the-vector-minus-one, inclusive". Previously, the ``in`` - operator could have detected whether, by purpose or accident, a "hole" was - created in a vector by way of assigning to an index that's more than 1 beyond - the end of the vector (with intermediate elements becoming "holes") and - whether a "hole" had ever been subsequently filled by an assignment. - - The behavior of creating a "hole" in a vector was never explicitly - documented, and should be thought of as undefined behavior and not something - the ``in`` operator can be used to detect. For this kind of usage, a - ``table[count] of X`` is more appropriate than a vector. - Removed Functionality --------------------- diff --git a/VERSION b/VERSION index 9e1efce972..0ece132839 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -4.1.0-dev.526 +4.1.0-dev.537 diff --git a/src/Expr.cc b/src/Expr.cc index b5ac3d56d5..56f0420b05 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -1239,7 +1239,8 @@ ValPtr IncrExpr::Eval(Frame* f) const for ( unsigned int i = 0; i < v_vec->Size(); ++i ) { auto elt = v_vec->ValAt(i); - v_vec->Assign(i, DoSingleEval(f, elt.get())); + if ( elt ) + v_vec->Assign(i, DoSingleEval(f, elt.get())); } op->Assign(f, std::move(v_vec)); diff --git a/src/Val.cc b/src/Val.cc index 600dee1c41..f4842003c1 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -3246,7 +3246,7 @@ ValPtr TypeVal::DoClone(CloneState* state) VectorVal::VectorVal(VectorTypePtr t) : Val(t) { - vector_val = new vector(); + vector_val = new vector>(); yield_type = t->Yield(); auto y_tag = yield_type->Tag(); @@ -3260,14 +3260,19 @@ VectorVal::~VectorVal() { int n = yield_types->size(); for ( auto i = 0; i < n; ++i ) - ZVal::DeleteIfManaged((*vector_val)[i], (*yield_types)[i]); + { + auto& elem = (*vector_val)[i]; + if ( elem ) + ZVal::DeleteIfManaged(*elem, (*yield_types)[i]); + } delete yield_types; } else if ( managed_yield ) { for ( auto& elem : *vector_val ) - ZVal::DeleteManagedType(elem); + if ( elem ) + ZVal::DeleteManagedType(*elem); } delete vector_val; @@ -3325,7 +3330,9 @@ bool VectorVal::Assign(unsigned int index, ValPtr element) if ( index >= n ) { - AddHoles(index - n); + if ( index > n ) + AddHoles(index - n); + vector_val->resize(index + 1); if ( yield_types ) yield_types->resize(index + 1); @@ -3335,14 +3342,21 @@ bool VectorVal::Assign(unsigned int index, ValPtr element) { const auto& t = element->GetType(); (*yield_types)[index] = t; - ZVal::DeleteIfManaged((*vector_val)[index], t); - (*vector_val)[index] = ZVal(std::move(element), t); + auto& elem = (*vector_val)[index]; + if ( elem ) + ZVal::DeleteIfManaged(*elem, t); + elem = ZVal(std::move(element), t); } else { - if ( managed_yield ) - ZVal::DeleteManagedType((*vector_val)[index]); - (*vector_val)[index] = ZVal(std::move(element), yield_type); + auto& elem = (*vector_val)[index]; + if ( managed_yield && elem ) + ZVal::DeleteManagedType(*elem); + + if ( element ) + elem = ZVal(std::move(element), yield_type); + else + elem = std::nullopt; } Modified(); @@ -3366,7 +3380,7 @@ bool VectorVal::Insert(unsigned int index, ValPtr element) if ( ! CheckElementType(element) ) return false; - vector::iterator it; + vector>::iterator it; vector::iterator types_it; auto n = vector_val->size(); @@ -3376,28 +3390,39 @@ bool VectorVal::Insert(unsigned int index, ValPtr element) it = std::next(vector_val->begin(), index); if ( yield_types ) { - ZVal::DeleteIfManaged(*it, element->GetType()); + if ( *it ) + ZVal::DeleteIfManaged(**it, element->GetType()); types_it = std::next(yield_types->begin(), index); } else if ( managed_yield ) - ZVal::DeleteManagedType(*it); + { + if ( *it ) + ZVal::DeleteManagedType(**it); + } } else { it = vector_val->end(); if ( yield_types ) types_it = yield_types->end(); - AddHoles(index - n); + + if ( index > n ) + AddHoles(index - n); } - if ( yield_types ) + if ( element ) { - const auto& t = element->GetType(); - yield_types->insert(types_it, t); - vector_val->insert(it, ZVal(std::move(element), t)); + if ( yield_types ) + { + const auto& t = element->GetType(); + yield_types->insert(types_it, t); + vector_val->insert(it, ZVal(std::move(element), t)); + } + else + vector_val->insert(it, ZVal(std::move(element), yield_type)); } else - vector_val->insert(it, ZVal(std::move(element), yield_type)); + vector_val->insert(it, std::nullopt); Modified(); return true; @@ -3410,7 +3435,7 @@ void VectorVal::AddHoles(int nholes) fill_t = base_type(TYPE_ANY); for ( auto i = 0; i < nholes; ++i ) - vector_val->emplace_back(ZVal(fill_t)); + vector_val->emplace_back(std::nullopt); } bool VectorVal::Remove(unsigned int index) @@ -3423,12 +3448,16 @@ bool VectorVal::Remove(unsigned int index) if ( yield_types ) { auto types_it = std::next(yield_types->begin(), index); - ZVal::DeleteIfManaged(*it, *types_it); + if ( *it ) + ZVal::DeleteIfManaged(**it, *types_it); yield_types->erase(types_it); } else if ( managed_yield ) - ZVal::DeleteManagedType(*it); + { + if ( *it ) + ZVal::DeleteManagedType(**it); + } vector_val->erase(it); @@ -3465,33 +3494,33 @@ ValPtr VectorVal::At(unsigned int index) const if ( index >= vector_val->size() ) return Val::nil; + auto& elem = (*vector_val)[index]; + if ( ! elem ) + return Val::nil; + const auto& t = yield_types ? (*yield_types)[index] : yield_type; - return (*vector_val)[index].ToVal(t); + return elem->ToVal(t); } static Func* sort_function_comp = nullptr; // Used for indirect sorting to support order(). -static std::vector index_map; +static std::vector*> index_map; // The yield type of the vector being sorted. static TypePtr sort_type; -static bool sort_type_is_managed = false; -static bool sort_function(const ZVal& a, const ZVal& b) +static bool sort_function(const std::optional& a, const std::optional& b) { - // Missing values are only applicable for managed types. - if ( sort_type_is_managed ) - { - if ( ! a.ManagedVal() ) - return 0; - if ( ! b.ManagedVal() ) - return 1; - } + if ( ! a ) + return false; - auto a_v = a.ToVal(sort_type); - auto b_v = b.ToVal(sort_type); + if ( ! b ) + return true; + + auto a_v = a->ToVal(sort_type); + auto b_v = b->ToVal(sort_type); auto result = sort_function_comp->Invoke(a_v, b_v); int int_result = result->CoerceToInt(); @@ -3499,19 +3528,37 @@ static bool sort_function(const ZVal& a, const ZVal& b) return int_result < 0; } -static bool signed_sort_function (const ZVal& a, const ZVal& b) +static bool signed_sort_function(const std::optional& a, const std::optional& b) { - return a.AsInt() < b.AsInt(); + if ( ! a ) + return false; + + if ( ! b ) + return true; + + return a->AsInt() < b->AsInt(); } -static bool unsigned_sort_function (const ZVal& a, const ZVal& b) +static bool unsigned_sort_function(const std::optional& a, const std::optional& b) { - return a.AsCount() < b.AsCount(); + if ( ! a ) + return false; + + if ( ! b ) + return true; + + return a->AsCount() < b->AsCount(); } -static bool double_sort_function (const ZVal& a, const ZVal& b) +static bool double_sort_function(const std::optional& a, const std::optional& b) { - return a.AsDouble() < b.AsDouble(); + if ( ! a ) + return false; + + if ( ! b ) + return true; + + return a->AsDouble() < b->AsDouble(); } static bool indirect_sort_function(size_t a, size_t b) @@ -3540,9 +3587,8 @@ void VectorVal::Sort(Func* cmp_func) reporter->RuntimeError(GetLocationInfo(), "cannot sort a vector-of-any"); sort_type = yield_type; - sort_type_is_managed = ZVal::IsManagedType(sort_type); - bool (*sort_func)(const ZVal&, const ZVal&); + bool (*sort_func)(const std::optional&, const std::optional&); if ( cmp_func ) { @@ -3577,7 +3623,6 @@ VectorValPtr VectorVal::Order(Func* cmp_func) } sort_type = yield_type; - sort_type_is_managed = ZVal::IsManagedType(sort_type); bool (*sort_func)(size_t, size_t); @@ -3671,8 +3716,8 @@ ValPtr VectorVal::DoClone(CloneState* state) for ( auto i = 0; i < n; ++i ) { - auto vc = At(i)->Clone(state); - vv->Append(std::move(vc)); + auto elem = At(i); + vv->Assign(i, elem ? elem->Clone(state) : nullptr); } return vv; diff --git a/src/Val.h b/src/Val.h index 4bdd1b9dbc..2f3fb29e21 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1505,10 +1505,6 @@ public: */ bool Assign(unsigned int index, ValPtr element); - // Note: the following nullptr method can also go upon removing the above. - void Assign(unsigned int index, std::nullptr_t) - { Assign(index, ValPtr{}); } - /** * Assigns a given value to multiple indices in the vector. * @param index The starting index to assign to. @@ -1581,18 +1577,19 @@ public: /** * Returns the given element in a given underlying representation. * Enables efficient vector access. Caller must ensure that the - * index lies within the vector's range. + * index lies within the vector's range, and does not point to + * a "hole". * @param index The position in the vector of the element to return. * @return The element's underlying value. */ bro_uint_t CountAt(unsigned int index) const - { return (*vector_val)[index].uint_val; } + { return (*vector_val)[index]->uint_val; } const RecordVal* RecordValAt(unsigned int index) const - { return (*vector_val)[index].record_val; } + { return (*vector_val)[index]->record_val; } bool BoolAt(unsigned int index) const - { return static_cast((*vector_val)[index].uint_val); } + { return static_cast((*vector_val)[index]->uint_val); } const StringVal* StringValAt(unsigned int index) const - { return (*vector_val)[index].string_val; } + { return (*vector_val)[index]->string_val; } const String* StringAt(unsigned int index) const { return StringValAt(index)->AsString(); } @@ -1601,7 +1598,7 @@ protected: * Returns the element at a given index or nullptr if it does not exist. * @param index The position in the vector of the element to return. * @return The element at the given index or nullptr if the index - * does not exist (it's greater than or equal to vector's current size). + * does not exist. * * Protected to ensure callers pick one of the differentiated accessors * above, as appropriate, with ValAt() providing the original semantics. @@ -1624,7 +1621,7 @@ private: // Add the given number of "holes" to the end of a vector. void AddHoles(int nholes); - std::vector* vector_val; + std::vector>* vector_val; // For homogeneous vectors (the usual case), the type of the // elements. Will be TYPE_VOID for empty vectors created using diff --git a/testing/btest/Baseline/language.record-index-complex-fields/output b/testing/btest/Baseline/language.record-index-complex-fields/output index 431623305a..a277a05a0d 100644 --- a/testing/btest/Baseline/language.record-index-complex-fields/output +++ b/testing/btest/Baseline/language.record-index-complex-fields/output @@ -11,7 +11,7 @@ b } } { -[a=13, tags_v=[0, 0, 2, 3], tags_t={ +[a=13, tags_v=[, , 2, 3], tags_t={ [four] = 4, [five] = 5 }, tags_s={ diff --git a/testing/btest/Baseline/language.vector/out b/testing/btest/Baseline/language.vector/out index 5d6541ae11..b75cf8048e 100644 --- a/testing/btest/Baseline/language.vector/out +++ b/testing/btest/Baseline/language.vector/out @@ -34,6 +34,11 @@ add element (PASS) access element (PASS) add element (PASS) access element (PASS) +add above a hole (PASS) +in operator for non-hole (PASS) +in operator for hole (PASS) +in operator for edge (PASS) +in operator for out of range (PASS) add element (PASS) access element (PASS) overwrite element (PASS) @@ -68,3 +73,8 @@ slicing assignment (PASS) slicing assignment (PASS) slicing assignment grow (PASS) slicing assignment shrink (PASS) +? operator (PASS) +copy of a vector with holes (PASS) +copy of a vector with trailing holes, [0, 2, 3, 77, , ], [0, 2, 3, 77, , ] +hole in vector of managed types, 5, [[a=T], [a=T], , , [a=T]] +hole in vector of managed types after replacing slice, 3, [[a=T], [a=T], ] diff --git a/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-json/ssh.log b/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-json/ssh.log index efb85b99a7..1a1c60d615 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-json/ssh.log +++ b/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-json/ssh.log @@ -1,2 +1,2 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -{"b":true,"i":-42,"e":"SSH::LOG","c":21,"p":123,"sn":"10.0.0.0/24","a":"1.2.3.4","d":3.14,"t":XXXXXXXXXX.XXXXXX,"iv":100.0,"s":"hurz","sc":[4,2,3,1],"ss":["CC","BB","AA"],"se":[],"vc":[10,20,30],"ve":[],"vn":[0,0,2],"f":"SSH::foo\n{ \nif (0 < SSH::i) \n\treturn (Foo);\nelse\n\treturn (Bar);\n\n}"} +{"b":true,"i":-42,"e":"SSH::LOG","c":21,"p":123,"sn":"10.0.0.0/24","a":"1.2.3.4","d":3.14,"t":XXXXXXXXXX.XXXXXX,"iv":100.0,"s":"hurz","sc":[4,2,3,1],"ss":["CC","BB","AA"],"se":[],"vc":[10,20,30],"ve":[],"vn":[0,null,2],"f":"SSH::foo\n{ \nif (0 < SSH::i) \n\treturn (Foo);\nelse\n\treturn (Bar);\n\n}"} diff --git a/testing/btest/Baseline/scripts.base.utils.json/output b/testing/btest/Baseline/scripts.base.utils.json/output index b1726f5da1..d5e4a4a38d 100644 --- a/testing/btest/Baseline/scripts.base.utils.json/output +++ b/testing/btest/Baseline/scripts.base.utils.json/output @@ -29,7 +29,7 @@ XXXXXXXXXX.XXXXXX ["1.2.3.4"] [[false,true]] [{"s":"test"}] -[0,0,2] +[0,null,2] [] [2,1] ["1.2.3.4"] diff --git a/testing/btest/language/vector.zeek b/testing/btest/language/vector.zeek index ea330f3842..a0ef9a9f1e 100644 --- a/testing/btest/language/vector.zeek +++ b/testing/btest/language/vector.zeek @@ -10,6 +10,11 @@ function test_case(msg: string, expect: bool) # Note: only global vectors can be initialized with curly braces global vg1: vector of string = { "curly", "braces" }; +type R: record { + a: bool &default=T; +}; + + event zeek_init() { local v1: vector of string = vector( "test", "example" ); @@ -27,6 +32,7 @@ event zeek_init() local v13 = vector( F, F, T ); local v14 = v12 && v13; local v15 = v12 || v13; + local v18 = v12 ? v5 : v6; # Type inference tests @@ -110,6 +116,13 @@ event zeek_init() test_case( "add element", |v5| == 4 ); test_case( "access element", v5[3] == 77 ); + v5[10] = 10; + test_case( "add above a hole", |v5| == 11 ); + test_case( "in operator for non-hole", 3 in v5 ); + test_case( "in operator for hole", 4 !in v5 ); + test_case( "in operator for edge", |v5|-1 in v5 ); + test_case( "in operator for out of range", 44 !in v5 ); + vg1[2] = "global"; test_case( "add element", |vg1| == 3 ); test_case( "access element", vg1[2] == "global" ); @@ -134,7 +147,7 @@ event zeek_init() test_case( "access element", v4[0] == "new4" ); v5[0] = 0; - test_case( "overwrite element", |v5| == 4 ); + test_case( "overwrite element", |v5| == 11 ); test_case( "access element", v5[0] == 0 ); vg1[1] = "new5"; @@ -144,11 +157,13 @@ event zeek_init() # Test increment/decrement operators ++v5; - test_case( "++ operator", |v5| == 4 && v5[0] == 1 && v5[1] == 3 - && v5[2] == 4 && v5[3] == 78 ); + test_case( "++ operator", |v5| == 11 && v5[0] == 1 && v5[1] == 3 + && v5[2] == 4 && v5[3] == 78 && v5[10] == 11 + && 4 !in v5 ); --v5; - test_case( "-- operator", |v5| == 4 && v5[0] == 0 && v5[1] == 2 - && v5[2] == 3 && v5[3] == 77 ); + test_case( "-- operator", |v5| == 11 && v5[0] == 0 && v5[1] == 2 + && v5[2] == 3 && v5[3] == 77 && v5[10] == 10 + && 4 !in v5 ); # Test +,-,*,/,% of two vectors @@ -183,4 +198,23 @@ event zeek_init() test_case( "slicing assignment grow", all_set(v17 == vector(6, 2, 9, 10, 11, 5)) ); v17[2:5] = vector(9); test_case( "slicing assignment shrink", all_set(v17 == vector(6, 2, 9, 5)) ); + + # Test boolean ? operator. + test_case( "? operator", all_set(v18 == vector(1, 20, 3)) ); + + # Test copying of a vector with holes, as this used to crash. + local v19 = copy(v5); + test_case( "copy of a vector with holes", |v5| == |v19| ); + # Even after removing some elements at the end, any trailing holes should + # be preserved after copying; + v5[6:] = vector(); + local v20 = copy(v5); + print "copy of a vector with trailing holes", v5, v20; + + local v21 = vector(R(), R()); + v21[4] = R(); + print "hole in vector of managed types", |v21|, v21; + v21[3:] = vector(); + print "hole in vector of managed types after replacing slice", |v21|, v21; + }