From ffe0d504471435f83eca9bd5ddfa6769ccc26d67 Mon Sep 17 00:00:00 2001 From: Alexander Alekhin Date: Fri, 17 Jul 2020 21:39:06 +0000 Subject: [PATCH] core(persistence): fix "use after free" bug - do not store user-controlled "FileStorage" pointer - store FileStorage::Impl pointer instead --- .../core/include/opencv2/core/persistence.hpp | 8 +++- modules/core/src/persistence.cpp | 40 +++++++++++-------- modules/core/test/test_io.cpp | 23 +++++++++++ 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/modules/core/include/opencv2/core/persistence.hpp b/modules/core/include/opencv2/core/persistence.hpp index 152fd560d8..9e3f8f6914 100644 --- a/modules/core/include/opencv2/core/persistence.hpp +++ b/modules/core/include/opencv2/core/persistence.hpp @@ -510,6 +510,8 @@ public: @param fs Pointer to the file storage structure. @param blockIdx Index of the memory block where the file node is stored @param ofs Offset in bytes from the beginning of the serialized storage + + @deprecated */ FileNode(const FileStorage* fs, size_t blockIdx, size_t ofs); @@ -614,7 +616,9 @@ public: CV_WRAP Mat mat() const; //protected: - const FileStorage* fs; + FileNode(FileStorage::Impl* fs, size_t blockIdx, size_t ofs); + + FileStorage::Impl* fs; size_t blockIdx; size_t ofs; }; @@ -679,7 +683,7 @@ public: bool equalTo(const FileNodeIterator& it) const; protected: - const FileStorage* fs; + FileStorage::Impl* fs; size_t blockIdx; size_t ofs; size_t blockSize; diff --git a/modules/core/src/persistence.cpp b/modules/core/src/persistence.cpp index 1e07119f4e..4bf52a3134 100644 --- a/modules/core/src/persistence.cpp +++ b/modules/core/src/persistence.cpp @@ -1376,9 +1376,9 @@ public: return new_ptr; } - unsigned getStringOfs( const std::string& key ) + unsigned getStringOfs( const std::string& key ) const { - str_hash_t::iterator it = str_hash.find(key); + str_hash_t::const_iterator it = str_hash.find(key); return it != str_hash.end() ? it->second : 0; } @@ -1468,7 +1468,7 @@ public: writeInt(ptr, (int)rawSize); } - void normalizeNodeOfs(size_t& blockIdx, size_t& ofs) + void normalizeNodeOfs(size_t& blockIdx, size_t& ofs) const { while( ofs >= fs_data_blksz[blockIdx] ) { @@ -2048,18 +2048,24 @@ FileStorage& operator << (FileStorage& fs, const String& str) FileNode::FileNode() + : fs(NULL) { - fs = 0; blockIdx = ofs = 0; } -FileNode::FileNode(const FileStorage* _fs, size_t _blockIdx, size_t _ofs) +FileNode::FileNode(FileStorage::Impl* _fs, size_t _blockIdx, size_t _ofs) + : fs(_fs) { - fs = _fs; blockIdx = _blockIdx; ofs = _ofs; } +FileNode::FileNode(const FileStorage* _fs, size_t _blockIdx, size_t _ofs) + : FileNode(_fs->p.get(), _blockIdx, _ofs) +{ + // nothing +} + FileNode::FileNode(const FileNode& node) { fs = node.fs; @@ -2082,7 +2088,7 @@ FileNode FileNode::operator[](const std::string& nodename) const CV_Assert( isMap() ); - unsigned key = fs->p->getStringOfs(nodename); + unsigned key = fs->getStringOfs(nodename); size_t i, sz = size(); FileNodeIterator it = begin(); @@ -2091,7 +2097,7 @@ FileNode FileNode::operator[](const std::string& nodename) const FileNode n = *it; const uchar* p = n.ptr(); unsigned key2 = (unsigned)readInt(p + 1); - CV_Assert( key2 < fs->p->str_hash_data.size() ); + CV_Assert( key2 < fs->str_hash_data.size() ); if( key == key2 ) return n; } @@ -2167,7 +2173,7 @@ std::string FileNode::name() const if(!p) return std::string(); size_t nameofs = p[1] | (p[2]<<8) | (p[3]<<16) | (p[4]<<24); - return fs->p->getName(nameofs); + return fs->getName(nameofs); } FileNode::operator int() const @@ -2292,12 +2298,12 @@ size_t FileNode::rawSize() const uchar* FileNode::ptr() { - return !fs ? 0 : (uchar*)fs->p->getNodePtr(blockIdx, ofs); + return !fs ? 0 : (uchar*)fs->getNodePtr(blockIdx, ofs); } const uchar* FileNode::ptr() const { - return !fs ? 0 : fs->p->getNodePtr(blockIdx, ofs); + return !fs ? 0 : fs->getNodePtr(blockIdx, ofs); } void FileNode::setValue( int type, const void* value, int len ) @@ -2328,7 +2334,7 @@ void FileNode::setValue( int type, const void* value, int len ) else CV_Error(Error::StsNotImplemented, "Only scalar types can be dynamically assigned to a file node"); - p = fs->p->reserveNodeSpace(*this, sz); + p = fs->reserveNodeSpace(*this, sz); *p++ = (uchar)(type | (tag & NAMED)); if( tag & NAMED ) p += 4; @@ -2402,8 +2408,8 @@ FileNodeIterator::FileNodeIterator( const FileNode& node, bool seekEnd ) idx = nodeNElems; } } - fs->p->normalizeNodeOfs(blockIdx, ofs); - blockSize = fs->p->fs_data_blksz[blockIdx]; + fs->normalizeNodeOfs(blockIdx, ofs); + blockSize = fs->fs_data_blksz[blockIdx]; } } @@ -2430,7 +2436,7 @@ FileNodeIterator& FileNodeIterator::operator=(const FileNodeIterator& it) FileNode FileNodeIterator::operator *() const { - return FileNode(idx < nodeNElems ? fs : 0, blockIdx, ofs); + return FileNode(idx < nodeNElems ? fs : NULL, blockIdx, ofs); } FileNodeIterator& FileNodeIterator::operator ++ () @@ -2442,8 +2448,8 @@ FileNodeIterator& FileNodeIterator::operator ++ () ofs += n.rawSize(); if( ofs >= blockSize ) { - fs->p->normalizeNodeOfs(blockIdx, ofs); - blockSize = fs->p->fs_data_blksz[blockIdx]; + fs->normalizeNodeOfs(blockIdx, ofs); + blockSize = fs->fs_data_blksz[blockIdx]; } return *this; } diff --git a/modules/core/test/test_io.cpp b/modules/core/test/test_io.cpp index 692e7f36c6..e695300d4b 100644 --- a/modules/core/test/test_io.cpp +++ b/modules/core/test/test_io.cpp @@ -1788,4 +1788,27 @@ TEST(Core_InputOutput, FileStorage_copy_constructor_17412) EXPECT_EQ(0, remove(fname.c_str())); } +TEST(Core_InputOutput, FileStorage_copy_constructor_17412_heap) +{ + std::string fname = tempfile("test.yml"); + FileStorage fs_orig(fname, cv::FileStorage::WRITE); + fs_orig << "string" << "wat"; + fs_orig.release(); + + // no crash anymore + cv::FileStorage fs; + + // use heap to allow valgrind detections + { + cv::FileStorage* fs2 = new cv::FileStorage(fname, cv::FileStorage::READ); + fs = *fs2; + delete fs2; + } + + std::string s; + fs["string"] >> s; + EXPECT_EQ(s, "wat"); + EXPECT_EQ(0, remove(fname.c_str())); +} + }} // namespace