From b6e6302b4031d36d6e297b25aba444b0dc26f1e8 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Fri, 3 Mar 2017 12:40:37 -0800 Subject: [PATCH] Ascii reader error changes - fix small bugs The changes are now a bit more succinct with less code changes required. Behavior is tested a little bit more thoroughly and a memory problem when reading incomplete lines was fixed. ReadHeader also always directly returns if header reading failed. Error messages now are back to what they were before the change, if the new behavior is not used. I also tweaked the documentation text a bit. --- .../base/frameworks/input/readers/ascii.bro | 34 ++++++-- src/input/readers/ascii/Ascii.cc | 85 ++++++++++--------- src/input/readers/ascii/Ascii.h | 10 ++- .../out | 26 ++++++ .../bro..stderr | 4 + .../bro..stdout | 3 + .../bro..stderr | 4 +- .../base/frameworks/input/invalid-lines.bro | 67 +++++++++++++++ .../base/frameworks/input/invalidtext.bro | 1 + .../input/missing-file-initially.bro | 12 +-- .../base/frameworks/input/missing-file.bro | 1 + 11 files changed, 187 insertions(+), 60 deletions(-) create mode 100644 testing/btest/Baseline/scripts.base.frameworks.input.invalid-lines/out create mode 100644 testing/btest/scripts/base/frameworks/input/invalid-lines.bro diff --git a/scripts/base/frameworks/input/readers/ascii.bro b/scripts/base/frameworks/input/readers/ascii.bro index 521e8d4ca3..83d8ced94b 100644 --- a/scripts/base/frameworks/input/readers/ascii.bro +++ b/scripts/base/frameworks/input/readers/ascii.bro @@ -19,16 +19,32 @@ export { ## String to use for an unset &optional field. const unset_field = Input::unset_field &redef; - ## Choose if the ascii input reader should globally - ## fail on invalid lines and continue parsing afterward. - ## Individual readers can use a different value. + ## Fail on invalid lines. If set to false, the ascii + ## input reader will jump over invalid lines, reporting + ## warnings in reporter.log. If set to true, errors in + ## input lines will be handled as fatal errors for the + ## reader thread; reading will abort immediately and + ## an error will be logged to reporter.log. + ## Invidivual readers can use a different value using + ## the $config table. + ## fail_on_invalid_lines = T was the default behavior + ## untill Bro 2.5. const fail_on_invalid_lines = F &redef; - ## Set to true if you would like the old behavior of the - ## ascii reader where the reader thread would die if any file - ## errors occur (like permissions problems or file missing). - ## The default behavior is to continue attempting to open and read - ## the file even in light of problems. - ## Individual readers can use a different value. + ## Fail on file read problems. If set to true, the ascii + ## input reader will fail when encountering any problems + ## while reading a file different from invalid lines. + ## Examples fur such problems are permission problems, or + ## missing files. + ## When set to false, these problems will be ignored. This + ## has an especially big effect for the REREAD mode, which will + ## seamlessly recover from read errors when a file is + ## only temporarily inaccessible. For MANUAL or STREAM files, + ## errors will most likely still be fatal since no automatic + ## re-reading of the file is attempted. + ## Invidivual readers can use a different value using + ## the $config table. + ## fail_on_file_problem = T was the default behavior + ## untill Bro 2.5. const fail_on_file_problem = F &redef; } diff --git a/src/input/readers/ascii/Ascii.cc b/src/input/readers/ascii/Ascii.cc index 5621a87eb4..bedddce075 100644 --- a/src/input/readers/ascii/Ascii.cc +++ b/src/input/readers/ascii/Ascii.cc @@ -61,7 +61,7 @@ void Ascii::DoClose() bool Ascii::DoInit(const ReaderInfo& info, int num_fields, const Field* const* fields) { - is_failed = false; + suppress_warnings = false; separator.assign( (const char*) BifConst::InputAscii::separator->Bytes(), BifConst::InputAscii::separator->Len()); @@ -109,43 +109,48 @@ bool Ascii::DoInit(const ReaderInfo& info, int num_fields, const Field* const* f formatter::Ascii::SeparatorInfo sep_info(separator, set_separator, unset_field, empty_field); formatter = unique_ptr(new formatter::Ascii(this, sep_info)); - DoUpdate(); - - return true; + return DoUpdate(); } -void Ascii::FailWarn(bool is_error, const char *msg) +void Ascii::FailWarn(bool is_error, const char *msg, bool suppress_future) { if ( is_error ) Error(msg); else - Warning(msg); + { + // suppress error message when we are already in error mode. + // There is no reason to repeat it every second. + if ( ! suppress_warnings ) + Warning(msg); + + if ( suppress_future ) + suppress_warnings = true; + } } bool Ascii::OpenFile() { - if ( file.is_open() && ! is_failed ) + if ( file.is_open() ) return true; file.open(Info().source); + if ( ! file.is_open() ) { - if ( ! is_failed ) - FailWarn(fail_on_file_problem, Fmt("Init: cannot open %s", Info().source)); - is_failed = true; - return !fail_on_file_problem; + FailWarn(fail_on_file_problem, Fmt("Init: cannot open %s", Info().source), true); + + return ! fail_on_file_problem; } if ( ReadHeader(false) == false ) { - if ( ! is_failed ) - FailWarn(fail_on_file_problem, Fmt("Init: cannot open %s; headers are incorrect", Info().source)); + FailWarn(fail_on_file_problem, Fmt("Init: cannot open %s; problem reading file header", Info().source), true); + file.close(); - is_failed = true; - return !fail_on_file_problem; + return ! fail_on_file_problem; } - is_failed = false; + suppress_warnings = false; return true; } @@ -158,7 +163,11 @@ bool Ascii::ReadHeader(bool useCached) if ( ! useCached ) { if ( ! GetLine(line) ) + { + FailWarn(fail_on_file_problem, Fmt("Could not read input data file %s; first line could not be read", + Info().source), true); return false; + } headerline = line; } @@ -198,12 +207,10 @@ bool Ascii::ReadHeader(bool useCached) continue; } - if ( ! is_failed ) - FailWarn(fail_on_file_problem, Fmt("Did not find requested field %s in input data file %s.", - field->name, Info().source)); + FailWarn(fail_on_file_problem, Fmt("Did not find requested field %s in input data file %s.", + field->name, Info().source), true); - is_failed = true; - return !fail_on_file_problem; + return false; } FieldMapping f(field->name, field->type, field->subtype, ifields[field->name]); @@ -213,12 +220,10 @@ bool Ascii::ReadHeader(bool useCached) map::iterator fit2 = ifields.find(field->secondary_name); if ( fit2 == ifields.end() ) { - if ( ! is_failed ) - FailWarn(fail_on_file_problem, Fmt("Could not find requested port type field %s in input data file.", - field->secondary_name)); + FailWarn(fail_on_file_problem, Fmt("Could not find requested port type field %s in input data file.", + field->secondary_name), true); - is_failed = true; - return !fail_on_file_problem; + return false; } f.secondary_position = ifields[field->secondary_name]; @@ -258,7 +263,7 @@ bool Ascii::GetLine(string& str) bool Ascii::DoUpdate() { if ( ! OpenFile() ) - return !fail_on_file_problem; + return ! fail_on_file_problem; switch ( Info().mode ) { case MODE_REREAD: @@ -267,11 +272,10 @@ bool Ascii::DoUpdate() struct stat sb; if ( stat(Info().source, &sb) == -1 ) { - if ( ! is_failed ) - FailWarn(fail_on_file_problem, Fmt("Could not get stat for %s", Info().source)); + FailWarn(fail_on_file_problem, Fmt("Could not get stat for %s", Info().source), true); + file.close(); - is_failed = true; - return !fail_on_file_problem; + return ! fail_on_file_problem; } if ( sb.st_mtime <= mtime ) // no change @@ -293,10 +297,9 @@ bool Ascii::DoUpdate() if ( Info().mode == MODE_STREAM ) { file.clear(); // remove end of file evil bits - if ( !ReadHeader(true) ) + if ( ! ReadHeader(true) ) { - is_failed = true; - return !fail_on_file_problem; // header reading failed + return ! fail_on_file_problem; // header reading failed } break; @@ -360,15 +363,15 @@ bool Ascii::DoUpdate() if ( (*fit).position > pos || (*fit).secondary_position > pos ) { FailWarn(fail_on_invalid_lines, Fmt("Not enough fields in line %s. Found %d fields, want positions %d and %d", - line.c_str(), pos, (*fit).position, (*fit).secondary_position)); - - for ( int i = 0; i < fpos; i++ ) - delete fields[i]; - - delete [] fields; + line.c_str(), pos, (*fit).position, (*fit).secondary_position)); if ( fail_on_invalid_lines ) { + for ( int i = 0; i < fpos; i++ ) + delete fields[i]; + + delete [] fields; + return false; } else @@ -432,7 +435,7 @@ bool Ascii::DoUpdate() bool Ascii::DoHeartbeat(double network_time, double current_time) { if ( ! OpenFile() ) - return !fail_on_file_problem; + return ! fail_on_file_problem; switch ( Info().mode ) { diff --git a/src/input/readers/ascii/Ascii.h b/src/input/readers/ascii/Ascii.h index 9300382ca2..7a7fa52590 100644 --- a/src/input/readers/ascii/Ascii.h +++ b/src/input/readers/ascii/Ascii.h @@ -56,8 +56,10 @@ private: bool ReadHeader(bool useCached); bool GetLine(string& str); bool OpenFile(); - void FailWarn(bool is_error, const char *msg); - + // Call Warning or Error, depending on the is_error boolean. + // In case of a warning, setting suppress_future to true will suppress all future warnings + // (by setting suppress_warnings to true, until suppress_warnings is set back to false) + void FailWarn(bool is_error, const char *msg, bool suppress_future = false); ifstream file; time_t mtime; @@ -77,8 +79,8 @@ private: bool fail_on_file_problem; // this is an internal indicator in case the read is currently in a failed state - // it's used by the options for continuing instead of failing and killing the reader. - bool is_failed; + // it's used to suppress duplicate error messages. + bool suppress_warnings; std::unique_ptr formatter; }; diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.invalid-lines/out b/testing/btest/Baseline/scripts.base.frameworks.input.invalid-lines/out new file mode 100644 index 0000000000..3406639d29 --- /dev/null +++ b/testing/btest/Baseline/scripts.base.frameworks.input.invalid-lines/out @@ -0,0 +1,26 @@ +{ +[-43] = [b=T, e=SSH::LOG, c=21, p=123/unknown, sn=10.0.0.0/24, a=1.2.3.4, d=3.14, t=1315801931.273616, iv=100.0, s=hurz, ns=4242 HOHOHO, sc={ +2, +4, +1, +3 +}, ss={ +BB, +AA, +CC +}, se={ + +}, vc=[10, 20, 30], ve=[]], +[-42] = [b=T, e=SSH::LOG, c=21, p=123/unknown, sn=10.0.0.0/24, a=1.2.3.4, d=3.14, t=1315801931.273616, iv=100.0, s=hurz, ns=4242, sc={ +2, +4, +1, +3 +}, ss={ +BB, +AA, +CC +}, se={ + +}, vc=[10, 20, 30], ve=[]] +} diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stderr b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stderr index 6c4d0f0194..337cdcda87 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stderr +++ b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stderr @@ -1,4 +1,8 @@ warning: ../does-not-exist.dat/Input::READER_ASCII: Init: cannot open ../does-not-exist.dat +warning: ../does-not-exist.dat/Input::READER_ASCII: Init: cannot open ../does-not-exist.dat +warning: ../does-not-exist.dat/Input::READER_ASCII: Init: cannot open ../does-not-exist.dat error: ../does-not-exist.dat/Input::READER_ASCII: Init: cannot open ../does-not-exist.dat +error: ../does-not-exist.dat/Input::READER_ASCII: Init failed error: ../does-not-exist.dat/Input::READER_ASCII: terminating thread +warning: ../does-not-exist.dat/Input::READER_ASCII: Could not get stat for ../does-not-exist.dat received termination signal diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stdout b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stdout index d96c6d457d..39309bc8cf 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stdout +++ b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file-initially/bro..stdout @@ -1,2 +1,5 @@ now it does and more! +now it does +and more! +Streaming still works diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file/bro..stderr b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file/bro..stderr index f5d074fe7e..5093925d2d 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.input.missing-file/bro..stderr +++ b/testing/btest/Baseline/scripts.base.frameworks.input.missing-file/bro..stderr @@ -1,2 +1,4 @@ -warning: does-not-exist.dat/Input::READER_ASCII: Init: cannot open does-not-exist.dat +error: does-not-exist.dat/Input::READER_ASCII: Init: cannot open does-not-exist.dat +error: does-not-exist.dat/Input::READER_ASCII: Init failed +error: does-not-exist.dat/Input::READER_ASCII: terminating thread received termination signal diff --git a/testing/btest/scripts/base/frameworks/input/invalid-lines.bro b/testing/btest/scripts/base/frameworks/input/invalid-lines.bro new file mode 100644 index 0000000000..83be1efd09 --- /dev/null +++ b/testing/btest/scripts/base/frameworks/input/invalid-lines.bro @@ -0,0 +1,67 @@ +# @TEST-EXEC: btest-bg-run bro bro -b %INPUT +# @TEST-EXEC: btest-bg-wait 10 +# @TEST-EXEC: btest-diff out + +redef exit_only_after_terminate = T; +redef InputAscii::fail_on_invalid_lines = F; + +@TEST-START-FILE input.log +#separator \x09 +#path ssh +#fields b i e c p sn a d t iv s sc ss se vc ve ns +#types bool int enum count port subnet addr double time interval string table table table vector vector string +T -42 SSH::LOG 21 123 10.0.0.0/24 1.2.3.4 3.14 1315801931.273616 100.000000 hurz 2,4,1,3 CC,AA,BB EMPTY 10,20,30 +T -42 SSH::LOG 21 123 10.0.0.0/24 1.2.3.4 3.14 1315801931.273616 100.000000 hurz 2,4,1,3 CC,AA,BB EMPTY 10,20,30 EMPTY 4242 +T -43 SSH::LOG 21 123 10.0.0.0/24 1.2.3.4 3.14 1315801931.273616 100.000000 hurz 2,4,1,3 CC,AA,BB EMPTY 10,20,30 EMPTY 4242 HOHOHO +T -41 +@TEST-END-FILE + +@load base/protocols/ssh + +global outfile: file; + +redef InputAscii::empty_field = "EMPTY"; + +module A; + +type Idx: record { + i: int; +}; + +type Val: record { + b: bool; + e: Log::ID; + c: count; + p: port; + sn: subnet; + a: addr; + d: double; + t: time; + iv: interval; + s: string; + ns: string; + sc: set[count]; + ss: set[string]; + se: set[string]; + vc: vector of int; + ve: vector of int; +}; + +global servers: table[int] of Val = table(); +global servers2: table[int] of Val = table(); + +event bro_init() + { + outfile = open("../out"); + # first read in the old stuff into the table... + Input::add_table([$source="../input.log", $name="ssh", $idx=Idx, $val=Val, $destination=servers]); + Input::add_table([$source="../input.log", $name="ssh2", $idx=Idx, $val=Val, $destination=servers2, $config=table(["fail_on_invalid_lines"] = "T")]); + } + +event Input::end_of_data(name: string, source:string) + { + print outfile, servers; + Input::remove("ssh"); + close(outfile); + terminate(); + } diff --git a/testing/btest/scripts/base/frameworks/input/invalidtext.bro b/testing/btest/scripts/base/frameworks/input/invalidtext.bro index 1de4e96671..3f5b590dec 100644 --- a/testing/btest/scripts/base/frameworks/input/invalidtext.bro +++ b/testing/btest/scripts/base/frameworks/input/invalidtext.bro @@ -13,6 +13,7 @@ @TEST-END-FILE redef exit_only_after_terminate = T; +redef InputAscii::fail_on_invalid_lines = T; global outfile: file; diff --git a/testing/btest/scripts/base/frameworks/input/missing-file-initially.bro b/testing/btest/scripts/base/frameworks/input/missing-file-initially.bro index a7128a4455..73fd57284e 100644 --- a/testing/btest/scripts/base/frameworks/input/missing-file-initially.bro +++ b/testing/btest/scripts/base/frameworks/input/missing-file-initially.bro @@ -1,10 +1,12 @@ # This tests files that don't exist initially and then do later during # runtime to make sure the ascii reader is resilient to files missing. -# It does a second test at the same time which configures the old +# It does a second test at the same time which configures the old # failing behavior. # @TEST-EXEC: btest-bg-run bro bro %INPUT -# @TEST-EXEC: btest-bg-wait -k 5 +# @TEST-EXEC: sleep 2; cp does-exist.dat does-not-exist.dat +# @TEST-EXEC: sleep 2; mv does-not-exist.dat does-not-exist-again.dat; echo "Streaming still works" >> does-not-exist-again.dat +# @TEST-EXEC: btest-bg-wait -k 3 # @TEST-EXEC: btest-diff bro/.stdout # @TEST-EXEC: btest-diff bro/.stderr @@ -40,8 +42,8 @@ event line2(description: Input::EventDescription, tpe: Input::Event, v: Val) event bro_init() { Input::add_event([$source="../does-not-exist.dat", $name="input", $reader=Input::READER_ASCII, $mode=Input::REREAD, $fields=Val, $ev=line, $want_record=T]); - Input::add_event([$source="../does-not-exist.dat", $name="input2", $reader=Input::READER_ASCII, $mode=Input::REREAD, $fields=Val, $ev=line2, $want_record=T, + Input::add_event([$source="../does-not-exist.dat", $name="inputstream", $reader=Input::READER_ASCII, $mode=Input::STREAM, $fields=Val, $ev=line, $want_record=T]); + Input::add_event([$source="../does-not-exist.dat", $name="inputmanual", $reader=Input::READER_ASCII, $mode=Input::MANUAL, $fields=Val, $ev=line, $want_record=T]); + Input::add_event([$source="../does-not-exist.dat", $name="input2", $reader=Input::READER_ASCII, $mode=Input::REREAD, $fields=Val, $ev=line2, $want_record=T, $config=table(["fail_on_file_problem"] = "T")]); - - system("sleep 2; mv ../does-exist.dat ../does-not-exist.dat;"); } diff --git a/testing/btest/scripts/base/frameworks/input/missing-file.bro b/testing/btest/scripts/base/frameworks/input/missing-file.bro index 08adfe2150..2ec3bb937f 100644 --- a/testing/btest/scripts/base/frameworks/input/missing-file.bro +++ b/testing/btest/scripts/base/frameworks/input/missing-file.bro @@ -3,6 +3,7 @@ # @TEST-EXEC: btest-diff bro/.stderr redef exit_only_after_terminate = T; +redef InputAscii::fail_on_file_problem = T; global outfile: file; global try: count;