diff --git a/CHANGES b/CHANGES index 2e1a09e78b..d91a2e4700 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,10 @@ +2.2-341 | 2014-04-17 18:01:01 -0500 + + * Refactor initialization of ASCII log writer options. (Jon Siwek) + + * Fix a memory leak in ASCII log writer. (Jon Siwek) + 2.2-338 | 2014-04-17 17:48:17 -0500 * Disable input/logging threads setting their names on every diff --git a/VERSION b/VERSION index 5a0e87dbb5..85d7fee81b 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.2-338 +2.2-341 diff --git a/src/logging/writers/Ascii.cc b/src/logging/writers/Ascii.cc index b926a6c55b..43ffe47308 100644 --- a/src/logging/writers/Ascii.cc +++ b/src/logging/writers/Ascii.cc @@ -24,43 +24,13 @@ Ascii::Ascii(WriterFrontend* frontend) : WriterBackend(frontend) tsv = false; use_json = false; formatter = 0; + + InitConfigOptions(); + init_options = InitFilterOptions(); } -Ascii::~Ascii() +void Ascii::InitConfigOptions() { - if ( ! ascii_done ) - { - fprintf(stderr, "internal error: finish missing\n"); - abort(); - } - - delete formatter; - } - -bool Ascii::WriteHeaderField(const string& key, const string& val) - { - string str = meta_prefix + key + separator + val + "\n"; - - return safe_write(fd, str.c_str(), str.length()); - } - -void Ascii::CloseFile(double t) - { - if ( ! fd ) - return; - - if ( include_meta && ! tsv ) - WriteHeaderField("close", Timestamp(0)); - - safe_close(fd); - fd = 0; - } - -bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * fields) - { - assert(! fd); - - // Set some default values. output_to_stdout = BifConst::LogAscii::output_to_stdout; include_meta = BifConst::LogAscii::include_meta; use_json = BifConst::LogAscii::use_json; @@ -96,9 +66,15 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * (const char*) tsfmt.Bytes(), tsfmt.Len() ); + } + +bool Ascii::InitFilterOptions() + { + const WriterInfo& info = Info(); // Set per-filter configuration options. - for ( WriterInfo::config_map::const_iterator i = info.config.begin(); i != info.config.end(); i++ ) + for ( WriterInfo::config_map::const_iterator i = info.config.begin(); + i != info.config.end(); ++i ) { if ( strcmp(i->first, "tsv") == 0 ) { @@ -158,6 +134,17 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * json_timestamps.assign(i->second); } + if ( ! InitFormatter() ) + return false; + + return true; + } + +bool Ascii::InitFormatter() + { + delete formatter; + formatter = 0; + if ( use_json ) { formatter::JSON::TimeFormat tf = formatter::JSON::TS_EPOCH; @@ -179,7 +166,6 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * // Using JSON implicitly turns off the header meta fields. include_meta = false; } - else { // Use the default "Bro logs" format. @@ -189,6 +175,46 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * formatter = new formatter::Ascii(this, sep_info); } + return true; + } + +Ascii::~Ascii() + { + if ( ! ascii_done ) + { + fprintf(stderr, "internal error: finish missing\n"); + abort(); + } + + delete formatter; + } + +bool Ascii::WriteHeaderField(const string& key, const string& val) + { + string str = meta_prefix + key + separator + val + "\n"; + + return safe_write(fd, str.c_str(), str.length()); + } + +void Ascii::CloseFile(double t) + { + if ( ! fd ) + return; + + if ( include_meta && ! tsv ) + WriteHeaderField("close", Timestamp(0)); + + safe_close(fd); + fd = 0; + } + +bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * fields) + { + assert(! fd); + + if ( ! init_options ) + return false; + string path = info.path; if ( output_to_stdout ) @@ -206,58 +232,65 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const * return false; } - if ( include_meta ) + if ( ! WriteHeader(path) ) { - string names; - string types; - - for ( int i = 0; i < num_fields; ++i ) - { - if ( i > 0 ) - { - names += separator; - types += separator; - } - - names += string(fields[i]->name); - types += fields[i]->TypeName().c_str(); - } - - if ( tsv ) - { - // A single TSV-style line is all we need. - string str = names + "\n"; - if ( ! safe_write(fd, str.c_str(), str.length()) ) - goto write_error; - - return true; - } - - string str = meta_prefix - + "separator " // Always use space as separator here. - + get_escaped_string(separator, false) - + "\n"; - - if ( ! safe_write(fd, str.c_str(), str.length()) ) - goto write_error; - - if ( ! (WriteHeaderField("set_separator", get_escaped_string(set_separator, false)) && - WriteHeaderField("empty_field", get_escaped_string(empty_field, false)) && - WriteHeaderField("unset_field", get_escaped_string(unset_field, false)) && - WriteHeaderField("path", get_escaped_string(path, false)) && - WriteHeaderField("open", Timestamp(0))) ) - goto write_error; - - if ( ! (WriteHeaderField("fields", names) - && WriteHeaderField("types", types)) ) - goto write_error; + Error(Fmt("error writing to %s: %s", fname.c_str(), Strerror(errno))); + return false; } return true; + } -write_error: - Error(Fmt("error writing to %s: %s", fname.c_str(), Strerror(errno))); - return false; +bool Ascii::WriteHeader(const string& path) + { + if ( ! include_meta ) + return true; + + string names; + string types; + + for ( int i = 0; i < NumFields(); ++i ) + { + if ( i > 0 ) + { + names += separator; + types += separator; + } + + names += string(Fields()[i]->name); + types += Fields()[i]->TypeName().c_str(); + } + + if ( tsv ) + { + // A single TSV-style line is all we need. + string str = names + "\n"; + if ( ! safe_write(fd, str.c_str(), str.length()) ) + return false; + + return true; + } + + string str = meta_prefix + + "separator " // Always use space as separator here. + + get_escaped_string(separator, false) + + "\n"; + + if ( ! safe_write(fd, str.c_str(), str.length()) ) + return false; + + if ( ! (WriteHeaderField("set_separator", get_escaped_string(set_separator, false)) && + WriteHeaderField("empty_field", get_escaped_string(empty_field, false)) && + WriteHeaderField("unset_field", get_escaped_string(unset_field, false)) && + WriteHeaderField("path", get_escaped_string(path, false)) && + WriteHeaderField("open", Timestamp(0))) ) + return false; + + if ( ! (WriteHeaderField("fields", names) && + WriteHeaderField("types", types)) ) + return false; + + return true; } bool Ascii::DoFlush(double network_time) @@ -294,7 +327,7 @@ bool Ascii::DoWrite(int num_fields, const Field* const * fields, if ( ! formatter->Describe(&desc, num_fields, fields, vals) ) return false; - desc.AddRaw("\n"); + desc.AddRaw("\n", 1); const char* bytes = (const char*)desc.Bytes(); int len = desc.Len(); diff --git a/src/logging/writers/Ascii.h b/src/logging/writers/Ascii.h index 15afdef62f..54402cc141 100644 --- a/src/logging/writers/Ascii.h +++ b/src/logging/writers/Ascii.h @@ -34,9 +34,13 @@ protected: private: bool IsSpecial(string path) { return path.find("/dev/") == 0; } + bool WriteHeader(const string& path); bool WriteHeaderField(const string& key, const string& value); void CloseFile(double t); string Timestamp(double t); // Uses current time if t is zero. + void InitConfigOptions(); + bool InitFilterOptions(); + bool InitFormatter(); int fd; string fname; @@ -58,6 +62,7 @@ private: string json_timestamps; threading::formatter::Formatter* formatter; + bool init_options; }; } diff --git a/testing/btest/core/leaks/ascii-log-rotation.bro b/testing/btest/core/leaks/ascii-log-rotation.bro new file mode 100644 index 0000000000..a84f80ea90 --- /dev/null +++ b/testing/btest/core/leaks/ascii-log-rotation.bro @@ -0,0 +1,75 @@ +# Needs perftools support. +# +# @TEST-SERIALIZE: comm +# @TEST-GROUP: leaks +# +# @TEST-REQUIRES: bro --help 2>&1 | grep -q mem-leaks +# +# @TEST-EXEC: btest-bg-run receiver HEAP_CHECK_DUMP_DIRECTORY=. HEAPCHECK=local bro -b -m ../receiver.bro +# @TEST-EXEC: sleep 1 +# @TEST-EXEC: btest-bg-run sender HEAP_CHECK_DUMP_DIRECTORY=. HEAPCHECK=local bro -b -m ../sender.bro +# @TEST-EXEC: sleep 1 +# @TEST-EXEC: btest-bg-wait 60 + +@TEST-START-FILE sender.bro + +@load base/frameworks/communication +@load base/protocols/dns + +redef Communication::nodes += { + ["foo"] = [$host = 127.0.0.1, $connect=T] +}; + +global write_count: count = 0; + +event do_write() + { + print "do_write"; + local cid: conn_id = conn_id($orig_h=1.2.3.4,$orig_p=1/tcp, + $resp_h=5.6.7.8,$resp_p=2/tcp); + local dns_info_dummy = DNS::Info($ts=network_time(), $uid="FAKE", + $id=cid, $proto=tcp); + Log::write(DNS::LOG, dns_info_dummy); + schedule .1sec { do_write() }; + ++write_count; + + if ( write_count == 200 ) + terminate(); + } + +event remote_connection_handshake_done(p: event_peer) + { + print "remote_connection_handshake_done", p; + schedule .1sec { do_write() }; + } + +event remote_connection_closed(p: event_peer) + { + print "remote_connection_closed", p; + } + +@TEST-END-FILE + +@TEST-START-FILE receiver.bro + +@load frameworks/communication/listen +@load base/protocols/dns + +redef Communication::nodes += { + ["foo"] = [$host = 127.0.0.1, $connect=F, $request_logs=T] +}; + +redef Log::default_rotation_interval = 2sec; + +event remote_connection_handshake_done(p: event_peer) + { + print "remote_connection_handshake_done", p; + } + +event remote_connection_closed(p: event_peer) + { + print "remote_connection_closed", p; + terminate(); + } + +@TEST-END-FILE