From e7cf52e33c7d973c280df2225266207225876558 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Tue, 7 Jun 2016 10:54:18 -0700 Subject: [PATCH 1/2] Raw Writer: First step - make code more c++11-y, remove raw pointers. --- src/input/readers/raw/Raw.cc | 72 ++++++++++++++---------------------- src/input/readers/raw/Raw.h | 24 +++++++----- 2 files changed, 42 insertions(+), 54 deletions(-) diff --git a/src/input/readers/raw/Raw.cc b/src/input/readers/raw/Raw.cc index 76d8958fea..843d1d25d1 100644 --- a/src/input/readers/raw/Raw.cc +++ b/src/input/readers/raw/Raw.cc @@ -26,10 +26,8 @@ using threading::Field; const int Raw::block_size = 4096; // how big do we expect our chunks of data to be. -Raw::Raw(ReaderFrontend *frontend) : ReaderBackend(frontend) +Raw::Raw(ReaderFrontend *frontend) : ReaderBackend(frontend), file(nullptr, fclose), stderrfile(nullptr, fclose) { - file = 0; - stderrfile = 0; execute = false; firstrun = true; mtime = 0; @@ -40,8 +38,6 @@ Raw::Raw(ReaderFrontend *frontend) : ReaderBackend(frontend) sep_length = BifConst::InputRaw::record_separator->Len(); - buf = 0; - outbuf = 0; bufpos = 0; stdin_fileno = fileno(stdin); @@ -61,13 +57,9 @@ Raw::~Raw() void Raw::DoClose() { - if ( file != 0 ) + if ( file ) CloseInput(); - // Just throw away output that has not been flushed. - delete [] buf; - buf = 0; - if ( execute && childpid > 0 && kill(childpid, 0) == 0 ) { // Kill child process group. @@ -255,7 +247,7 @@ bool Raw::Execute() else ClosePipeEnd(stderr_in); - file = fdopen(pipes[stdout_in], "r"); + file = std::unique_ptr(fdopen(pipes[stdout_in], "r"), fclose); if ( ! file ) { @@ -267,7 +259,7 @@ bool Raw::Execute() if ( use_stderr ) { - stderrfile = fdopen(pipes[stderr_in], "r"); + stderrfile = std::unique_ptr(fdopen(pipes[stderr_in], "r"), fclose); if ( ! stderrfile ) { @@ -289,14 +281,14 @@ bool Raw::OpenInput() else { - file = fopen(fname.c_str(), "r"); + file = std::unique_ptr(fopen(fname.c_str(), "r"), fclose); if ( ! file ) { Error(Fmt("Init: cannot open %s", fname.c_str())); return false; } - if ( ! SetFDFlags(fileno(file), F_SETFD, FD_CLOEXEC) ) + if ( ! SetFDFlags(fileno(file.get()), F_SETFD, FD_CLOEXEC) ) Warning(Fmt("Init: cannot set close-on-exec for %s", fname.c_str())); } @@ -305,7 +297,7 @@ bool Raw::OpenInput() int whence = (offset >= 0) ? SEEK_SET : SEEK_END; int64_t pos = (offset >= 0) ? offset : offset + 1; // we want -1 to be the end of the file - if ( fseek(file, pos, whence) < 0 ) + if ( fseek(file.get(), pos, whence) < 0 ) { char buf[256]; strerror_r(errno, buf, sizeof(buf)); @@ -318,7 +310,7 @@ bool Raw::OpenInput() bool Raw::CloseInput() { - if ( file == 0 ) + if ( ! file ) { InternalWarning(Fmt("Trying to close closed file for stream %s", fname.c_str())); @@ -328,10 +320,10 @@ bool Raw::CloseInput() Debug(DBG_INPUT, "Raw reader starting close"); #endif - fclose(file); + file.reset(nullptr); if ( use_stderr ) - fclose(stderrfile); + stderrfile.reset(nullptr); if ( execute ) { @@ -339,9 +331,6 @@ bool Raw::CloseInput() ClosePipeEnd(i); } - file = 0; - stderrfile = 0; - #ifdef DEBUG Debug(DBG_INPUT, "Raw reader finished close"); #endif @@ -455,14 +444,14 @@ int64_t Raw::GetLine(FILE* arg_file) int pos = 0; // strstr_n only works on ints - so no use to use something different here int offset = 0; - if ( buf == 0 ) - buf = new char[block_size]; + if ( ! buf ) + buf = std::unique_ptr(new char[block_size]); int repeats = 1; for ( ;; ) { - size_t readbytes = fread(buf+bufpos+offset, 1, block_size-bufpos, arg_file); + size_t readbytes = fread(buf.get()+bufpos+offset, 1, block_size-bufpos, arg_file); pos += bufpos + readbytes; //printf("Pos: %d\n", pos); bufpos = offset = 0; // read full block size in next read... @@ -473,7 +462,7 @@ int64_t Raw::GetLine(FILE* arg_file) // researching everything each time is a bit... cpu-intensive. But otherwhise we have // to deal with situations where the separator is multi-character and split over multiple // reads... - int found = strstr_n(pos, (unsigned char*) buf, separator.size(), (unsigned char*) separator.c_str()); + int found = strstr_n(pos, (unsigned char*) buf.get(), separator.size(), (unsigned char*) separator.c_str()); if ( found == -1 ) { @@ -485,30 +474,27 @@ int64_t Raw::GetLine(FILE* arg_file) return -1; // signal EOF - and that we had no more data. else { - outbuf = buf; - buf = 0; + outbuf = std::move(buf); // buf is null after this return pos; } } repeats++; // bah, we cannot use realloc because we would have to change the delete in the manager to a free. - char * newbuf = new char[block_size*repeats]; - memcpy(newbuf, buf, block_size*(repeats-1)); - delete [] buf; - buf = newbuf; + std::unique_ptr newbuf = std::unique_ptr(new char[block_size*repeats]); + memcpy(newbuf.get(), buf.get(), block_size*(repeats-1)); + buf = std::move(newbuf); offset = block_size*(repeats-1); } else { - outbuf = buf; - buf = 0; + outbuf = std::move(buf); if ( found < pos ) { // we have leftovers. copy them into the buffer for the next line - buf = new char[block_size]; - memcpy(buf, outbuf + found + sep_length, pos - found - sep_length); + buf = std::unique_ptr(new char[block_size]); + memcpy(buf.get(), outbuf.get() + found + sep_length, pos - found - sep_length); bufpos = pos - found - sep_length; } @@ -586,9 +572,9 @@ bool Raw::DoUpdate() case MODE_MANUAL: case MODE_STREAM: - if ( Info().mode == MODE_STREAM && file != 0 ) + if ( Info().mode == MODE_STREAM && file ) { - clearerr(file); // remove end of file evil bits + clearerr(file.get()); // remove end of file evil bits break; } @@ -610,7 +596,7 @@ bool Raw::DoUpdate() if ( stdin_towrite > 0 ) WriteToStdin(); - int64_t length = GetLine(file); + int64_t length = GetLine(file.get()); //printf("Read %lld bytes\n", length); if ( length == -3 ) @@ -624,7 +610,7 @@ bool Raw::DoUpdate() // filter has exactly one text field. convert to it. Value* val = new Value(TYPE_STRING, true); - val->val.string_val.data = outbuf; + val->val.string_val.data = outbuf.release(); val->val.string_val.length = length; fields[0] = val; @@ -636,15 +622,13 @@ bool Raw::DoUpdate() } Put(fields); - - outbuf = 0; } if ( use_stderr ) { for ( ;; ) { - int64_t length = GetLine(stderrfile); + int64_t length = GetLine(stderrfile.get()); //printf("Read stderr %lld bytes\n", length); if ( length == -3 ) return false; @@ -654,7 +638,7 @@ bool Raw::DoUpdate() Value** fields = new Value*[2]; Value* val = new Value(TYPE_STRING, true); - val->val.string_val.data = outbuf; + val->val.string_val.data = outbuf.release(); val->val.string_val.length = length; fields[0] = val; Value* bval = new Value(TYPE_BOOL, true); @@ -662,8 +646,6 @@ bool Raw::DoUpdate() fields[1] = bval; Put(fields); - - outbuf = 0; } } diff --git a/src/input/readers/raw/Raw.h b/src/input/readers/raw/Raw.h index f08d22beca..5111c714c6 100644 --- a/src/input/readers/raw/Raw.h +++ b/src/input/readers/raw/Raw.h @@ -16,16 +16,22 @@ namespace input { namespace reader { */ class Raw : public ReaderBackend { public: - Raw(ReaderFrontend* frontend); + explicit Raw(ReaderFrontend* frontend); ~Raw(); + // prohibit copying and moving + Raw(const Raw&) = delete; + Raw(Raw&&) = delete; + Raw& operator=(const Raw&) = delete; + Raw& operator=(Raw&&) = delete; + static ReaderBackend* Instantiate(ReaderFrontend* frontend) { return new Raw(frontend); } protected: - virtual bool DoInit(const ReaderInfo& info, int arg_num_fields, const threading::Field* const* fields); - virtual void DoClose(); - virtual bool DoUpdate(); - virtual bool DoHeartbeat(double network_time, double current_time); + bool DoInit(const ReaderInfo& info, int arg_num_fields, const threading::Field* const* fields) override; + void DoClose() override; + bool DoUpdate() override; + bool DoHeartbeat(double network_time, double current_time) override; private: void ClosePipeEnd(int i); @@ -40,8 +46,8 @@ private: void WriteToStdin(); string fname; // Source with a potential "|" removed. - FILE* file; - FILE* stderrfile; + std::unique_ptr file; + std::unique_ptr stderrfile; bool execute; bool firstrun; time_t mtime; @@ -51,8 +57,8 @@ private: unsigned int sep_length; // length of the separator int bufpos; - char* buf; - char* outbuf; + std::unique_ptr buf; + std::unique_ptr outbuf; int stdin_fileno; int stdout_fileno; From e8591303694c2432be91ff175dbb2d5f49f2cb47 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Wed, 15 Jun 2016 17:08:35 -0700 Subject: [PATCH 2/2] Exec: fix reader cleanup when using read_files Wen using read_files, the Exec framework called Input::remove on the wrong input stream: it always got called on the input stream of the execution, not on the input stream of the current file that was being read. This lead to threads never being closed and file handles being kept open until Bro is closed. This means that before this patch, every time ActiveHTTP is used, a thread stays around and several file handles are used. --- scripts/base/utils/exec.bro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/base/utils/exec.bro b/scripts/base/utils/exec.bro index 15d88e9851..a926775bda 100644 --- a/scripts/base/utils/exec.bro +++ b/scripts/base/utils/exec.bro @@ -116,7 +116,7 @@ event Input::end_of_data(orig_name: string, source:string) if ( track_file !in result$files ) result$files[track_file] = vector(); - Input::remove(name); + Input::remove(orig_name); if ( name !in pending_files ) delete pending_commands[name];