Merge remote-tracking branch 'origin/topic/jsiwek/ascii-log-memleak-fix'

* origin/topic/jsiwek/ascii-log-memleak-fix:
  Refactor initialization of ASCII log writer options.
  Fix a memory leak in ASCII log writer.

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

VERSION

@@ -1 +1 @@
-2.2-338
+2.2-341

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,12 +232,24 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const *
 		return false;
 		}
 
-	if ( include_meta )
+	if ( ! WriteHeader(path) )
 		{
+		Error(Fmt("error writing to %s: %s", fname.c_str(), Strerror(errno)));
+		return false;
+		}
+
+	return true;
+	}
+
+bool Ascii::WriteHeader(const string& path)
+	{
+	if ( ! include_meta )
+		return true;
+
 	string names;
 	string types;
 
-		for ( int i = 0; i < num_fields; ++i )
+	for ( int i = 0; i < NumFields(); ++i )
 		{
 		if ( i > 0 )
 			{
@@ -219,8 +257,8 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const *
 			types += separator;
 			}
 
-			names += string(fields[i]->name);
+		names += string(Fields()[i]->name);
-			types += fields[i]->TypeName().c_str();
+		types += Fields()[i]->TypeName().c_str();
 		}
 
 	if ( tsv )
@@ -228,7 +266,7 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const *
 		// 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 false;
 
 		return true;
 		}
@@ -239,25 +277,20 @@ bool Ascii::DoInit(const WriterInfo& info, int num_fields, const Field* const *
 		+ "\n";
 
 	if ( ! safe_write(fd, str.c_str(), str.length()) )
-			goto write_error;
+		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))) )
-			goto write_error;
+		return false;
 
-		if ( ! (WriteHeaderField("fields", names)
+	if ( ! (WriteHeaderField("fields", names) &&
-			&& WriteHeaderField("types", types)) )
+	        WriteHeaderField("types", types)) )
-			goto write_error;
+		return false;
-		}
 
 	return true;
-
-write_error:
-	Error(Fmt("error writing to %s: %s", fname.c_str(), Strerror(errno)));
-	return false;
 	}
 
 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();

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;
 };
 
 }

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