From c9fb27da0a72135417956dca8dafa959ebb67c10 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 25 Sep 2014 08:53:43 +0200 Subject: [PATCH 1/3] CBufferedFile: convert into a non-refcounted RAII wrapper - it now takes over the passed file descriptor and closes it in the destructor - this fixes a leak in LoadExternalBlockFile(), where an exception could cause the file to not getting closed - disallow copies (like recently added for CAutoFile) - make nType and nVersion private --- src/main.cpp | 2 +- src/serialize.h | 30 +++++++++++++++++++++--------- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a3e36ff87..033373888 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3084,6 +3084,7 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp) int nLoaded = 0; try { + // This takes over fileIn and calls fclose() on it in the CBufferedFile destructor CBufferedFile blkdat(fileIn, 2*MAX_BLOCK_SIZE, MAX_BLOCK_SIZE+8, SER_DISK, CLIENT_VERSION); uint64_t nStartByte = 0; if (dbp) { @@ -3140,7 +3141,6 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp) LogPrintf("%s : Deserialize or I/O error - %s", __func__, e.what()); } } - fclose(fileIn); } catch(std::runtime_error &e) { AbortNode(_("Error: system error: ") + e.what()); } diff --git a/src/serialize.h b/src/serialize.h index 68501facf..f56dce148 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -1256,13 +1256,19 @@ public: } }; -/** Wrapper around a FILE* that implements a ring buffer to - * deserialize from. It guarantees the ability to rewind - * a given number of bytes. */ +/** Non-refcounted RAII wrapper around a FILE* that implements a ring buffer to + * deserialize from. It guarantees the ability to rewind a given number of bytes. */ class CBufferedFile { private: - FILE *src; // source file + // Disallow copies + CBufferedFile(const CBufferedFile&); + CBufferedFile& operator=(const CBufferedFile&); + + int nType; + int nVersion; + + FILE *src; // source file uint64_t nSrcPos; // how many bytes have been read from source uint64_t nReadPos; // how many bytes have been read from this uint64_t nReadLimit; // up to which position we're allowed to read @@ -1289,12 +1295,18 @@ protected: } public: - int nType; - int nVersion; - CBufferedFile(FILE *fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nTypeIn, int nVersionIn) : - src(fileIn), nSrcPos(0), nReadPos(0), nReadLimit((uint64_t)(-1)), nRewind(nRewindIn), vchBuf(nBufSize, 0), - nType(nTypeIn), nVersion(nVersionIn) { + nSrcPos(0), nReadPos(0), nReadLimit((uint64_t)(-1)), nRewind(nRewindIn), vchBuf(nBufSize, 0) + { + src = fileIn; + nType = nTypeIn; + nVersion = nVersionIn; + } + + ~CBufferedFile() + { + if (src) + fclose(src); } // check whether we're at the end of the source file From 0c35486dc97909cea67b24e8758bd0f40ac33a9a Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Mon, 29 Sep 2014 16:10:29 +0200 Subject: [PATCH 2/3] CBufferedFile: add explicit close function - also use identical close function for CAutoFile (avoids setting file to NULL under wrong conditions) --- src/serialize.h | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/serialize.h b/src/serialize.h index f56dce148..63c72cb8e 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -1154,7 +1154,7 @@ public: -/** Non-refcounted RAII wrapper for FILE*. +/** Non-refcounted RAII wrapper for FILE* * * Will automatically close the file when it goes out of scope if not null. * If you're returning the file pointer, return file.release(). @@ -1186,9 +1186,10 @@ public: void fclose() { - if (file != NULL && file != stdin && file != stdout && file != stderr) + if (file) { ::fclose(file); - file = NULL; + file = NULL; + } } FILE* release() { FILE* ret = file; file = NULL; return ret; } @@ -1257,7 +1258,11 @@ public: }; /** Non-refcounted RAII wrapper around a FILE* that implements a ring buffer to - * deserialize from. It guarantees the ability to rewind a given number of bytes. */ + * deserialize from. It guarantees the ability to rewind a given number of bytes. + * + * Will automatically close the file when it goes out of scope if not null. + * If you need to close the file early, use file.fclose() instead of fclose(file). + */ class CBufferedFile { private: @@ -1305,8 +1310,15 @@ public: ~CBufferedFile() { - if (src) - fclose(src); + fclose(); + } + + void fclose() + { + if (src) { + ::fclose(src); + src = NULL; + } } // check whether we're at the end of the source file From 938bccebf1cb3ed6c7b8bfb8236a5172433bf890 Mon Sep 17 00:00:00 2001 From: Philip Kaufmann Date: Thu, 2 Oct 2014 10:59:28 +0200 Subject: [PATCH 3/3] CAutoFile: make file private --- src/serialize.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/serialize.h b/src/serialize.h index 63c72cb8e..ff11edc06 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -1166,12 +1166,13 @@ private: // Disallow copies CAutoFile(const CAutoFile&); CAutoFile& operator=(const CAutoFile&); -protected: - FILE* file; -public: + int nType; int nVersion; + + FILE* file; +public: CAutoFile(FILE* filenew, int nTypeIn, int nVersionIn) { file = filenew;