diff --git a/CHANGES b/CHANGES index 5ac92e4596..8218d47ae0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,15 @@ +2.2-262 | 2014-03-30 20:12:47 +0200 + + * Refactor SerializationFormat::EndWrite and ChunkedIO::Chunk memory + management. (Jon Siwek) + + * Improve SerializationFormat's write buffer growth strategy. (Jon + Siwek) + + * Add --parse-only option to exit after parsing scripts. May be + useful for syntax-checking tools. (Jon Siwek) + 2.2-256 | 2014-03-30 19:57:28 +0200 * For the summary statistics framewirk, change all &create_expire diff --git a/VERSION b/VERSION index 5d578e2eb7..b5309fe89c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.2-256 +2.2-262 diff --git a/src/ChunkedIO.cc b/src/ChunkedIO.cc index 22489bbb0c..54e2e59575 100644 --- a/src/ChunkedIO.cc +++ b/src/ChunkedIO.cc @@ -127,12 +127,7 @@ ChunkedIOFd::~ChunkedIOFd() delete [] read_buffer; delete [] write_buffer; safe_close(fd); - - if ( partial ) - { - delete [] partial->data; - delete partial; - } + delete partial; } bool ChunkedIOFd::Write(Chunk* chunk) @@ -169,10 +164,9 @@ bool ChunkedIOFd::Write(Chunk* chunk) while ( left ) { - Chunk* part = new Chunk; + uint32 sz = min(BUFFER_SIZE - sizeof(uint32), left); + Chunk* part = new Chunk(new char[sz], sz); - part->len = min(BUFFER_SIZE - sizeof(uint32), left); - part->data = new char[part->len]; memcpy(part->data, p, part->len); left -= part->len; p += part->len; @@ -181,9 +175,7 @@ bool ChunkedIOFd::Write(Chunk* chunk) return false; } - delete [] chunk->data; delete chunk; - return true; } @@ -239,7 +231,6 @@ bool ChunkedIOFd::PutIntoWriteBuffer(Chunk* chunk) memcpy(write_buffer + write_len, chunk->data, len); write_len += len; - delete [] chunk->data; delete chunk; if ( network_time - last_flush > 0.005 ) @@ -362,9 +353,7 @@ ChunkedIO::Chunk* ChunkedIOFd::ExtractChunk() read_pos += sizeof(uint32); - Chunk* chunk = new Chunk; - chunk->len = len; - chunk->data = new char[real_len]; + Chunk* chunk = new Chunk(new char[real_len], len); memcpy(chunk->data, read_buffer + read_pos, real_len); read_pos += real_len; @@ -375,17 +364,13 @@ ChunkedIO::Chunk* ChunkedIOFd::ExtractChunk() ChunkedIO::Chunk* ChunkedIOFd::ConcatChunks(Chunk* c1, Chunk* c2) { - Chunk* c = new Chunk; - - c->len = c1->len + c2->len; - c->data = new char[c->len]; + uint32 sz = c1->len + c2->len; + Chunk* c = new Chunk(new char[sz], sz); memcpy(c->data, c1->data, c1->len); memcpy(c->data + c1->len, c2->data, c2->len); - delete [] c1->data; delete c1; - delete [] c2->data; delete c2; return c; @@ -627,7 +612,6 @@ void ChunkedIOFd::Clear() while ( pending_head ) { ChunkQueue* next = pending_head->next; - delete [] pending_head->chunk->data; delete pending_head->chunk; delete pending_head; pending_head = next; @@ -946,7 +930,6 @@ bool ChunkedIOSSL::Flush() --stats.pending; delete q; - delete [] c->data; delete c; write_state = LEN; @@ -1063,7 +1046,10 @@ bool ChunkedIOSSL::Read(Chunk** chunk, bool mayblock) } if ( ! read_chunk->data ) + { read_chunk->data = new char[read_chunk->len]; + read_chunk->free_func = Chunk::free_func_delete; + } if ( ! ReadData(read_chunk->data, read_chunk->len, &error) ) return ! error; @@ -1123,7 +1109,6 @@ void ChunkedIOSSL::Clear() while ( write_head ) { Queue* next = write_head->next; - delete [] write_head->chunk->data; delete write_head->chunk; delete write_head; write_head = next; @@ -1231,12 +1216,13 @@ bool CompressedChunkedIO::Read(Chunk** chunk, bool may_block) return false; } - delete [] (*chunk)->data; + (*chunk)->free_func((*chunk)->data); uncompressed_bytes_read += uncompressed_len; (*chunk)->len = uncompressed_len; (*chunk)->data = uncompressed; + (*chunk)->free_func = Chunk::free_func_delete; return true; } @@ -1280,8 +1266,9 @@ bool CompressedChunkedIO::Write(Chunk* chunk) memcpy(compressed, chunk->data, chunk->len); *(uint32*) (compressed + chunk->len) = 0; // uncompressed_length - delete [] chunk->data; + chunk->free_func(chunk->data); chunk->data = compressed; + chunk->free_func = Chunk::free_func_delete; chunk->len += 4; DBG_LOG(DBG_CHUNKEDIO, "zlib write pass-through: size=%d", chunk->len); @@ -1322,8 +1309,9 @@ bool CompressedChunkedIO::Write(Chunk* chunk) *(uint32*) zout.next_out = original_size; // uncompressed_length - delete [] chunk->data; + chunk->free_func(chunk->data); chunk->data = compressed; + chunk->free_func = Chunk::free_func_delete; chunk->len = ((char*) zout.next_out - compressed) + sizeof(uint32); diff --git a/src/ChunkedIO.h b/src/ChunkedIO.h index 6ee79cd3c4..a9865e4c05 100644 --- a/src/ChunkedIO.h +++ b/src/ChunkedIO.h @@ -11,7 +11,7 @@ #ifdef NEED_KRB5_H # include -#endif +#endif #include #include @@ -26,10 +26,28 @@ public: ChunkedIO(); virtual ~ChunkedIO() { } - typedef struct { + struct Chunk { + typedef void (*FreeFunc)(char*); + static void free_func_free(char* data) { free(data); } + static void free_func_delete(char* data) { delete [] data; } + + Chunk() + : data(), len(), free_func(free_func_delete) + { } + + // Takes ownership of data. + Chunk(char* arg_data, uint32 arg_len, + FreeFunc arg_ff = free_func_delete) + : data(arg_data), len(arg_len), free_func(arg_ff) + { } + + ~Chunk() + { free_func(data); } + char* data; uint32 len; - } Chunk; + FreeFunc free_func; + }; // Initialization before any I/O operation is performed. Returns false // on any form of error. diff --git a/src/RemoteSerializer.cc b/src/RemoteSerializer.cc index 8d07f34d38..99ec3d7c1e 100644 --- a/src/RemoteSerializer.cc +++ b/src/RemoteSerializer.cc @@ -372,10 +372,7 @@ static bool sendCMsg(ChunkedIO* io, char msg_type, RemoteSerializer::PeerID id) CMsg* msg = (CMsg*) new char[sizeof(CMsg)]; new (msg) CMsg(msg_type, id); - ChunkedIO::Chunk* c = new ChunkedIO::Chunk; - c->len = sizeof(CMsg); - c->data = (char*) msg; - + ChunkedIO::Chunk* c = new ChunkedIO::Chunk((char*)msg, sizeof(CMsg)); return io->Write(c); } @@ -386,10 +383,7 @@ static ChunkedIO::Chunk* makeSerialMsg(RemoteSerializer::PeerID id) CMsg* msg = (CMsg*) new char[sizeof(CMsg)]; new (msg) CMsg(MSG_SERIAL, id); - ChunkedIO::Chunk* c = new ChunkedIO::Chunk; - c->len = sizeof(CMsg); - c->data = (char*) msg; - + ChunkedIO::Chunk* c = new ChunkedIO::Chunk((char*)msg, sizeof(CMsg)); return c; } @@ -424,7 +418,7 @@ static bool sendToIO(ChunkedIO* io, ChunkedIO::Chunk* c) } static bool sendToIO(ChunkedIO* io, char msg_type, RemoteSerializer::PeerID id, - const char* str, int len = -1) + const char* str, int len = -1, bool delete_with_free = false) { if ( ! sendCMsg(io, msg_type, id) ) { @@ -432,9 +426,14 @@ static bool sendToIO(ChunkedIO* io, char msg_type, RemoteSerializer::PeerID id, return false; } - ChunkedIO::Chunk* c = new ChunkedIO::Chunk; - c->len = len >= 0 ? len : strlen(str) + 1; - c->data = const_cast(str); + uint32 sz = len >= 0 ? len : strlen(str) + 1; + ChunkedIO::Chunk* c = new ChunkedIO::Chunk(const_cast(str), sz); + + if ( delete_with_free ) + c->free_func = ChunkedIO::Chunk::free_func_free; + else + c->free_func = ChunkedIO::Chunk::free_func_delete; + return sendToIO(io, c); } @@ -455,10 +454,8 @@ static bool sendToIO(ChunkedIO* io, char msg_type, RemoteSerializer::PeerID id, for ( int i = 0; i < nargs; i++ ) args[i] = htonl(va_arg(ap, uint32)); - ChunkedIO::Chunk* c = new ChunkedIO::Chunk; - c->len = sizeof(uint32) * nargs; - c->data = (char*) args; - + ChunkedIO::Chunk* c = new ChunkedIO::Chunk((char*)args, + sizeof(uint32) * nargs); return sendToIO(io, c); } @@ -1529,7 +1526,6 @@ bool RemoteSerializer::Poll(bool may_block) current_msgtype = msg->Type(); current_args = 0; - delete [] c->data; delete c; switch ( current_msgtype ) { @@ -1592,7 +1588,6 @@ bool RemoteSerializer::Poll(bool may_block) msgstate = TYPE; bool result = DoMessage(); - delete [] current_args->data; delete current_args; current_args = 0; @@ -1787,9 +1782,7 @@ void RemoteSerializer::PeerConnected(Peer* peer) *args++ = htonl(peer->our_runtime); strcpy((char*) args, peer->our_class.c_str()); - ChunkedIO::Chunk* c = new ChunkedIO::Chunk; - c->len = len; - c->data = data; + ChunkedIO::Chunk* c = new ChunkedIO::Chunk(data, len); if ( peer->our_class.size() ) Log(LogInfo, fmt("sending class \"%s\"", peer->our_class.c_str()), peer); @@ -2568,8 +2561,8 @@ bool RemoteSerializer::SendLogCreateWriter(PeerID peer_id, EnumVal* id, EnumVal* goto error; c = new ChunkedIO::Chunk; - c->data = 0; c->len = fmt.EndWrite(&c->data); + c->free_func = ChunkedIO::Chunk::free_func_free; if ( ! SendToChild(c) ) goto error; @@ -2577,11 +2570,7 @@ bool RemoteSerializer::SendLogCreateWriter(PeerID peer_id, EnumVal* id, EnumVal* return true; error: - if ( c ) - { - delete [] c->data; - delete c; - } + delete c; FatalError(io->Error()); return false; @@ -2643,21 +2632,21 @@ bool RemoteSerializer::SendLogWrite(Peer* peer, EnumVal* id, EnumVal* writer, st { if ( ! FlushLogBuffer(peer) ) { - delete [] data; + free(data); return false; } } // If the data is actually larger than our complete buffer, just send it out. if ( len > LOG_BUFFER_SIZE ) - return SendToChild(MSG_LOG_WRITE, peer, data, len); + return SendToChild(MSG_LOG_WRITE, peer, data, len, true); // Now we have space in the buffer, copy it into there. memcpy(peer->log_buffer + peer->log_buffer_used, data, len); peer->log_buffer_used += len; assert(peer->log_buffer_used <= LOG_BUFFER_SIZE); - delete [] data; + free(data); return true; @@ -3112,14 +3101,19 @@ bool RemoteSerializer::SendCMsgToChild(char msg_type, Peer* peer) return true; } -bool RemoteSerializer::SendToChild(char type, Peer* peer, char* str, int len) +bool RemoteSerializer::SendToChild(char type, Peer* peer, char* str, int len, + bool delete_with_free) { DEBUG_COMM(fmt("parent: (->child) %s (#%" PRI_SOURCE_ID ", %s)", msgToStr(type), peer ? peer->id : PEER_NONE, str)); - if ( child_pid && sendToIO(io, type, peer ? peer->id : PEER_NONE, str, len) ) + if ( child_pid && sendToIO(io, type, peer ? peer->id : PEER_NONE, str, len, + delete_with_free) ) return true; - delete [] str; + if ( delete_with_free ) + free(str); + else + delete [] str; if ( ! child_pid ) return false; @@ -3169,7 +3163,7 @@ bool RemoteSerializer::SendToChild(ChunkedIO::Chunk* c) if ( child_pid && sendToIO(io, c) ) return true; - delete [] c->data; + c->free_func(c->data); c->data = 0; if ( ! child_pid ) @@ -3545,7 +3539,6 @@ bool SocketComm::ProcessParentMessage() parent_msgtype = msg->Type(); parent_args = 0; - delete [] c->data; delete c; switch ( parent_msgtype ) { @@ -3600,7 +3593,6 @@ bool SocketComm::ProcessParentMessage() if ( parent_args ) { - delete [] parent_args->data; delete parent_args; parent_args = 0; } @@ -3900,7 +3892,6 @@ bool SocketComm::ProcessRemoteMessage(SocketComm::Peer* peer) peer->state = msg->Type(); } - delete [] c->data; delete c; break; diff --git a/src/RemoteSerializer.h b/src/RemoteSerializer.h index 5ff7fff8d6..9dbfbd9dae 100644 --- a/src/RemoteSerializer.h +++ b/src/RemoteSerializer.h @@ -317,7 +317,8 @@ protected: // Communication helpers bool SendCMsgToChild(char msg_type, Peer* peer); - bool SendToChild(char type, Peer* peer, char* str, int len = -1); + bool SendToChild(char type, Peer* peer, char* str, int len = -1, + bool delete_with_free = false); bool SendToChild(char type, Peer* peer, int nargs, ...); // can send uints32 only bool SendToChild(ChunkedIO::Chunk* c); diff --git a/src/SerializationFormat.cc b/src/SerializationFormat.cc index eb8462521e..6a133d64e4 100644 --- a/src/SerializationFormat.cc +++ b/src/SerializationFormat.cc @@ -5,6 +5,8 @@ #include "Serializer.h" #include "Reporter.h" +const float SerializationFormat::GROWTH_FACTOR = 2.5; + SerializationFormat::SerializationFormat() : output(), output_size(), output_pos(), input(), input_len(), input_pos(), bytes_written(), bytes_read() @@ -13,7 +15,7 @@ SerializationFormat::SerializationFormat() SerializationFormat::~SerializationFormat() { - delete [] output; + free(output); } void SerializationFormat::StartRead(char* data, uint32 arg_len) @@ -33,13 +35,13 @@ void SerializationFormat::StartWrite() { if ( output && output_size > INITIAL_SIZE ) { - delete [] output; + free(output); output = 0; } if ( ! output ) { - output = new char[INITIAL_SIZE]; + output = (char*)safe_malloc(INITIAL_SIZE); output_size = INITIAL_SIZE; } @@ -49,9 +51,12 @@ void SerializationFormat::StartWrite() uint32 SerializationFormat::EndWrite(char** data) { - *data = new char[output_pos]; - memcpy(*data, output, output_pos); - return output_pos; + uint32 rval = output_pos; + *data = output; + output = 0; + output_size = 0; + output_pos = 0; + return rval; } bool SerializationFormat::ReadData(void* b, size_t count) @@ -75,11 +80,8 @@ bool SerializationFormat::WriteData(const void* b, size_t count) // Increase buffer if necessary. while ( output_pos + count > output_size ) { - output_size = output_pos + count + INITIAL_SIZE; - char* tmp = new char[output_size]; - memcpy(tmp, output, output_pos); - delete [] output; - output = tmp; + output_size *= GROWTH_FACTOR; + output = (char*)safe_realloc(output, output_size); } memcpy(output + output_pos, b, count); diff --git a/src/SerializationFormat.h b/src/SerializationFormat.h index f270b61bae..e652341698 100644 --- a/src/SerializationFormat.h +++ b/src/SerializationFormat.h @@ -44,7 +44,15 @@ public: // Serialization. virtual void StartWrite(); - virtual uint32 EndWrite(char** data); // passes ownership + + /** + * Retrieves serialized data. + * @param data A pointer that will be assigned to point at the internal + * buffer containing serialized data. The memory should + * be reclaimed using "free()". + * @return The number of bytes in the buffer object assigned to \a data. + */ + virtual uint32 EndWrite(char** data); virtual bool Write(int v, const char* tag) = 0; virtual bool Write(uint16 v, const char* tag) = 0; @@ -74,6 +82,7 @@ protected: bool WriteData(const void* buf, size_t count); static const uint32 INITIAL_SIZE = 65536; + static const float GROWTH_FACTOR; char* output; uint32 output_size; uint32 output_pos; diff --git a/src/Serializer.cc b/src/Serializer.cc index 156ad67f2e..36b1c74000 100644 --- a/src/Serializer.cc +++ b/src/Serializer.cc @@ -81,6 +81,7 @@ bool Serializer::EndSerialization(SerialInfo* info) ChunkedIO::Chunk* chunk = new ChunkedIO::Chunk; chunk->len = format->EndWrite(&chunk->data); + chunk->free_func = ChunkedIO::Chunk::free_func_free; if ( ! io->Write(chunk) ) { @@ -282,7 +283,6 @@ int Serializer::Unserialize(UnserialInfo* info, bool block) if ( ! info->chunk ) { // only delete if we allocated it ourselves - delete [] chunk->data; delete chunk; } diff --git a/src/Type.cc b/src/Type.cc index 02a486056d..672153d957 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -131,7 +131,7 @@ BroType* BroType::Clone() const sinfo.cache = false; this->Serialize(&sinfo); - char* data = 0; + char* data; uint32 len = form->EndWrite(&data); form->StartRead(data, len); @@ -141,7 +141,7 @@ BroType* BroType::Clone() const BroType* rval = this->Unserialize(&uinfo, false); assert(rval != this); - delete [] data; + free(data); return rval; } diff --git a/src/Val.cc b/src/Val.cc index 4aec6a7c31..9b5b0b9d58 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -92,8 +92,7 @@ Val* Val::Clone() const uinfo.cache = false; Val* clone = Unserialize(&uinfo, type); - delete [] data; - + free(data); return clone; }