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.