[Lint] OpenFileGDB: rename constant to DEFAULT_STRING_WIDTH and add e…

…xtensive comments
makesoftwaresafe · Feb 1, 2024 · 6a9caf8 · 6a9caf8
1 parent 2650f83
commit 6a9caf8
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 8 deletions.
diff --git a/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h b/ogr/ogrsf_frmts/openfilegdb/ogr_openfilegdb.h
@@ -48,7 +48,17 @@ std::string OFGDBGenerateUUID();
 
 int OGROpenFileGDBIsComparisonOp(int op);
 
-constexpr int OPENFILEGDB_DEFAULT_STRING_WIDTH = 65536;
+// The FileGeodatabase format does not really allow strings of arbitrary width
+// in the XML and .gdbtable header declaration. They must have a non-zero
+// maximum width. But if we put it to a huge value (let's say 1 billion), this
+// causes crashes in some Esri products (cf #5952, perhaps they allocate
+// std::string's to that maximum size?).
+// Hence this default of a relative large but not too large
+// width when creating a OGR string field width of unspecified width.
+// Note that when opening a FileGeodatabase with string fields of that width,
+// we do not advertise it in OGRFieldDefn::GetWidth() but we advertise 0 instead,
+// to allow round-tripping.
+constexpr int DEFAULT_STRING_WIDTH = 65536;
 
 // UUID of object type
 constexpr const char *pszFolderTypeUUID =

diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp
@@ -661,10 +661,11 @@ int OGROpenFileGDBLayer::BuildLayerDefinition()
         oFieldDefn.SetAlternativeName(poGDBField->GetAlias().c_str());
         oFieldDefn.SetSubType(eSubType);
         // On creation in the FileGDB driver (GDBFieldTypeToLengthInBytes) if
-        // string width is 0, we pick up 65536 by default to mean unlimited
-        // string length, but we do not want to advertise such a big number.
+        // string width is 0, we pick up DEFAULT_STRING_WIDTH=65536 by default
+        // to mean unlimited string length, but we do not want to advertise
+        // such a big number.
         if (eType == OFTString &&
-            (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH ||
+            (nWidth < DEFAULT_STRING_WIDTH ||
              CPLTestBool(CPLGetConfigOption(
                  "OPENFILEGDB_REPORT_GENUINE_FIELD_WIDTH", "NO"))))
         {

diff --git a/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp b/ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer_write.cpp
@@ -1350,17 +1350,23 @@ OGRErr OGROpenFileGDBLayer::CreateField(const OGRFieldDefn *poFieldIn,
         nWidth = poField->GetWidth();
         if (nWidth == 0)
         {
-            // We can't use a 0 width value since that prevents ArcMap
-            // from editing (#5952)
-            nWidth = OPENFILEGDB_DEFAULT_STRING_WIDTH;
+            // Hard-coded non-zero default string width if the user doesn't
+            // override it with the below configuration option.
+            // See comment at declaration of DEFAULT_STRING_WIDTH for more
+            // details
+            nWidth = DEFAULT_STRING_WIDTH;
             if (const char *pszVal = CPLGetConfigOption(
                     "OPENFILEGDB_DEFAULT_STRING_WIDTH", nullptr))
             {
                 const int nVal = atoi(pszVal);
                 if (nVal >= 0)
                     nWidth = nVal;
             }
-            if (nWidth < OPENFILEGDB_DEFAULT_STRING_WIDTH)
+            // Advertise a non-zero user-modified width back to the created
+            // OGRFieldDefn, only if it is less than the hard-coded default
+            // value (this will avoid potential issues with excessively large
+            // field width afterwards)
+            if (nWidth < DEFAULT_STRING_WIDTH)
                 poField->SetWidth(nWidth);
         }
     }