From d2366438948049e4fad46300fbac805f0b45dcae Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 17 Mar 2015 13:45:00 -0700 Subject: [PATCH 1/3] Make error message when encountering not existing enums better. Example: internal error: Value not 'NoSuch::Notice' for stream 'ignored_notices' is not a valid enum. Abort trap: 6 Addresses BIT-1199 --- src/input/Manager.cc | 82 ++++++++++++++++++++------------------ src/input/Manager.h | 8 ++-- src/input/ReaderBackend.cc | 2 +- 3 files changed, 49 insertions(+), 43 deletions(-) diff --git a/src/input/Manager.cc b/src/input/Manager.cc index 044f9fcae3..ac79257ad9 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -971,7 +971,7 @@ Val* Manager::RecordValToIndexVal(RecordVal *r) } -Val* Manager::ValueToIndexVal(int num_fields, const RecordType *type, const Value* const *vals) +Val* Manager::ValueToIndexVal(const Stream* i, int num_fields, const RecordType *type, const Value* const *vals) { Val* idxval; int position = 0; @@ -979,7 +979,7 @@ Val* Manager::ValueToIndexVal(int num_fields, const RecordType *type, const Valu if ( num_fields == 1 && type->FieldType(0)->Tag() != TYPE_RECORD ) { - idxval = ValueToVal(vals[0], type->FieldType(0)); + idxval = ValueToVal(i, vals[0], type->FieldType(0)); position = 1; } else @@ -988,11 +988,11 @@ Val* Manager::ValueToIndexVal(int num_fields, const RecordType *type, const Valu for ( int j = 0 ; j < type->NumFields(); j++ ) { if ( type->FieldType(j)->Tag() == TYPE_RECORD ) - l->Append(ValueToRecordVal(vals, + l->Append(ValueToRecordVal(i, vals, type->FieldType(j)->AsRecordType(), &position)); else { - l->Append(ValueToVal(vals[position], type->FieldType(j))); + l->Append(ValueToVal(i, vals[position], type->FieldType(j))); position++; } } @@ -1107,10 +1107,10 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) valval = 0; else if ( stream->num_val_fields == 1 && !stream->want_record ) - valval = ValueToVal(vals[position], stream->rtype->FieldType(0)); + valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0)); else - valval = ValueToRecordVal(vals, stream->rtype, &position); + valval = ValueToRecordVal(i, vals, stream->rtype, &position); // call stream first to determine if we really add / change the entry @@ -1118,7 +1118,7 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - predidx = ValueToRecordVal(vals, stream->itype, &startpos); + predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); if ( updated ) ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, BifType::Enum::Input::Event); @@ -1168,7 +1168,7 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) // I think there is an unref missing here. But if I insert is, it crashes :) } else - idxval = ValueToIndexVal(stream->num_idx_fields, stream->itype, vals); + idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); Val* oldval = 0; if ( updated == true ) @@ -1203,7 +1203,7 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); if ( updated ) { // in case of update send back the old value. @@ -1427,7 +1427,7 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * if ( stream->want_record ) { - RecordVal * r = ValueToRecordVal(vals, stream->fields, &position); + RecordVal * r = ValueToRecordVal(i, vals, stream->fields, &position); out_vals.push_back(r); } @@ -1438,13 +1438,13 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * Val* val = 0; if ( stream->fields->FieldType(j)->Tag() == TYPE_RECORD ) - val = ValueToRecordVal(vals, + val = ValueToRecordVal(i, vals, stream->fields->FieldType(j)->AsRecordType(), &position); else { - val = ValueToVal(vals[position], stream->fields->FieldType(j)); + val = ValueToVal(i, vals[position], stream->fields->FieldType(j)); position++; } @@ -1464,7 +1464,7 @@ int Manager::PutTable(Stream* i, const Value* const *vals) assert(i->stream_type == TABLE_STREAM); TableStream* stream = (TableStream*) i; - Val* idxval = ValueToIndexVal(stream->num_idx_fields, stream->itype, vals); + Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); Val* valval; int position = stream->num_idx_fields; @@ -1473,9 +1473,9 @@ int Manager::PutTable(Stream* i, const Value* const *vals) valval = 0; else if ( stream->num_val_fields == 1 && stream->want_record == 0 ) - valval = ValueToVal(vals[position], stream->rtype->FieldType(0)); + valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0)); else - valval = ValueToRecordVal(vals, stream->rtype, &position); + valval = ValueToRecordVal(i, vals, stream->rtype, &position); // if we have a subscribed event, we need to figure out, if this is an update or not // same for predicates @@ -1503,7 +1503,7 @@ int Manager::PutTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); if ( updated ) ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, @@ -1538,7 +1538,7 @@ int Manager::PutTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); if ( updated ) { @@ -1612,7 +1612,7 @@ bool Manager::Delete(ReaderFrontend* reader, Value* *vals) if ( i->stream_type == TABLE_STREAM ) { TableStream* stream = (TableStream*) i; - Val* idxval = ValueToIndexVal(stream->num_idx_fields, stream->itype, vals); + Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); assert(idxval != 0); readVals = stream->num_idx_fields + stream->num_val_fields; bool streamresult = true; @@ -1626,7 +1626,7 @@ bool Manager::Delete(ReaderFrontend* reader, Value* *vals) Ref(val); EnumVal *ev = new EnumVal(BifEnum::Input::EVENT_REMOVED, BifType::Enum::Input::Event); int startpos = 0; - Val* predidx = ValueToRecordVal(vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); streamresult = CallPred(stream->pred, 3, ev, predidx, val); @@ -1708,8 +1708,15 @@ bool Manager::CallPred(Func* pred_func, const int numvals, ...) return result; } -bool Manager::SendEvent(const string& name, const int num_vals, Value* *vals) +bool Manager::SendEvent(ReaderFrontend* reader, const string& name, const int num_vals, Value* *vals) { + Stream *i = FindStream(reader); + if ( i == 0 ) + { + reporter->InternalWarning("Unknown reader %s in SendEvent for event %s", reader->Name(), name.c_str()); + return false; + } + EventHandler* handler = event_registry->Lookup(name.c_str()); if ( handler == 0 ) { @@ -1733,8 +1740,8 @@ bool Manager::SendEvent(const string& name, const int num_vals, Value* *vals) } val_list* vl = new val_list; - for ( int i = 0; i < num_vals; i++) - vl->append(ValueToVal(vals[i], type->FieldType(i))); + for ( int j = 0; j < num_vals; j++) + vl->append(ValueToVal(i, vals[j], type->FieldType(j))); mgr.QueueEvent(handler, vl, SOURCE_LOCAL); @@ -1809,7 +1816,7 @@ RecordVal* Manager::ListValToRecordVal(ListVal* list, RecordType *request_type, } // Convert a threading value to a record value -RecordVal* Manager::ValueToRecordVal(const Value* const *vals, +RecordVal* Manager::ValueToRecordVal(const Stream* stream, const Value* const *vals, RecordType *request_type, int* position) { assert(position != 0); // we need the pointer to point to data. @@ -1819,7 +1826,7 @@ RecordVal* Manager::ValueToRecordVal(const Value* const *vals, { Val* fieldVal = 0; if ( request_type->FieldType(i)->Tag() == TYPE_RECORD ) - fieldVal = ValueToRecordVal(vals, request_type->FieldType(i)->AsRecordType(), position); + fieldVal = ValueToRecordVal(stream, vals, request_type->FieldType(i)->AsRecordType(), position); else if ( request_type->FieldType(i)->Tag() == TYPE_FILE || request_type->FieldType(i)->Tag() == TYPE_FUNC ) { @@ -1834,7 +1841,7 @@ RecordVal* Manager::ValueToRecordVal(const Value* const *vals, } else { - fieldVal = ValueToVal(vals[*position], request_type->FieldType(i)); + fieldVal = ValueToVal(stream, vals[*position], request_type->FieldType(i)); (*position)++; } @@ -2103,12 +2110,12 @@ HashKey* Manager::HashValues(const int num_elements, const Value* const *vals) } // convert threading value to Bro value -Val* Manager::ValueToVal(const Value* val, BroType* request_type) +Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_type) { if ( request_type->Tag() != TYPE_ANY && request_type->Tag() != val->type ) { - reporter->InternalError("Typetags don't match: %d vs %d", request_type->Tag(), val->type); + reporter->InternalError("Typetags don't match: %d vs %d in stream %s", request_type->Tag(), val->type, i->name.c_str()); return 0; } @@ -2189,9 +2196,9 @@ Val* Manager::ValueToVal(const Value* val, BroType* request_type) set_index->Append(type->Ref()); SetType* s = new SetType(set_index, 0); TableVal* t = new TableVal(s); - for ( int i = 0; i < val->val.set_val.size; i++ ) + for ( int j = 0; j < val->val.set_val.size; j++ ) { - Val* assignval = ValueToVal( val->val.set_val.vals[i], type ); + Val* assignval = ValueToVal(i, val->val.set_val.vals[j], type); t->Assign(assignval, 0); Unref(assignval); // idex is not consumed by assign. } @@ -2206,8 +2213,8 @@ Val* Manager::ValueToVal(const Value* val, BroType* request_type) BroType* type = request_type->AsVectorType()->YieldType(); VectorType* vt = new VectorType(type->Ref()); VectorVal* v = new VectorVal(vt); - for ( int i = 0; i < val->val.vector_val.size; i++ ) - v->Assign(i, ValueToVal( val->val.set_val.vals[i], type )); + for ( int j = 0; j < val->val.vector_val.size; j++ ) + v->Assign(j, ValueToVal(i, val->val.set_val.vals[j], type)); Unref(vt); return v; @@ -2216,25 +2223,24 @@ Val* Manager::ValueToVal(const Value* val, BroType* request_type) case TYPE_ENUM: { // Convert to string first to not have to deal with missing // \0's... - string module_string(val->val.string_val.data, val->val.string_val.length); - string var_string(val->val.string_val.data, val->val.string_val.length); + string enum_string(val->val.string_val.data, val->val.string_val.length); - string module = extract_module_name(module_string.c_str()); - string var = extract_var_name(var_string.c_str()); + string module = extract_module_name(enum_string.c_str()); + string var = extract_var_name(enum_string.c_str()); // Well, this is kind of stupid, because EnumType just // mangles the module name and the var name together again... // but well. bro_int_t index = request_type->AsEnumType()->Lookup(module, var.c_str()); if ( index == -1 ) - reporter->InternalError("Value not found in enum mappimg. Module: %s, var: %s, var size: %zu", - module.c_str(), var.c_str(), var.size()); + reporter->InternalError("Value not '%s' for stream '%s' is not a valid enum.", + enum_string.c_str(), i->name.c_str()); return new EnumVal(index, request_type->Ref()->AsEnumType()); } default: - reporter->InternalError("unsupported type for input_read"); + reporter->InternalError("Unsupported type for input_read in stream %s", i->name.c_str()); } assert(false); diff --git a/src/input/Manager.h b/src/input/Manager.h index cfac803129..551eeaecf3 100644 --- a/src/input/Manager.h +++ b/src/input/Manager.h @@ -129,7 +129,7 @@ protected: // Allows readers to directly send Bro events. The num_vals and vals // must be the same the named event expects. Takes ownership of // threading::Value fields. - bool SendEvent(const string& name, const int num_vals, threading::Value* *vals); + bool SendEvent(ReaderFrontend* reader, const string& name, const int num_vals, threading::Value* *vals); // Instantiates a new ReaderBackend of the given type (note that // doing so creates a new thread!). @@ -205,14 +205,14 @@ private: // Convert Threading::Value to an internal Bro Type (works also with // Records). - Val* ValueToVal(const threading::Value* val, BroType* request_type); + Val* ValueToVal(const Stream* i, const threading::Value* val, BroType* request_type); // Convert Threading::Value to an internal Bro List type. - Val* ValueToIndexVal(int num_fields, const RecordType* type, const threading::Value* const *vals); + Val* ValueToIndexVal(const Stream* i, int num_fields, const RecordType* type, const threading::Value* const *vals); // Converts a threading::value to a record type. Mostly used by // ValueToVal. - RecordVal* ValueToRecordVal(const threading::Value* const *vals, RecordType *request_type, int* position); + RecordVal* ValueToRecordVal(const Stream* i, const threading::Value* const *vals, RecordType *request_type, int* position); Val* RecordValToIndexVal(RecordVal *r); diff --git a/src/input/ReaderBackend.cc b/src/input/ReaderBackend.cc index 72043c5932..685ada56aa 100644 --- a/src/input/ReaderBackend.cc +++ b/src/input/ReaderBackend.cc @@ -64,7 +64,7 @@ public: virtual bool Process() { - bool success = input_mgr->SendEvent(name, num_vals, val); + bool success = input_mgr->SendEvent(Object(), name, num_vals, val); if ( ! success ) reporter->Error("SendEvent for event %s failed", name); From c27848fc32c9cbeb08ded522c2fdc34c0805d747 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 19 Mar 2015 14:58:38 -0700 Subject: [PATCH 2/3] Change the way the input framework deals with values it cannot convert into BroVals (especially enums) Not we do not force an internal error anymore. Instead, we raise an normal error and set an error flag that signals to the top-level functions that the value could not be converted and should not be propagated to the Bro core. This sadly makes the already messy code even more messy - but since errors can happen in deeply nested data structures, the alternative (catching the error at every possible location and then trying to clean up there instead of recursively deleting the data that cannot be used later) is much worse. Addresses BIT-1199 --- src/input/Manager.cc | 318 +++++++++++------- src/input/Manager.h | 6 +- .../bro..stderr | 2 + .../bro..stdout | 4 + .../base/frameworks/input/missing-enum.bro | 37 ++ 5 files changed, 248 insertions(+), 119 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stderr create mode 100644 testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stdout create mode 100644 testing/btest/scripts/base/frameworks/input/missing-enum.bro diff --git a/src/input/Manager.cc b/src/input/Manager.cc index ac79257ad9..0de6efd782 100644 --- a/src/input/Manager.cc +++ b/src/input/Manager.cc @@ -971,7 +971,7 @@ Val* Manager::RecordValToIndexVal(RecordVal *r) } -Val* Manager::ValueToIndexVal(const Stream* i, int num_fields, const RecordType *type, const Value* const *vals) +Val* Manager::ValueToIndexVal(const Stream* i, int num_fields, const RecordType *type, const Value* const *vals, bool& have_error) { Val* idxval; int position = 0; @@ -979,7 +979,7 @@ Val* Manager::ValueToIndexVal(const Stream* i, int num_fields, const RecordType if ( num_fields == 1 && type->FieldType(0)->Tag() != TYPE_RECORD ) { - idxval = ValueToVal(i, vals[0], type->FieldType(0)); + idxval = ValueToVal(i, vals[0], type->FieldType(0), have_error); position = 1; } else @@ -989,10 +989,10 @@ Val* Manager::ValueToIndexVal(const Stream* i, int num_fields, const RecordType { if ( type->FieldType(j)->Tag() == TYPE_RECORD ) l->Append(ValueToRecordVal(i, vals, - type->FieldType(j)->AsRecordType(), &position)); + type->FieldType(j)->AsRecordType(), &position, have_error)); else { - l->Append(ValueToVal(i, vals[position], type->FieldType(j))); + l->Append(ValueToVal(i, vals[position], type->FieldType(j), have_error)); position++; } } @@ -1079,7 +1079,7 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) { // seen before if ( stream->num_val_fields == 0 || h->valhash == valhash ) - { + { // ok, exact duplicate, move entry to new dicrionary and do nothing else. stream->lastDict->Remove(idxhash); stream->currDict->Insert(idxhash, h); @@ -1094,8 +1094,8 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) stream->lastDict->Remove(idxhash); // keep h for predicates updated = true; - } + } Val* valval; @@ -1103,63 +1103,68 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) int position = stream->num_idx_fields; + bool convert_error = false; // this will be set to true by ValueTo* on Error + if ( stream->num_val_fields == 0 ) valval = 0; else if ( stream->num_val_fields == 1 && !stream->want_record ) - valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0)); + valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0), convert_error); else - valval = ValueToRecordVal(i, vals, stream->rtype, &position); - + valval = ValueToRecordVal(i, vals, stream->rtype, &position, convert_error); // call stream first to determine if we really add / change the entry - if ( stream->pred ) + if ( stream->pred && !convert_error ) { EnumVal* ev; int startpos = 0; - predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); + bool pred_convert_error = false; + predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, pred_convert_error); - if ( updated ) - ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, BifType::Enum::Input::Event); - else - ev = new EnumVal(BifEnum::Input::EVENT_NEW, BifType::Enum::Input::Event); - - bool result; - if ( stream->num_val_fields > 0 ) // we have values - result = CallPred(stream->pred, 3, ev, predidx->Ref(), valval->Ref()); - else // no values - result = CallPred(stream->pred, 2, ev, predidx->Ref()); - - if ( result == false ) + // if we encountered a convert error here - just continue as we would have without + // emitting the event. I do not really think that that can happen just here and not + // at the top-level. But - this is safe. + if ( !pred_convert_error ) { - Unref(predidx); - Unref(valval); - - if ( ! updated ) - { - // just quit and delete everything we created. - delete idxhash; - return stream->num_val_fields + stream->num_idx_fields; - } - + if ( updated ) + ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, BifType::Enum::Input::Event); else + ev = new EnumVal(BifEnum::Input::EVENT_NEW, BifType::Enum::Input::Event); + + bool result; + if ( stream->num_val_fields > 0 ) // we have values + result = CallPred(stream->pred, 3, ev, predidx->Ref(), valval->Ref()); + else // no values + result = CallPred(stream->pred, 2, ev, predidx->Ref()); + + if ( result == false ) { - // keep old one - stream->currDict->Insert(idxhash, h); - delete idxhash; - return stream->num_val_fields + stream->num_idx_fields; + Unref(predidx); + Unref(valval); + + if ( ! updated ) + { + // just quit and delete everything we created. + delete idxhash; + return stream->num_val_fields + stream->num_idx_fields; + } + + else + { + // keep old one + stream->currDict->Insert(idxhash, h); + delete idxhash; + return stream->num_val_fields + stream->num_idx_fields; + } } } + } // now we don't need h anymore - if we are here, the entry is updated and a new h is created. - if ( h ) - { - delete h; - h = 0; - } - + delete h; + h = 0; Val* idxval; if ( predidx != 0 ) @@ -1168,7 +1173,20 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) // I think there is an unref missing here. But if I insert is, it crashes :) } else - idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); + idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals, convert_error); + + if ( convert_error ) + { + // abort here and free everything that was allocated so far. + Unref(predidx); + Unref(valval); + Unref(idxval); + + delete idxhash; + return stream->num_val_fields + stream->num_idx_fields; + } + + assert(idxval); Val* oldval = 0; if ( updated == true ) @@ -1178,7 +1196,7 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) oldval = stream->tab->Lookup(idxval, false); } - assert(idxval); + HashKey* k = stream->tab->ComputeHash(idxval); if ( ! k ) reporter->InternalError("could not hash"); @@ -1203,16 +1221,21 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, convert_error); - if ( updated ) + if ( convert_error ) + { + // the only thing to clean up here is predidx. Everything else should + // already be ok again + Unref(predidx); + } + else if ( updated ) { // in case of update send back the old value. assert ( stream->num_val_fields > 0 ); ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, BifType::Enum::Input::Event); assert ( oldval != 0 ); SendEvent(stream->event, 4, stream->description->Ref(), ev, predidx, oldval); } - else { ev = new EnumVal(BifEnum::Input::EVENT_NEW, BifType::Enum::Input::Event); @@ -1223,7 +1246,6 @@ int Manager::SendEntryTable(Stream* i, const Value* const *vals) } else SendEvent(stream->event, 4, stream->description->Ref(), ev, predidx, valval->Ref()); - } } @@ -1416,7 +1438,6 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * assert(i->stream_type == EVENT_STREAM); EventStream* stream = (EventStream*) i; - Val *val; list out_vals; Ref(stream->description); out_vals.push_back(stream->description); @@ -1425,9 +1446,11 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * int position = 0; + bool convert_error = false; + if ( stream->want_record ) { - RecordVal * r = ValueToRecordVal(i, vals, stream->fields, &position); + RecordVal * r = ValueToRecordVal(i, vals, stream->fields, &position, convert_error); out_vals.push_back(r); } @@ -1440,11 +1463,11 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * if ( stream->fields->FieldType(j)->Tag() == TYPE_RECORD ) val = ValueToRecordVal(i, vals, stream->fields->FieldType(j)->AsRecordType(), - &position); + &position, convert_error); else { - val = ValueToVal(i, vals[position], stream->fields->FieldType(j)); + val = ValueToVal(i, vals[position], stream->fields->FieldType(j), convert_error); position++; } @@ -1452,7 +1475,14 @@ int Manager::SendEventStreamEvent(Stream* i, EnumVal* type, const Value* const * } } - SendEvent(stream->event, out_vals); + if ( convert_error ) + { + // we have an error somewhere in our out_vals. Just delete all of them. + for ( list::const_iterator it = out_vals.begin(), end = out_vals.end(); it != end; ++it ) + Unref(*it); + } + else + SendEvent(stream->event, out_vals); return stream->num_fields; } @@ -1464,7 +1494,9 @@ int Manager::PutTable(Stream* i, const Value* const *vals) assert(i->stream_type == TABLE_STREAM); TableStream* stream = (TableStream*) i; - Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); + bool convert_error = 0; + + Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals, convert_error); Val* valval; int position = stream->num_idx_fields; @@ -1473,9 +1505,16 @@ int Manager::PutTable(Stream* i, const Value* const *vals) valval = 0; else if ( stream->num_val_fields == 1 && stream->want_record == 0 ) - valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0)); + valval = ValueToVal(i, vals[position], stream->rtype->FieldType(0), convert_error); else - valval = ValueToRecordVal(i, vals, stream->rtype, &position); + valval = ValueToRecordVal(i, vals, stream->rtype, &position, convert_error); + + if ( convert_error ) + { + Unref(valval); + Unref(idxval); + return stream->num_idx_fields + stream->num_val_fields; + } // if we have a subscribed event, we need to figure out, if this is an update or not // same for predicates @@ -1503,31 +1542,37 @@ int Manager::PutTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); + bool pred_convert_error = false; + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, pred_convert_error); - if ( updated ) - ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, - BifType::Enum::Input::Event); + if ( pred_convert_error ) + Unref(predidx); else - ev = new EnumVal(BifEnum::Input::EVENT_NEW, - BifType::Enum::Input::Event); - - bool result; - if ( stream->num_val_fields > 0 ) // we have values { - Ref(valval); - result = CallPred(stream->pred, 3, ev, predidx, valval); - } - else // no values - result = CallPred(stream->pred, 2, ev, predidx); + if ( updated ) + ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, + BifType::Enum::Input::Event); + else + ev = new EnumVal(BifEnum::Input::EVENT_NEW, + BifType::Enum::Input::Event); - if ( result == false ) - { - // do nothing - Unref(idxval); - Unref(valval); - Unref(oldval); - return stream->num_val_fields + stream->num_idx_fields; + bool result; + if ( stream->num_val_fields > 0 ) // we have values + { + Ref(valval); + result = CallPred(stream->pred, 3, ev, predidx, valval); + } + else // no values + result = CallPred(stream->pred, 2, ev, predidx); + + if ( result == false ) + { + // do nothing + Unref(idxval); + Unref(valval); + Unref(oldval); + return stream->num_val_fields + stream->num_idx_fields; + } } } @@ -1538,28 +1583,34 @@ int Manager::PutTable(Stream* i, const Value* const *vals) { EnumVal* ev; int startpos = 0; - Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); + bool event_convert_error = false; + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, event_convert_error); - if ( updated ) - { - // in case of update send back the old value. - assert ( stream->num_val_fields > 0 ); - ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, - BifType::Enum::Input::Event); - assert ( oldval != 0 ); - SendEvent(stream->event, 4, stream->description->Ref(), - ev, predidx, oldval); - } + if ( event_convert_error ) + Unref(predidx); else { - ev = new EnumVal(BifEnum::Input::EVENT_NEW, - BifType::Enum::Input::Event); - if ( stream->num_val_fields == 0 ) + if ( updated ) + { + // in case of update send back the old value. + assert ( stream->num_val_fields > 0 ); + ev = new EnumVal(BifEnum::Input::EVENT_CHANGED, + BifType::Enum::Input::Event); + assert ( oldval != 0 ); SendEvent(stream->event, 4, stream->description->Ref(), - ev, predidx); + ev, predidx, oldval); + } else - SendEvent(stream->event, 4, stream->description->Ref(), - ev, predidx, valval->Ref()); + { + ev = new EnumVal(BifEnum::Input::EVENT_NEW, + BifType::Enum::Input::Event); + if ( stream->num_val_fields == 0 ) + SendEvent(stream->event, 4, stream->description->Ref(), + ev, predidx); + else + SendEvent(stream->event, 4, stream->description->Ref(), + ev, predidx, valval->Ref()); + } } } @@ -1612,29 +1663,42 @@ bool Manager::Delete(ReaderFrontend* reader, Value* *vals) if ( i->stream_type == TABLE_STREAM ) { TableStream* stream = (TableStream*) i; - Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals); + bool convert_error = false; + Val* idxval = ValueToIndexVal(i, stream->num_idx_fields, stream->itype, vals, convert_error); assert(idxval != 0); readVals = stream->num_idx_fields + stream->num_val_fields; bool streamresult = true; + if ( convert_error ) + { + Unref(idxval); + return false; + } + if ( stream->pred || stream->event ) { Val *val = stream->tab->Lookup(idxval); if ( stream->pred ) { - Ref(val); - EnumVal *ev = new EnumVal(BifEnum::Input::EVENT_REMOVED, BifType::Enum::Input::Event); int startpos = 0; - Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos); + Val* predidx = ValueToRecordVal(i, vals, stream->itype, &startpos, convert_error); - streamresult = CallPred(stream->pred, 3, ev, predidx, val); - - if ( streamresult == false ) + if ( convert_error ) + Unref(predidx); + else { - // keep it. - Unref(idxval); - success = true; + Ref(val); + EnumVal *ev = new EnumVal(BifEnum::Input::EVENT_REMOVED, BifType::Enum::Input::Event); + + streamresult = CallPred(stream->pred, 3, ev, predidx, val); + + if ( streamresult == false ) + { + // keep it. + Unref(idxval); + success = true; + } } } @@ -1739,13 +1803,22 @@ bool Manager::SendEvent(ReaderFrontend* reader, const string& name, const int nu return false; } + bool convert_error = false; + val_list* vl = new val_list; for ( int j = 0; j < num_vals; j++) - vl->append(ValueToVal(i, vals[j], type->FieldType(j))); - - mgr.QueueEvent(handler, vl, SOURCE_LOCAL); + vl->append(ValueToVal(i, vals[j], type->FieldType(j), convert_error)); delete_value_ptr_array(vals, num_vals); + + if ( convert_error ) + { + delete_vals(vl); + return false; + } + else + mgr.QueueEvent(handler, vl, SOURCE_LOCAL); + return true; } @@ -1817,7 +1890,7 @@ RecordVal* Manager::ListValToRecordVal(ListVal* list, RecordType *request_type, // Convert a threading value to a record value RecordVal* Manager::ValueToRecordVal(const Stream* stream, const Value* const *vals, - RecordType *request_type, int* position) + RecordType *request_type, int* position, bool& have_error) { assert(position != 0); // we need the pointer to point to data. @@ -1826,7 +1899,7 @@ RecordVal* Manager::ValueToRecordVal(const Stream* stream, const Value* const *v { Val* fieldVal = 0; if ( request_type->FieldType(i)->Tag() == TYPE_RECORD ) - fieldVal = ValueToRecordVal(stream, vals, request_type->FieldType(i)->AsRecordType(), position); + fieldVal = ValueToRecordVal(stream, vals, request_type->FieldType(i)->AsRecordType(), position, have_error); else if ( request_type->FieldType(i)->Tag() == TYPE_FILE || request_type->FieldType(i)->Tag() == TYPE_FUNC ) { @@ -1841,7 +1914,7 @@ RecordVal* Manager::ValueToRecordVal(const Stream* stream, const Value* const *v } else { - fieldVal = ValueToVal(stream, vals[*position], request_type->FieldType(i)); + fieldVal = ValueToVal(stream, vals[*position], request_type->FieldType(i), have_error); (*position)++; } @@ -2110,8 +2183,13 @@ HashKey* Manager::HashValues(const int num_elements, const Value* const *vals) } // convert threading value to Bro value -Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_type) +// have_error is a reference to a boolean which is set to true as soon as an error occured. +// When have_error is set to true at the beginning of the function, it is assumed that +// an error already occured in the past and processing is aborted. +Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_type, bool& have_error) { + if ( have_error ) + return 0; if ( request_type->Tag() != TYPE_ANY && request_type->Tag() != val->type ) { @@ -2198,9 +2276,10 @@ Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_typ TableVal* t = new TableVal(s); for ( int j = 0; j < val->val.set_val.size; j++ ) { - Val* assignval = ValueToVal(i, val->val.set_val.vals[j], type); + Val* assignval = ValueToVal(i, val->val.set_val.vals[j], type, have_error); + t->Assign(assignval, 0); - Unref(assignval); // idex is not consumed by assign. + Unref(assignval); // index is not consumed by assign. } Unref(s); @@ -2214,7 +2293,9 @@ Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_typ VectorType* vt = new VectorType(type->Ref()); VectorVal* v = new VectorVal(vt); for ( int j = 0; j < val->val.vector_val.size; j++ ) - v->Assign(j, ValueToVal(i, val->val.set_val.vals[j], type)); + { + v->Assign(j, ValueToVal(i, val->val.set_val.vals[j], type, have_error)); + } Unref(vt); return v; @@ -2233,9 +2314,14 @@ Val* Manager::ValueToVal(const Stream* i, const Value* val, BroType* request_typ // but well. bro_int_t index = request_type->AsEnumType()->Lookup(module, var.c_str()); if ( index == -1 ) - reporter->InternalError("Value not '%s' for stream '%s' is not a valid enum.", + { + reporter->Error("Value not '%s' for stream '%s' is not a valid enum.", enum_string.c_str(), i->name.c_str()); + have_error = true; + return 0; + } + return new EnumVal(index, request_type->Ref()->AsEnumType()); } diff --git a/src/input/Manager.h b/src/input/Manager.h index 551eeaecf3..d61ed3a485 100644 --- a/src/input/Manager.h +++ b/src/input/Manager.h @@ -205,14 +205,14 @@ private: // Convert Threading::Value to an internal Bro Type (works also with // Records). - Val* ValueToVal(const Stream* i, const threading::Value* val, BroType* request_type); + Val* ValueToVal(const Stream* i, const threading::Value* val, BroType* request_type, bool& have_error); // Convert Threading::Value to an internal Bro List type. - Val* ValueToIndexVal(const Stream* i, int num_fields, const RecordType* type, const threading::Value* const *vals); + Val* ValueToIndexVal(const Stream* i, int num_fields, const RecordType* type, const threading::Value* const *vals, bool& have_error); // Converts a threading::value to a record type. Mostly used by // ValueToVal. - RecordVal* ValueToRecordVal(const Stream* i, const threading::Value* const *vals, RecordType *request_type, int* position); + RecordVal* ValueToRecordVal(const Stream* i, const threading::Value* const *vals, RecordType *request_type, int* position, bool& have_error); Val* RecordValToIndexVal(RecordVal *r); diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stderr b/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stderr new file mode 100644 index 0000000000..c8e56d4c21 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stderr @@ -0,0 +1,2 @@ +error: Value not 'IdoNot::Exist' for stream 'enum' is not a valid enum. +received termination signal diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stdout b/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stdout new file mode 100644 index 0000000000..e760668629 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.frameworks.input.missing-enum/bro..stdout @@ -0,0 +1,4 @@ +Table: +{ + +} diff --git a/testing/btest/scripts/base/frameworks/input/missing-enum.bro b/testing/btest/scripts/base/frameworks/input/missing-enum.bro new file mode 100644 index 0000000000..0d37aae453 --- /dev/null +++ b/testing/btest/scripts/base/frameworks/input/missing-enum.bro @@ -0,0 +1,37 @@ +# @TEST-EXEC: btest-bg-run bro bro -b %INPUT +# @TEST-EXEC: btest-bg-wait 10 +# @TEST-EXEC: btest-diff bro/.stderr +# @TEST-EXEC: btest-diff bro/.stdout + +@TEST-START-FILE input.log +#fields e i +IdoNot::Exist 1 +@TEST-END-FILE + +redef exit_only_after_terminate = T; + +module A; + +type Idx: record { + i: int; +}; + +type Val: record { + e: Log::ID; +}; + +global etable: table[int] of Log::ID = table(); + +event bro_init() + { + # first read in the old stuff into the table... + Input::add_table([$source="../input.log", $name="enum", $idx=Idx, $val=Val, $destination=etable, $want_record=F]); + } + +event Input::end_of_data(name: string, source:string) + { + print "Table:"; + print etable; + Input::remove("enum"); + terminate(); + } From 1f33dd0c381a2ae3a6aeb38e5c61ac417eef038e Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 19 Mar 2015 15:59:49 -0700 Subject: [PATCH 3/3] add a basic leak test for an unparseable enum --- .../btest/core/leaks/input-missing-enum.bro | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 testing/btest/core/leaks/input-missing-enum.bro diff --git a/testing/btest/core/leaks/input-missing-enum.bro b/testing/btest/core/leaks/input-missing-enum.bro new file mode 100644 index 0000000000..66ecc7ae56 --- /dev/null +++ b/testing/btest/core/leaks/input-missing-enum.bro @@ -0,0 +1,41 @@ +# Needs perftools support. +# +# @TEST-GROUP: leaks +# +# @TEST-REQUIRES: bro --help 2>&1 | grep -q mem-leaks +# +# @TEST-EXEC: HEAP_CHECK_DUMP_DIRECTORY=. HEAPCHECK=local btest-bg-run bro bro -m -b %INPUT +# @TEST-EXEC: btest-bg-wait 15 + +@TEST-START-FILE input.log +#fields e i +IdoNot::Exist 1 +@TEST-END-FILE + +redef exit_only_after_terminate = T; + +module A; + +type Idx: record { + i: int; +}; + +type Val: record { + e: Log::ID; +}; + +global etable: table[int] of Log::ID = table(); + +event bro_init() + { + # first read in the old stuff into the table... + Input::add_table([$source="../input.log", $name="enum", $idx=Idx, $val=Val, $destination=etable, $want_record=F]); + } + +event Input::end_of_data(name: string, source:string) + { + print "Table:"; + print etable; + Input::remove("enum"); + terminate(); + }