Skip to content

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
    • Help
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
otb
otb
  • Project
    • Project
    • Details
    • Activity
    • Releases
    • Cycle Analytics
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Charts
  • Issues 191
    • Issues 191
    • List
    • Board
    • Labels
    • Milestones
  • Merge Requests 12
    • Merge Requests 12
  • CI / CD
    • CI / CD
    • Pipelines
    • Jobs
    • Schedules
    • Charts
  • Registry
    • Registry
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Members
    • Members
  • Collapse sidebar
  • Activity
  • Graph
  • Charts
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
  • Main Repositories
  • otbotb
  • Issues
  • #1819

Closed
Open
Opened Jan 23, 2019 by Stéphane Albert@salbert
  • Report abuse
  • New issue
Report abuse New issue

otb::TestHelper::RegressionTestOgrFile() is exception unsafe.

Description

otb::TestHelper::RegressionTestOgrFile() [1]:

  • is not exception-safe (memory-leaks) (see details below)
  • can be reduced by factorizing code in sub-functions (see details below)
  • performances might be improved by outputting all features into a single SHP file instead of one SHP file per feature [2] as addressed by #1778.

N.B.: Subsequent OTB functions called by this code should be reviewed as well.

Details (commented diff)

File: otbTestHelper.patch

diff --git a/Modules/IO/TestKernel/src/otbTestHelper.cxx b/Modules/IO/TestKernel/src/otbTestHelper.cxx
index 95128d65db..af6b5edfca 100644
--- a/Modules/IO/TestKernel/src/otbTestHelper.cxx
+++ b/Modules/IO/TestKernel/src/otbTestHelper.cxx
@@ -1718,6 +1718,8 @@ std::map<std::string, int> TestHelper::RegressionTestBaselines(char *baselineFil
 int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *baselineOgrFilename,
                                       const double toleranceDiffValue) const
 {
+  // Use assertions on input parameters.
+
   const char *ref_pszDataSource = baselineOgrFilename;
   const char *test_pszDataSource = testOgrFilename;
   //const char *ref_pszWHERE = NULL;
@@ -1727,6 +1729,12 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
   /* -------------------------------------------------------------------- */
   /*      Open data source.                                               */
   /* -------------------------------------------------------------------- */
+  //
+  // 1. Should be replaced by read-only otb::ogr::DataSource (in
+  // Adapters/GdalAdapters/include/otbOGRDataSourceWrappers.h)
+  // providing exception-safety (against memory-leaks).
+  //
+  // {
   GDALDataset *ref_poDS = nullptr;
   GDALDriver *  ref_poDriver = nullptr;
   //OGRGeometry *  ref_poSpatialFilter = NULL;
@@ -1776,43 +1784,66 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
       std::cout << "Had to open TEST data source read-only."<<std::endl;
       }
     }
+  // } (1)
   /* -------------------------------------------------------------------- */
   /*      Report failure                                                  */
   /* -------------------------------------------------------------------- */
+  //
+  // 2. Can be replaced by try/catch if (1)
+  //
+  // {
   if (ref_poDS == nullptr)
     {
-
     if (m_ReportErrors)
       {
       std::cout << "FAILURE:\n" "Unable to open REF datasource `" << ref_pszDataSource << "' with the following drivers." << std::endl;
 
+      // 2.1. Should be factorized into private function
+      // {
       std::vector<std::string> drivers = ogr::GetAvailableDriversAsStringVector();
 
       for (std::vector<std::string>::const_iterator it = drivers.begin();it!=drivers.end();++it)
         {
         std::cout << "  -> " << *it << std::endl;
         }
+      // } (2.1)
       }
+    // Memory-leak of GDALDataset instances in case of error, see (1)
+    // {
     return (1);
+    // }
     }
+  //
+  // } (2)
   ref_poDriver = ref_poDS->GetDriver();
   CPLAssert(ref_poDriver != NULL);
 
+  //
+  // 2. Can be replaced by try/catch if (1)
+  //
+  // {
   if (test_poDS == nullptr)
     {
     if (m_ReportErrors)
       {
       std::cout << "FAILURE:\n""Unable to open TEST datasource `" << test_pszDataSource << "' with the following drivers." << std::endl;
 
+      // 2.1. Should be factorized into private function
+      // {
       std::vector<std::string> drivers = ogr::GetAvailableDriversAsStringVector();
 
       for (std::vector<std::string>::const_iterator it = drivers.begin();it!=drivers.end();++it)
         {
         std::cout << "  -> " << *it << std::endl;
         }
+      // } (2.1)
       }
+    // Memory-leak of GDALDataset instances in case of error, see (1)
+    // {
     return (1);
+    // }
     }
+  // } (2)
   test_poDriver = test_poDS->GetDriver();
   CPLAssert(test_poDriver != NULL);
 
@@ -1823,11 +1854,21 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
 
   // TODO: Improve this check as it will stop as soon as one of the
   // list ends (i.e. it does not guarantee that all files are present)
+  //
+  // 3. This part is debatable and might be
+  // removed/improved/optimized: it checks that all auxiliary files
+  // are the same for both ref and test filenames thus making the
+  // hypothesis that both basenames are the same which is not
+  // necessarily needed.
+  //
+  // {
   std::vector<std::string> refFileList = otb::ogr::GetFileListAsStringVector(ref_poDS);
   std::vector<std::string> testFileList = otb::ogr::GetFileListAsStringVector(test_poDS);
 
   unsigned int fileId = 0;
 
+  // See TOTO above: maybe a simple
+  // refFileList.size()==testFileList.size() would be enough.
   while (fileId < refFileList.size() && fileId < testFileList.size())
     {
     std::string strRefName(refFileList[fileId]);
@@ -1843,7 +1884,7 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
       }
     ++fileId;
     }
-
+  // } (3)
   /* -------------------------------------------------------------------- */
   /*      Process each data source layer.                                 */
   /* -------------------------------------------------------------------- */
@@ -1851,6 +1892,8 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
 
   for (int iLayer = 0; iLayer < ref_poDS->GetLayerCount(); ++iLayer)
     {
+    // 4. See (1) and use safe OTB OGR wrappers.
+    // {
     OGRLayer *ref_poLayer = ref_poDS->GetLayer(iLayer);
     OGRLayer *test_poLayer = test_poDS->GetLayer(iLayer);
 
@@ -1859,7 +1902,10 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
       if (m_ReportErrors)
         std::cout << "FAILURE: Couldn't fetch advertised layer " << iLayer <<
         " for REF data source" << std::endl;
+      // 4.1. Memory-leak of OGRLayer if error
+      // {
       return (1);
+      // }
       }
     if (test_poLayer == nullptr)
       {
@@ -1868,6 +1914,7 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
         " for REF data source" << std::endl;
       return (1);
       }
+    // } (4)
 
     //Check Layer inforamtion
     ogrReportOnLayer(ref_poLayer, nullptr, nullptr, test_poLayer, nullptr, nullptr, nbdiff);
@@ -1875,8 +1922,11 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
     //If no difference, check the feature
     if (nbdiff == 0)
       {
+      // 5. Use safe OTB OGR wrappers, see (1) and (4)
+      // {
       OGRFeature *      ref_poFeature = nullptr;
       OGRFeature *      test_poFeature = nullptr;
+      // }
       std::string       basefilename(test_pszDataSource);
       int               nbFeature(0);
       std::stringstream oss2;
@@ -1890,29 +1940,48 @@ int TestHelper::RegressionTestOgrFile(const char *testOgrFilename, const char *b
         oss.str("");
         oss << nbFeature;
 
+	// 5.1. Should be factorized in private utilitary function.
+	// {
         std::string ref_filename = basefilename + "_temporary_layer_" + oss2.str() + "_feature_" + oss.str()
                                    + "_ref.txt";
         std::string test_filename = basefilename + "_temporary_layer_" + oss2.str() + "_feature_" + oss.str()
                                     + "_test.txt";
+	// }
+
+	// 5.2. Should be factorized in private utilitary
+	// sub-function using C++ file streams instead of C file API.
+	// {
         FILE *ref_f(nullptr);
         ref_f = fopen(ref_filename.c_str(), "w");
         if (ref_f == nullptr)
           {
+	  // 5.2.1 Memory-leak of OGRFeature, if error
+	  // {
           itkGenericExceptionMacro(<< "Impossible to create ASCII file <" << ref_filename << ">.");
+	  // } (5.2.1)
           }
         DumpOGRFeature(ref_f, ref_poFeature);
         OGRFeature::DestroyFeature( ref_poFeature );
         fclose(ref_f);
+	// } (5.2)
 
+	// 5.2. Should be factorized in private utilitary
+	// sub-function using C++ file stream API instead of C file
+	// API.
+	// {
         FILE *test_f(nullptr);
         test_f = fopen(test_filename.c_str(), "w");
         if (test_f == nullptr)
           {
+	  // 5.2.1 Memory-leak of OGRFeature, if error
+	  // {
           itkGenericExceptionMacro(<< "Impossible to create ASCII file <" << test_filename << ">.");
+	  // } (5.2.1)
           }
         DumpOGRFeature(test_f, test_poFeature);
         OGRFeature::DestroyFeature( test_poFeature );
         fclose(test_f);
+	// } (5.2)
 
         //Check ASCII comparison
         std::vector<std::string> ignoredLines;

Steps to reproduce

N.A.

Configuration information

Any.

Edited Jan 23, 2019 by Stéphane Albert
Assignee
Assign to
None
Milestone
None
Assign milestone
Time tracking
None
Due date
No due date
1
Labels
bug
Assign labels
  • View project labels
Reference: orfeotoolbox/otb#1819