From c920b45fb8b400000dd4e3ac3ca4747817408dd4 Mon Sep 17 00:00:00 2001
From: Alexander Alekhin <alexander.a.alekhin@gmail.com>
Date: Tue, 24 Mar 2020 08:58:30 +0000
Subject: [PATCH] core(persistence): fix resource leaks - force closing files

backporting commit 673eb2b00628c0a702fb1653cfa06860cf85be19
---
 modules/core/src/persistence.cpp   |  4 +-
 modules/core/src/persistence_c.cpp | 58 ++++++++++++++++------------
 modules/core/test/test_io.cpp      | 61 ++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 27 deletions(-)

diff --git a/modules/core/src/persistence.cpp b/modules/core/src/persistence.cpp
index 4231336a8d..40d1cdfa07 100644
--- a/modules/core/src/persistence.cpp
+++ b/modules/core/src/persistence.cpp
@@ -251,10 +251,10 @@ void icvClose( CvFileStorage* fs, cv::String* out )
             else if ( fs->fmt == CV_STORAGE_FORMAT_JSON )
                 icvPuts( fs, "}\n" );
         }
-
-        icvCloseFile(fs);
     }
 
+    icvCloseFile(fs);
+
     if( fs->outbuf && out )
     {
         *out = cv::String(fs->outbuf->begin(), fs->outbuf->end());
diff --git a/modules/core/src/persistence_c.cpp b/modules/core/src/persistence_c.cpp
index d36cae3297..9ec70190df 100644
--- a/modules/core/src/persistence_c.cpp
+++ b/modules/core/src/persistence_c.cpp
@@ -68,10 +68,9 @@ static bool is_param_exist( const std::vector<std::string> & params, const std::
 
 //===========================================================================================
 
-CV_IMPL CvFileStorage*
-cvOpenFileStorage( const char* query, CvMemStorage* dststorage, int flags, const char* encoding )
+static
+void cvOpenFileStorage_(CvFileStorage*& fs, const char* query, CvMemStorage* dststorage, int flags, const char* encoding)
 {
-    CvFileStorage* fs = 0;
     int default_block_size = 1 << 18;
     bool append = (flags & 3) == CV_STORAGE_APPEND;
     bool mem = (flags & CV_STORAGE_MEMORY) != 0;
@@ -104,10 +103,6 @@ cvOpenFileStorage( const char* query, CvMemStorage* dststorage, int flags, const
     if( mem && append )
         CV_Error( CV_StsBadFlag, "CV_STORAGE_APPEND and CV_STORAGE_MEMORY are not currently compatible" );
 
-    fs = (CvFileStorage*)cvAlloc( sizeof(*fs) );
-    CV_Assert(fs);
-    memset( fs, 0, sizeof(*fs));
-
     fs->memstorage = cvCreateMemStorage( default_block_size );
     fs->dststorage = dststorage ? dststorage : fs->memstorage;
 
@@ -373,9 +368,12 @@ cvOpenFileStorage( const char* query, CvMemStorage* dststorage, int flags, const
         const char* yaml_signature = "%YAML";
         const char* json_signature = "{";
         const char* xml_signature  = "<?xml";
-        char buf[16];
-        icvGets( fs, buf, sizeof(buf)-2 );
-        char* bufPtr = cv_skip_BOM(buf);
+        char buf[16] = { 0 };
+        char* bufPtr = icvGets( fs, buf, sizeof(buf)-2);
+        if (!bufPtr)
+            CV_Error(CV_BADARG_ERR, "Can't read from input stream or input stream is empty");
+        bufPtr = cv_skip_BOM(bufPtr);
+        CV_Assert(bufPtr);
         size_t bufOffset = bufPtr - buf;
 
         if(strncmp( bufPtr, yaml_signature, strlen(yaml_signature) ) == 0)
@@ -417,21 +415,12 @@ cvOpenFileStorage( const char* query, CvMemStorage* dststorage, int flags, const
 
         //mode = cvGetErrMode();
         //cvSetErrMode( CV_ErrModeSilent );
-        try
+        switch (fs->fmt)
         {
-            switch (fs->fmt)
-            {
-            case CV_STORAGE_FORMAT_XML : { icvXMLParse ( fs ); break; }
-            case CV_STORAGE_FORMAT_YAML: { icvYMLParse ( fs ); break; }
-            case CV_STORAGE_FORMAT_JSON: { icvJSONParse( fs ); break; }
-            default: break;
-            }
-        }
-        catch (...)
-        {
-            fs->is_opened = true;
-            cvReleaseFileStorage( &fs );
-            CV_RETHROW();
+        case CV_STORAGE_FORMAT_XML : { icvXMLParse ( fs ); break; }
+        case CV_STORAGE_FORMAT_YAML: { icvYMLParse ( fs ); break; }
+        case CV_STORAGE_FORMAT_JSON: { icvJSONParse( fs ); break; }
+        default: break;
         }
         //cvSetErrMode( mode );
 
@@ -457,9 +446,28 @@ _exit_:
         }
     }
 
-    return  fs;
+    return;
 }
 
+CV_IMPL CvFileStorage*
+cvOpenFileStorage(const char* query, CvMemStorage* dststorage, int flags, const char* encoding)
+{
+    CvFileStorage* fs = (CvFileStorage*)cvAlloc( sizeof(*fs) );
+    CV_Assert(fs);
+    memset( fs, 0, sizeof(*fs));
+
+    try
+    {
+        cvOpenFileStorage_(fs, query, dststorage, flags, encoding);
+        return fs;
+    }
+    catch (...)
+    {
+        if (fs)
+            cvReleaseFileStorage(&fs);
+        throw;
+    }
+}
 
 /* closes file storage and deallocates buffers */
 CV_IMPL  void
diff --git a/modules/core/test/test_io.cpp b/modules/core/test/test_io.cpp
index 9744d5b425..80a2503609 100644
--- a/modules/core/test/test_io.cpp
+++ b/modules/core/test/test_io.cpp
@@ -1703,4 +1703,65 @@ TEST(Core_InputOutput, FileStorage_YAML_parse_multiple_documents)
     ASSERT_EQ(0, std::remove(filename.c_str()));
 }
 
+TEST(Core_InputOutput, FileStorage_empty_16823)
+{
+    std::string fname = tempfile("test_fs_empty.yml");
+    {
+        // create empty file
+        std::ofstream f(fname.c_str(), std::ios::out);
+    }
+
+    try
+    {
+        FileStorage fs(fname, FileStorage::READ);
+        ADD_FAILURE() << "Exception must be thrown for empty file.";
+    }
+    catch (const cv::Exception&)
+    {
+        // expected way
+        // closed files can be checked manually through 'strace'
+    }
+    catch (const std::exception& e)
+    {
+        ADD_FAILURE() << "Unexpected exception: " << e.what();
+    }
+    catch (...)
+    {
+        ADD_FAILURE() << "Unexpected unknown C++ exception";
+    }
+
+    EXPECT_EQ(0, remove(fname.c_str()));
+}
+
+TEST(Core_InputOutput, FileStorage_open_empty_16823)
+{
+    std::string fname = tempfile("test_fs_open_empty.yml");
+    {
+        // create empty file
+        std::ofstream f(fname.c_str(), std::ios::out);
+    }
+
+    FileStorage fs;
+    try
+    {
+        fs.open(fname, FileStorage::READ);
+        ADD_FAILURE() << "Exception must be thrown for empty file.";
+    }
+    catch (const cv::Exception&)
+    {
+        // expected way
+        // closed files can be checked manually through 'strace'
+    }
+    catch (const std::exception& e)
+    {
+        ADD_FAILURE() << "Unexpected exception: " << e.what();
+    }
+    catch (...)
+    {
+        ADD_FAILURE() << "Unexpected unknown C++ exception";
+    }
+
+    EXPECT_EQ(0, remove(fname.c_str()));
+}
+
 }} // namespace