diff --git a/.gitignore b/.gitignore index fa397f98d2..8dacf57dc7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,7 @@ build tmp *.gcov + +# Configuration and build directories for CLion +.idea +cmake-build-debug \ No newline at end of file diff --git a/CHANGES b/CHANGES index 2bf9485d30..96b1e74a6d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,20 @@ +2.6-369 | 2019-06-04 17:53:10 -0700 + + * GH-155: Improve coercion of expression lists to vector types (Tim Wojtulewicz, Corelight) + + * GH-159: Allow coercion of numeric record field values to other types (Tim Wojtulewicz, Corelight) + + * Allow passing a location to BroObj::Warning and BroObj::Error. (Tim Wojtulewicz, Corelight) + + This allows callers (such as check_and_promote) to pass an expression + location to be logged if the location doesn't exist in the value being + promoted. + + * Add CLion directories to gitignore (Tim Wojtulewicz, Corelight) + + * Move #define outside of max_type for clarity (Tim Wojtulewicz, Corelight) + 2.6-361 | 2019-06-04 10:30:21 -0700 * GH-293: Protect copy() against reference cycles. (Robin Sommer, Corelight) diff --git a/VERSION b/VERSION index 368e86895d..a5f150802d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.6-361 +2.6-369 diff --git a/src/Expr.cc b/src/Expr.cc index 27d5de135b..1ec357e945 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2591,7 +2591,7 @@ bool AssignExpr::TypeCheck(attr_list* attrs) if ( op2->Tag() == EXPR_LIST ) { - op2 = new VectorConstructorExpr(op2->AsListExpr()); + op2 = new VectorConstructorExpr(op2->AsListExpr(), op1->Type()); return true; } } @@ -4136,15 +4136,41 @@ RecordCoerceExpr::RecordCoerceExpr(Expr* op, RecordType* r) if ( ! same_type(sup_t_i, sub_t_i) ) { - if ( sup_t_i->Tag() != TYPE_RECORD || - sub_t_i->Tag() != TYPE_RECORD || - ! record_promotion_compatible(sup_t_i->AsRecordType(), - sub_t_i->AsRecordType()) ) + auto is_arithmetic_promotable = [](BroType* sup, BroType* sub) -> bool { - char buf[512]; - safe_snprintf(buf, sizeof(buf), + auto sup_tag = sup->Tag(); + auto sub_tag = sub->Tag(); + + if ( ! BothArithmetic(sup_tag, sub_tag) ) + return false; + + if ( sub_tag == TYPE_DOUBLE && IsIntegral(sup_tag) ) + return false; + + if ( sub_tag == TYPE_INT && sup_tag == TYPE_COUNT ) + return false; + + return true; + }; + + auto is_record_promotable = [](BroType* sup, BroType* sub) -> bool + { + if ( sup->Tag() != TYPE_RECORD ) + return false; + + if ( sub->Tag() != TYPE_RECORD ) + return false; + + return record_promotion_compatible(sup->AsRecordType(), + sub->AsRecordType()); + }; + + if ( ! is_arithmetic_promotable(sup_t_i, sub_t_i) && + ! is_record_promotable(sup_t_i, sub_t_i) ) + { + string error_msg = fmt( "type clash for field \"%s\"", sub_r->FieldName(i)); - Error(buf, sub_t_i); + Error(error_msg.c_str(), sub_t_i); SetError(); break; } @@ -4162,11 +4188,9 @@ RecordCoerceExpr::RecordCoerceExpr(Expr* op, RecordType* r) { if ( ! t_r->FieldDecl(i)->FindAttr(ATTR_OPTIONAL) ) { - char buf[512]; - safe_snprintf(buf, sizeof(buf), - "non-optional field \"%s\" missing", - t_r->FieldName(i)); - Error(buf); + string error_msg = fmt( + "non-optional field \"%s\" missing", t_r->FieldName(i)); + Error(error_msg.c_str()); SetError(); break; } @@ -4254,6 +4278,20 @@ Val* RecordCoerceExpr::Fold(Val* v) const rhs = new_val; } } + else if ( BothArithmetic(rhs_type->Tag(), field_type->Tag()) && + ! same_type(rhs_type, field_type) ) + { + if ( Val* new_val = check_and_promote(rhs, field_type, false, op->GetLocationInfo()) ) + { + // Don't call unref here on rhs because check_and_promote already called it. + rhs = new_val; + } + else + { + Unref(val); + RuntimeError("Failed type conversion"); + } + } val->Assign(i, rhs); } diff --git a/src/Obj.cc b/src/Obj.cc index 9c3b50a950..fdf2b7ef99 100644 --- a/src/Obj.cc +++ b/src/Obj.cc @@ -100,21 +100,21 @@ BroObj::~BroObj() delete location; } -void BroObj::Warn(const char* msg, const BroObj* obj2, int pinpoint_only) const +void BroObj::Warn(const char* msg, const BroObj* obj2, int pinpoint_only, const Location* expr_location) const { ODesc d; - DoMsg(&d, msg, obj2, pinpoint_only); + DoMsg(&d, msg, obj2, pinpoint_only, expr_location); reporter->Warning("%s", d.Description()); reporter->PopLocation(); } -void BroObj::Error(const char* msg, const BroObj* obj2, int pinpoint_only) const +void BroObj::Error(const char* msg, const BroObj* obj2, int pinpoint_only, const Location* expr_location) const { if ( suppress_errors ) return; ODesc d; - DoMsg(&d, msg, obj2, pinpoint_only); + DoMsg(&d, msg, obj2, pinpoint_only, expr_location); reporter->Error("%s", d.Description()); reporter->PopLocation(); } @@ -200,7 +200,7 @@ void BroObj::UpdateLocationEndInfo(const Location& end) } void BroObj::DoMsg(ODesc* d, const char s1[], const BroObj* obj2, - int pinpoint_only) const + int pinpoint_only, const Location* expr_location) const { d->SetShort(); @@ -211,6 +211,8 @@ void BroObj::DoMsg(ODesc* d, const char s1[], const BroObj* obj2, if ( obj2 && obj2->GetLocationInfo() != &no_location && *obj2->GetLocationInfo() != *GetLocationInfo() ) loc2 = obj2->GetLocationInfo(); + else if ( expr_location ) + loc2 = expr_location; reporter->PushLocation(GetLocationInfo(), loc2); } diff --git a/src/Obj.h b/src/Obj.h index 047eec0856..21730ff367 100644 --- a/src/Obj.h +++ b/src/Obj.h @@ -118,9 +118,9 @@ public: // included in the message, though if pinpoint_only is non-zero, // then obj2 is only used to pinpoint the location. void Warn(const char* msg, const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; void Error(const char* msg, const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; // Report internal errors. void BadTag(const char* msg, const char* t1 = 0, @@ -178,7 +178,7 @@ private: friend class SuppressErrors; void DoMsg(ODesc* d, const char s1[], const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; void PinPoint(ODesc* d, const BroObj* obj2 = 0, int pinpoint_only = 0) const; diff --git a/src/Type.cc b/src/Type.cc index f9cb915c71..3fda9aa7ce 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -2123,6 +2123,10 @@ int is_assignable(BroType* t) return 0; } +#define CHECK_TYPE(t) \ + if ( t1 == t || t2 == t ) \ + return t; + TypeTag max_type(TypeTag t1, TypeTag t2) { if ( t1 == TYPE_INTERVAL || t1 == TYPE_TIME ) @@ -2132,10 +2136,6 @@ TypeTag max_type(TypeTag t1, TypeTag t2) if ( BothArithmetic(t1, t2) ) { -#define CHECK_TYPE(t) \ - if ( t1 == t || t2 == t ) \ - return t; - CHECK_TYPE(TYPE_DOUBLE); CHECK_TYPE(TYPE_INT); CHECK_TYPE(TYPE_COUNT); diff --git a/src/Val.cc b/src/Val.cc index 49eafc7929..bb9a3d1601 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -564,6 +564,35 @@ void Val::ValDescribeReST(ODesc* d) const } } + +bool Val::WouldOverflow(const BroType* from_type, const BroType* to_type, const Val* val) + { + if ( !to_type || !from_type ) + return true; + else if ( same_type(to_type, from_type) ) + return false; + + if ( to_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return false; + else if ( to_type->InternalType() == TYPE_INTERNAL_UNSIGNED ) + { + if ( from_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return (val->InternalDouble() < 0.0 || val->InternalDouble() > static_cast(UINT64_MAX)); + else if ( from_type->InternalType() == TYPE_INTERNAL_INT ) + return (val->InternalInt() < 0); + } + else if ( to_type->InternalType() == TYPE_INTERNAL_INT ) + { + if ( from_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return (val->InternalDouble() < static_cast(INT64_MIN) || + val->InternalDouble() > static_cast(INT64_MAX)); + else if ( from_type->InternalType() == TYPE_INTERNAL_UNSIGNED ) + return (val->InternalUnsigned() > INT64_MAX); + } + + return false; + } + MutableVal::~MutableVal() { for ( list::iterator i = aliases.begin(); i != aliases.end(); ++i ) @@ -3565,7 +3594,7 @@ bool OpaqueVal::DoUnserialize(UnserialInfo* info) return true; } -Val* check_and_promote(Val* v, const BroType* t, int is_init) +Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* expr_location) { if ( ! v ) return 0; @@ -3589,7 +3618,7 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) if ( same_type(t, vt, is_init) ) return v; - t->Error("type clash", v); + t->Error("type clash", v, 0, expr_location); Unref(v); return 0; } @@ -3598,9 +3627,9 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) (! IsArithmetic(v_tag) || t_tag != TYPE_TIME || ! v->IsZero()) ) { if ( t_tag == TYPE_LIST || v_tag == TYPE_LIST ) - t->Error("list mixed with scalar", v); + t->Error("list mixed with scalar", v, 0, expr_location); else - t->Error("arithmetic mixed with non-arithmetic", v); + t->Error("arithmetic mixed with non-arithmetic", v, 0, expr_location); Unref(v); return 0; } @@ -3608,12 +3637,12 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) if ( v_tag == t_tag ) return v; - if ( t_tag != TYPE_TIME ) + if ( t_tag != TYPE_TIME && ! BothArithmetic(t_tag, v_tag) ) { TypeTag mt = max_type(t_tag, v_tag); if ( mt != t_tag ) { - t->Error("over-promotion of arithmetic value", v); + t->Error("over-promotion of arithmetic value", v, 0, expr_location); Unref(v); return 0; } @@ -3630,7 +3659,13 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) Val* promoted_v; switch ( it ) { case TYPE_INTERNAL_INT: - if ( t_tag == TYPE_INT ) + if ( ( vit == TYPE_INTERNAL_UNSIGNED || vit == TYPE_INTERNAL_DOUBLE ) && Val::WouldOverflow(vt, t, v) ) + { + t->Error("overflow promoting from unsigned/double to signed arithmetic value", v, 0, expr_location); + Unref(v); + return 0; + } + else if ( t_tag == TYPE_INT ) promoted_v = val_mgr->GetInt(v->CoerceToInt()); else if ( t_tag == TYPE_BOOL ) promoted_v = val_mgr->GetBool(v->CoerceToInt()); @@ -3644,7 +3679,13 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) break; case TYPE_INTERNAL_UNSIGNED: - if ( t_tag == TYPE_COUNT || t_tag == TYPE_COUNTER ) + if ( ( vit == TYPE_INTERNAL_DOUBLE || vit == TYPE_INTERNAL_INT) && Val::WouldOverflow(vt, t, v) ) + { + t->Error("overflow promoting from signed/double to unsigned arithmetic value", v, 0, expr_location); + Unref(v); + return 0; + } + else if ( t_tag == TYPE_COUNT || t_tag == TYPE_COUNTER ) promoted_v = val_mgr->GetCount(v->CoerceToUnsigned()); else // port { diff --git a/src/Val.h b/src/Val.h index d17dc24cb2..41ad6b654c 100644 --- a/src/Val.h +++ b/src/Val.h @@ -367,6 +367,8 @@ public: } #endif + static bool WouldOverflow(const BroType* from_type, const BroType* to_type, const Val* val); + protected: friend class EnumType; @@ -1228,7 +1230,7 @@ protected: // Unref()'ing the original. If not a match, generates an error message // and returns nil, also Unref()'ing v. If is_init is true, then // the checking is done in the context of an initialization. -extern Val* check_and_promote(Val* v, const BroType* t, int is_init); +extern Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* expr_location = nullptr); // Given a pointer to where a Val's core (i.e., its BRO value) resides, // returns a corresponding newly-created or Ref()'d Val. ptr must already diff --git a/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out new file mode 100644 index 0000000000..d139c43a0c --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out @@ -0,0 +1 @@ +error in ./double_convert_failure1.zeek, line 7 and double: type clash for field "cc" ((coerce [$cc=5.0] to myrecord) and double) diff --git a/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out new file mode 100644 index 0000000000..2cc83659de --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out @@ -0,0 +1 @@ +error in ./double_convert_failure2.zeek, line 7 and double: type clash for field "cc" ((coerce [$cc=-5.0] to myrecord) and double) diff --git a/testing/btest/Baseline/language.type-coerce-numerics/first_set.out b/testing/btest/Baseline/language.type-coerce-numerics/first_set.out new file mode 100644 index 0000000000..6ff2d7d4df --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/first_set.out @@ -0,0 +1,14 @@ +error in int and ./first_set.zeek, line 46: overflow promoting from unsigned/double to signed arithmetic value (int and 9223372036854775808) +expression error in ./first_set.zeek, line 46: Failed type conversion ((coerce [$ii=9223372036854775808] to record { ii:int; cc:count; dd:double; })) +3 +int +4 +int +5 +int +6 +int +7.0 +double +-5.0 +double diff --git a/testing/btest/Baseline/language.type-coerce-numerics/int_convert_failure.out b/testing/btest/Baseline/language.type-coerce-numerics/int_convert_failure.out new file mode 100644 index 0000000000..3c896e096f --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/int_convert_failure.out @@ -0,0 +1 @@ +error in ./int_convert_failure.zeek, line 7 and int: type clash for field "cc" ((coerce [$cc=-5] to myrecord) and int) diff --git a/testing/btest/Baseline/language.type-coerce-numerics/vectors.out b/testing/btest/Baseline/language.type-coerce-numerics/vectors.out new file mode 100644 index 0000000000..5baa5f67c7 --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/vectors.out @@ -0,0 +1,18 @@ +vector of count +vector of count +vector of count +[1, 2] +[3, 4] +[4, 6] +vector of int +vector of int +vector of int +[1, 2] +[3, 4] +[4, 6] +vector of double +vector of double +vector of double +[1.0, 2.0] +[3.0, 4.0] +[4.0, 6.0] diff --git a/testing/btest/language/type-coerce-numerics.zeek b/testing/btest/language/type-coerce-numerics.zeek new file mode 100644 index 0000000000..996326361b --- /dev/null +++ b/testing/btest/language/type-coerce-numerics.zeek @@ -0,0 +1,132 @@ +# @TEST-EXEC: zeek -b first_set.zeek >first_set.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff first_set.out + +# @TEST-EXEC-FAIL: zeek -b double_convert_failure1.zeek >double_convert_failure1.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure1.out + +# @TEST-EXEC-FAIL: zeek -b double_convert_failure2.zeek >double_convert_failure2.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure1.out + +# @TEST-EXEC-FAIL: zeek -b int_convert_failure.zeek >int_convert_failure.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff int_convert_failure.out + +# @TEST-EXEC: zeek -b vectors.zeek >vectors.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff vectors.out + +@TEST-START-FILE first_set.zeek +type myrecord : record { + ii: int &optional; + cc: count &optional; + dd: double &optional; +}; + +# Allow coercion from count values to int +global globalint: myrecord &redef; +redef globalint = [$ii = 2]; + +# All of these cases should succeed +event zeek_init() + { + # Allow coercion from count values to int + local intconvert1 = myrecord($ii = 3); + print(intconvert1$ii); + print(type_name(intconvert1$ii)); + + local intconvert2: myrecord = record($ii = 4); + print(intconvert2$ii); + print(type_name(intconvert2$ii)); + + local intconvert3: myrecord = [$ii = 5]; + print(intconvert3$ii); + print(type_name(intconvert3$ii)); + + local intconvert4: myrecord; + intconvert4$ii = 6; + print(intconvert4$ii); + print(type_name(intconvert4$ii)); + + # Convert from count/integer values into doubles + local doubleconvert1 = myrecord($dd = 7); + print(doubleconvert1$dd); + print(type_name(doubleconvert1$dd)); + + local doubleconvert2 = myrecord($dd = -5); + print(doubleconvert2$dd); + print(type_name(doubleconvert2$dd)); + } + +event zeek_init() + { + # This value is INT64_MAX+1, which overflows a signed integer and + # throws an error + local overflow = myrecord($ii = 9223372036854775808); + } +@TEST-END-FILE + +@TEST-START-FILE double_convert_failure1.zeek +type myrecord : record { + cc: count &optional; +}; + +event zeek_init() + { + local convert = myrecord($cc = 5.0); + } +@TEST-END-FILE + +@TEST-START-FILE double_convert_failure2.zeek +type myrecord : record { + cc: count &optional; +}; + +event zeek_init() + { + local convert = myrecord($cc = -5.0); + } +@TEST-END-FILE + +@TEST-START-FILE int_convert_failure.zeek +type myrecord : record { + cc: count &optional; +}; + +event zeek_init() + { + local convert = myrecord($cc = -5); + } +@TEST-END-FILE + +@TEST-START-FILE vectors.zeek +event zeek_init() + { + local c1 : vector of count = { 1 , 2 }; + local c2 : vector of count = { 3 , 4 }; + local c3 = c1 + c2; + print type_name(c1); + print type_name(c2); + print type_name(c3); + print c1; + print c2; + print c3; + + local i1 : vector of int = { 1, 2 }; + local i2 : vector of int = { 3, 4 }; + local i3 = i1 + i2; + print type_name(i1); + print type_name(i2); + print type_name(i3); + print i1; + print i2; + print i3; + + local d1 : vector of double = { 1, 2 }; + local d2 : vector of double = { 3, 4 }; + local d3 = d1 + d2; + print type_name(d1); + print type_name(d2); + print type_name(d3); + print d1; + print d2; + print d3; + } +@TEST-END-FILE