GDALDataset::ReportErrorV() and GDALRasterBand::ReportError(): avoid …

…formatting string to be 'tainted' by dataset name (https://github.com/OSGeo/gdal/security/code-scanning/522)
OSGeo · Jan 26, 2024 · cb5161d · cb5161d
1 parent 15a072a
commit cb5161d
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 23 deletions.
diff --git a/autotest/cpp/test_gdal.cpp b/autotest/cpp/test_gdal.cpp
@@ -2969,7 +2969,7 @@ TEST_F(test_gdal, GDALDatasetReportError)
     CPLPushErrorHandler(CPLQuietErrorHandler);
     poSrcDS->ReportError("%foo", CE_Warning, CPLE_AppDefined, "bar");
     CPLPopErrorHandler();
-    EXPECT_STREQ(CPLGetLastErrorMsg(), "bar");
+    EXPECT_STREQ(CPLGetLastErrorMsg(), "%foo: bar");
 
     CPLPushErrorHandler(CPLQuietErrorHandler);
     poSrcDS->ReportError(

diff --git a/gcore/gdaldataset.cpp b/gcore/gdaldataset.cpp
@@ -4540,20 +4540,14 @@ void GDALDataset::ReportErrorV(const char *pszDSName, CPLErr eErrClass,
                                CPLErrorNum err_no, const char *fmt,
                                va_list args)
 {
-    char szNewFmt[256] = {};
-    const size_t strlen_fmt = strlen(fmt);
-    constexpr const char *sep = ": ";
-    const size_t strlen_sep = strlen(sep);
-    if (strlen_fmt + strlen(pszDSName) + strlen_sep + 1 >= sizeof(szNewFmt) - 1)
-        pszDSName = CPLGetFilename(pszDSName);
-    const size_t strlen_dsname = strlen(pszDSName);
-    if (pszDSName[0] != '\0' && strchr(pszDSName, '%') == nullptr &&
-        strlen_fmt + strlen_dsname + strlen_sep + 1 < sizeof(szNewFmt) - 1)
-    {
-        memcpy(szNewFmt, pszDSName, strlen_dsname);
-        memcpy(szNewFmt + strlen_dsname, sep, strlen_sep);
-        memcpy(szNewFmt + strlen_dsname + strlen_sep, fmt, strlen_fmt + 1);
-        CPLErrorV(eErrClass, err_no, szNewFmt, args);
+    pszDSName = CPLGetFilename(pszDSName);
+    if (pszDSName[0] != '\0')
+    {
+        CPLError(eErrClass, err_no, "%s",
+                 std::string(pszDSName)
+                     .append(": ")
+                     .append(CPLString().vPrintf(fmt, args))
+                     .c_str());
     }
     else
     {

diff --git a/gcore/gdalrasterband.cpp b/gcore/gdalrasterband.cpp
@@ -7886,21 +7886,21 @@ void GDALRasterBand::ReportError(CPLErr eErrClass, CPLErrorNum err_no,
 
     va_start(args, fmt);
 
-    char szNewFmt[256] = {'\0'};
     const char *pszDSName = poDS ? poDS->GetDescription() : "";
-    if (strlen(fmt) + strlen(pszDSName) + 20 >= sizeof(szNewFmt) - 1)
-        pszDSName = CPLGetFilename(pszDSName);
-    if (pszDSName[0] != '\0' && strchr(pszDSName, '%') == nullptr &&
-        strlen(fmt) + strlen(pszDSName) + 20 < sizeof(szNewFmt) - 1)
+    pszDSName = CPLGetFilename(pszDSName);
+    if (pszDSName[0] != '\0')
     {
-        snprintf(szNewFmt, sizeof(szNewFmt), "%s, band %d: %s", pszDSName,
-                 GetBand(), fmt);
-        CPLErrorV(eErrClass, err_no, szNewFmt, args);
+        CPLError(eErrClass, err_no, "%s",
+                 CPLString()
+                     .Printf("%s, band %d: ", pszDSName, GetBand())
+                     .append(CPLString().vPrintf(fmt, args))
+                     .c_str());
     }
     else
     {
         CPLErrorV(eErrClass, err_no, fmt, args);
     }
+
     va_end(args);
 }
 #endif